-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
routeConfig: { | ||
component: route.routeConfig ? route.routeConfig.component : undefined, | ||
}, | ||
routeConfig: route.routeConfig |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
BREAKING CHANGE: 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 } ```
5b2b44c
to
92ab3bd
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The
routeConfig
isn't being serialized inDefaultRouterStateSerializer
.Issue Number: #1363
What is the new behavior?
The
routeConfig
is being serialized inDefaultRouterStateSerializer
.Note that not all properies are being serialized:
Does this PR introduce a breaking change?
The default router serializer now returns a
null
value forrouteConfig
whenrouteConfig
doesn't exist on theActivatedRouteSnapshot
instead of an empty object.BEFORE:
{ "routeConfig": {} }
AFTER:
Other information
If this doesn't belong in the default serializer, feel free to close.
It's always possible for the impacted users to create there own serializer.
Closes #1363