Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(RouterStore): serialize routeConfig inside the default serializer #1384

Merged
merged 1 commit into from
Oct 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions modules/router-store/spec/serializer.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { RouterStateSnapshot } from '@angular/router';
import { DefaultRouterStateSerializer } from '../src';

describe('serializer', () => {
it('should serialize all properties', () => {
const serializer = new DefaultRouterStateSerializer();
const snapshot = createSnapshot();
const routerState = {
url: 'url',
root: snapshot,
} as RouterStateSnapshot;

const actual = serializer.serialize(routerState);
const expected = {
url: 'url',
root: createExpectedSnapshot(),
};
expect(actual).toEqual(expected);
});

it('should serialize with an empty routeConfig', () => {
const serializer = new DefaultRouterStateSerializer();
const snapshot = { ...createSnapshot(), routeConfig: null };
const routerState = {
url: 'url',
root: snapshot,
} as RouterStateSnapshot;

const actual = serializer.serialize(routerState);
const expected = {
url: 'url',
root: {
...createExpectedSnapshot(),
routeConfig: null,
component: undefined,
},
};
expect(actual).toEqual(expected);
});

it('should serialize children', () => {
const serializer = new DefaultRouterStateSerializer();
const snapshot = {
...createSnapshot(),
children: [createSnapshot('child')],
};
const routerState = {
url: 'url',
root: snapshot,
} as RouterStateSnapshot;

const actual = serializer.serialize(routerState);

const expected = {
url: 'url',
root: {
...createExpectedSnapshot(),
firstChild: createExpectedSnapshot('child'),
children: [createExpectedSnapshot('child')],
},
};

expect(actual).toEqual(expected);
});

function createSnapshot(prefix = 'root'): any {
return {
params: `${prefix}-route.params`,
paramMap: `${prefix}-route.paramMap`,
data: `${prefix}-route.data`,
url: `${prefix}-route.url`,
outlet: `${prefix}-route.outlet`,
routeConfig: {
component: `${prefix}-route.routeConfig.component`,
path: `${prefix}-route.routeConfig.path`,
pathMatch: `${prefix}-route.routeConfig.pathMatch`,
redirectTo: `${prefix}-route.routeConfig.redirectTo`,
outlet: `${prefix}-route.routeConfig.outlet`,
},
queryParams: `${prefix}-route.queryParams`,
queryParamMap: `${prefix}-route.queryParamMap`,
fragment: `${prefix}-route.fragment`,
root: `${prefix}-route.root`,
parent: `${prefix}-route.parent`,
pathFromRoot: `${prefix}-route.params`,
firstChild: null,
children: [],
};
}

function createExpectedSnapshot(prefix = 'root') {
return {
...createSnapshot(prefix),
component: `${prefix}-route.routeConfig.component`,
root: undefined,
parent: undefined,
firstChild: undefined,
pathFromRoot: undefined,
};
}
});
12 changes: 9 additions & 3 deletions modules/router-store/src/serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,15 @@ export class DefaultRouterStateSerializer
data: route.data,
url: route.url,
outlet: route.outlet,
routeConfig: {
component: route.routeConfig ? route.routeConfig.component : undefined,
},
routeConfig: route.routeConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the routeConfig remain an empty object if its not defined to keep the existing behavior?

Copy link
Member Author

@timdeschryver timdeschryver Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially wanted to do this, but the Angular docs defines it as Route | null.
I wanted to stay as close as possible to this.

I should've payed more attention to when this... because this means that this is a breaking change.

What do you think of it? Should I change this to an empty object or flag this as a breaking change and add it to the migration log?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flag is as a breaking change. I'd rather the behavior be consistent and this would be the time to break it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this? I have some troubles to properly word this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

? {
component: route.routeConfig.component,
path: route.routeConfig.path,
pathMatch: route.routeConfig.pathMatch,
redirectTo: route.routeConfig.redirectTo,
outlet: route.routeConfig.outlet,
}
: null,
queryParams: route.queryParams,
queryParamMap: route.queryParamMap,
fragment: route.fragment,
Expand Down
20 changes: 20 additions & 0 deletions projects/ngrx.io/content/guide/migration/v7.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,26 @@ AFTER:
StoreRouterConnectingModule.forRoot(),
```

### ActivatedRouteSnapshot.RouteConfig

The default router serializer now returns a `null` value for `routeConfig` when `routeConfig` doesn't exist on the `ActivatedRouteSnapshot` instead of an empty object.

BEFORE:

```json
{
"routeConfig": {}
}
```

AFTER:

```json
{
"routeConfig": null
}
```

## @ngrx/store-devtools

The devtools is using the new `@ngrx/store-devtools/recompute` action to recompute its state instead of the `@ngrx/store/update-reducers` action.