strictActionSerializability does not work with RouterModule (and custom serializer)

[adrianfaciu]

Minimal reproduction of the bug/regression with instructions:

Steps to reproduce:

• create a new angular project and add ngrx store & router modules
• use the router module with a custom serializer (the one from the docs is perfect)
• trigger any router navigation

Due to #1630, strictActionSerializability cannot be used with the DefaultRouterStateSerializer, but it should work with a custom one, for example the one from the docs.

Right now one get's an error: 'ERROR Error: "Detected unserializable action at "payload.event""''

This looks to be triggered in the isPlainObject method that is used in the getUnserializable method. All the router events extend the base RouterEvent class so the prototype check will fail targetPrototype === Object.prototype .

GitHub repo here

Expected behavior:

Should be able to use strictActionSerializability with RouterModule.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

Angular version: 8.0.0-rc.2
NgRx version: 8.0.0-beta.1

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[adrianfaciu]

Would like to fix this, but not sure of a good approach. @timdeschryver if you have any hints, they will be more than welcome 😃

[timdeschryver]

Yes, we're aware of this and decided to hold of this because it would be a major breaking change. The Nx package also makes use of the unserializable properties. We made these checks opt-in so devs would have the time to transition.

For more info see #1613 and #1630.

I'm going to close this issue, but if you have any ideas or remarks feel free to share them 😃

[adrianfaciu]

My bad @timdeschryver, I was following the PR and the issue, but somehow I've missed this 😞. I thought it's just about the snapshot, just noticed the relevant part with:

          // Make the router event serializable
          // Creates a POJO from a class instance
          event: { ...payload.event },

I was not affected by the snapshot, I am using a custom serializer, but the router event still prevents me from using this.

Is there any breaking change if only the part from above would be added ? That might be safe and will enable the usage for this scenario.

Runtime checks rock! Awesome work! 👍

[timdeschryver]

If I remember correctly, I have tried that but sadly it did not work. There are some properties that just aren't serializable. In #1630 these are removed, if you do not need these properties, you could create a custom serializer without these properties.

Sorry misread the question. Sadly, this would still be a breaking change because the methods available on the navigation events would all be removed:

class Rectangle {
  constructor(height, width) {
    this.height = height;
    this.width = width;
  }

  print() {
    return {h: this.height, w: this.width}
  }
}

const r = new Rectangle(10, 5);
r.print(); // {h: 10, w: 5}

const r2 = {...r};
r2.print(); // Uncaught TypeError: r2.print is not a function
``

[timdeschryver]

As a solution we could use the spread operator, only if the runtime check is on.
What do you think @brandonroberts ?

[brandonroberts]

Something feels weird about modifying the router state in development based on runtime checks. If I recall correctly, the router events have a toString() method on them. Maybe we using the stringified version through configuration. I don't know if people actually use those events as they are today.

[timdeschryver]

I do like your proposition more.
After a quick look, I think we only use navigationId from the router event. This is inside the routerReducer - https://github.com/ngrx/platform/blob/master/modules/router-store/src/reducer.ts#L34

Instead of stringifying the event, could we just have the navigationId on the event? I'm not sure what one could do with a stringified version of the event, and we do need to extract the navigationId from it.

The reason why I would like to have this feature is because otherwise the serializability checks are not use able in combination with @ngrx/router-store.

[brandonroberts]

Yea, I like that option better also to encourage usage of the checks.

[timdeschryver]

👍 I will make the necessary changes.

[brandonroberts]

@timdeschryver if @adrianfaciu wants to fix it, you can help him through it instead.

[adrianfaciu]

It's ok, @timdeschryver already asked me :) He can go on with the changes. I'll have plenty of other issues to help with 😄

[brandonroberts]

👍

[AdditionAddict]

In case any one else ends up here I fixed a bug ERROR Error: "Detected unserializable action at "payload.event with this message using

    StoreRouterConnectingModule.forRoot({
      routerState: RouterState.Minimal,
      serializer: CustomSerializer,
    }),

See docs - https://ngrx.io/guide/router-store/configuration#router-state-snapshot

[JoinThisBand]

In case any one else ends up here I fixed a bug ERROR Error: "Detected unserializable action at "payload.event with this message using

    StoreRouterConnectingModule.forRoot({
      routerState: RouterState.Minimal,
      serializer: CustomSerializer,
    }),

See docs - https://ngrx.io/guide/router-store/configuration#router-state-snapshot

Thanks - pointed me in the right direction.

Alternatively, you can disable serializability runtime checks as per: https://ngrx.io/guide/store/configuration/runtime-checks - see below:

The serializability runtime checks cannot be enabled if you use @ngrx/router-store with the DefaultRouterStateSerializer. The default serializer has an unserializable router state and actions that are not serializable. To use the serializability runtime checks either use the MinimalRouterStateSerializer or implement a custom router state serializer. This also applies to Ivy with immutability runtime checks.