feat(router): enabling MinimalRouterStateSerializer by default #2326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ter setup module Closes ngrx#2225
|
Preview docs changes for dcf794a at https://previews.ngrx.io/pr2326-dcf794a/ |
timdeschryver
requested changes
Jan 30, 2020
There was a problem hiding this comment.
Changes LGTM, but I have some docs comments.
Could you also update the line https://github.com/ngrx/platform/pull/2326/files#diff-8394e314f48c3713d48bf5c1946b3f23R13:
- `serializer`: How a router snapshot is serialized. Defaults to `DefaultRouterStateSerializer` (see [Default Router State Serializer](#default-router-state-serializer)). See [Custom Router State Serializer](#custom-router-state-serializer) for more information.
| url: string; | ||
| params: Params; | ||
| queryParams: Params; | ||
| url: string; |
There was a problem hiding this comment.
Seems like something weird has happened here with the formation, could you revert this change please?
| } | ||
|
|
||
| export class CustomSerializer implements RouterStateSerializer<RouterStateUrl> { | ||
| serialize(routerState: RouterStateSnapshot): RouterStateUrl { | ||
| let route = routerState.root; | ||
| serialize(routerState: RouterStateSnapshot): RouterStateUrl { |
| @@ -63,7 +63,8 @@ export class CustomSerializer implements RouterStateSerializer<RouterStateUrl | |||
| // Only return an object including the URL, params and query params | |||
| // instead of the entire snapshot | |||
| return { url, params, queryParams }; | |||
| } | |||
|
|
|||
| } | |||
| The `DefaultRouterStateSerializer` cannot be used when [serializability runtime checks](guide/store/configuration/runtime-checks) are enabled. If you want to use runtime checks to enforce serializability of your state and actions, you can configure `RouterStoreModule` to use the `MinimalRouterStateSerializer` or implement a custom router state serializer. | ||
| This also applies to Ivy with immutability runtime checks. | ||
| The `DefaultRouterStateSerializer` cannot be used when [serializability runtime checks](guide/store/configuration/runtime-checks) are enabled. | ||
| With runtime checks enabled `MinimalRouterStateSerializer` serializer is used by default in `RouterStoreModule` when no other serializer is provided. This also applies to Ivy with immutability runtime checks. |
There was a problem hiding this comment.
Suggested change
| With runtime checks enabled `MinimalRouterStateSerializer` serializer is used by default in `RouterStoreModule` when no other serializer is provided. This also applies to Ivy with immutability runtime checks. | |
| With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer **must** be used. This also applies to Ivy with immutability runtime checks. |
alex-okrushko
approved these changes
Jan 31, 2020
|
updated... |
brandonroberts
approved these changes
Feb 5, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
If no router state serializer is provided through the configuration of router store, the DefaultRouterStateSerializer is used.
Closes #2225
What is the new behavior?
If no router state serializer is provided through the configuration of router store, the MinimalRouterStateSerializer is used.
Does this PR introduce a breaking change?
The default option for
rooterStateinStoreRouterConnectingModuleis changing. If still need to supportDefaultRouterStateSerializerwill need to explicitly pass inRouterState.FullforrouterState.Other information
It is very confusing to me that
serializerwhich is set in_createRouterConfigdoes not play any effect right now. @timdeschryver said that it was done for backward compatibility reasons and probably will be addressed in another PR by @alex-okrushko. If not, we can discuss here how to make it better.