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

Router Store: make minimal router state serializer the default #2225

Closed
brandonroberts opened this issue Nov 7, 2019 · 9 comments · Fixed by #2326
Closed

Router Store: make minimal router state serializer the default #2225

brandonroberts opened this issue Nov 7, 2019 · 9 comments · Fixed by #2326

Comments

@brandonroberts
Copy link
Member

This provides the minimal set of router state that is fully immutable and serializable. Will have to do some coordination with libraries such as Nx that use the full router state for the navigation effects.

@evgenyfedorenko
Copy link
Contributor

I could take this one, likely will need more info

@timdeschryver
Copy link
Member

timdeschryver commented Dec 3, 2019

The short answer is to update the docs and update the config's default serializer to be the minimal router serializer.

But I'm assuming we would also want to update the action's to reflect the minimal payloads (e.g. usage of MinimalRouterSnapshot instead of the ActivatedRouterSnapshot).

@evgenyfedorenko
Copy link
Contributor

evgenyfedorenko commented Dec 6, 2019

I am playing with router store in ngrx example app and passing in full serializer

    StoreRouterConnectingModule.forRoot({
      routerState: RouterState.Full,
    }),

getting an error

ERROR Error: Detected unserializable action at "payload.routerState.root.paramMap"

Should we fix this?

@edbzn
Copy link
Contributor

edbzn commented Dec 6, 2019

@evgenyfedorenko Hi, the routerState configured to RouterState.Full isn't compatible with these runtime checks:

runtimeChecks: {
  strictStateSerializability: true,
  strictActionSerializability: true,
}

It's due to the prototypes chain from paramMap, queryParamMap and component properties. That's why the MinimalRouterStateSerializer should be set by the default I think.

@evgenyfedorenko
Copy link
Contributor

@timdeschryver Do we want to be able to invoke StoreRouterConnectingModule with 0 passed in config options and default serializer to RouterState.Minimal? Currently serializer param is optional and is derived from routerState param. If only serializer is passed but not routerState it breaks if strictActionSerializability enabled. This does not make sense to me since serializer is explicitly specified already.

@evgenyfedorenko
Copy link
Contributor

@timdeschryver in case you missed it.

@timdeschryver
Copy link
Member

The default options should be RouterState.Minimal and the serializer MinimalRouterStateSerializer.

If the serializer is passed, this change will most likely be a breaking change.
The fix for it would be to set the routerState to Default. This is what #2291 solves.

@alex-okrushko
Copy link
Member

@timdeschryver btw, I find it a bit confusing that both routerState and serializer are present in the config, and they could be contradicting each other. I think I have a solution for that.

@timdeschryver
Copy link
Member

@alex-okrushko I agree, if I remember correctly it was done like this to not have a breaking change.

I also want to point out that it's not just setting the serializer, it's also used to dispatch the router events. With minimal, it will only dispatch the router event id and the url.

https://github.com/ngrx/platform/blob/master/modules/router-store/src/router_store_module.ts#L319-L339

evgenyfedorenko added a commit to evgenyfedorenko/platform that referenced this issue Jan 21, 2020
evgenyfedorenko added a commit to evgenyfedorenko/platform that referenced this issue Feb 5, 2020
evgenyfedorenko added a commit to evgenyfedorenko/platform that referenced this issue Feb 5, 2020
brandonroberts pushed a commit that referenced this issue Feb 5, 2020
BREAKING CHANGE:

The MinimalRouterStateSerializer is enabled by default.

BEFORE:

If no router state serializer is provided through the configuration of router store, the DefaultRouterStateSerializer is used.

AFTER: 

If no router state serializer is provided through the configuration of router store, the MinimalRouterStateSerializer is used.

Closes #2225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants