refactor(router-store): change serializer names #3430

david-shortman · 2022-05-25T06:30:36Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #3416

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

Includes migration

timdeschryver

I was busy reviewing the PR, and I think it's good.
But it also made me thinking, is "default" serializer a good term?
What about keeping minimal en full, and mention in the docs/ts-docs that minimal is the default? This way, it would be easier to make changes to it in the future, and it's also giving more information in comparison to "default".
I think the term minimal and default is now used in combination.

Thoughts?
Regardless of the result, I'll go over the PR once again in more detail.

markostanimirovic · 2022-05-27T09:31:43Z

What about keeping minimal en full, and mention in the docs/ts-docs that minimal is the default?

+1

david-shortman · 2022-05-27T12:12:28Z

I was busy reviewing the PR, and I think it's good.

But it also made me thinking, is "default" serializer a good term?

What about keeping minimal en full, and mention in the docs/ts-docs that minimal is the default? This way, it would be easier to make changes to it in the future, and it's also giving more information in comparison to "default".

I think the term minimal and default is now used in combination.

Thoughts?

Regardless of the result, I'll go over the PR once again in more detail.

Agreed. Will update.

netlify · 2022-05-30T04:05:06Z

✅ Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`9c6d631`
🔍 Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/6296b7019d28fe000990641a
😎 Deploy Preview	https://deploy-preview-3430--ngrx-io.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

brandonroberts · 2022-05-30T16:22:14Z

modules/router-store/src/serializers/full_serializer.ts

@@ -6,7 +6,10 @@ export interface SerializedRouterStateSnapshot extends BaseRouterStoreState {
  url: string;
 }

-export class DefaultRouterStateSerializer
+/**
+ * @deprecated The FullRouterStateSerializer cannot be used when serializability runtime checks are enabled. Use the {@link MinimalRouterStateSerializer} or a {@link https://ngrx.io/guide/router-store/configuration#custom-router-state-serializer custom serializer} instead.


I'm not sure we need to deprecate this yet. Deprecation means this could be removed at some point. It's still a valid serializer given its limitations. At some point there was a discussion about making the router state serializable.

timdeschryver

Looking good!
Left a few remarks for the docs.

timdeschryver · 2022-05-30T16:18:41Z

projects/ngrx.io/content/guide/router-store/configuration.md

+<div class="alert is-important">
+
+The `FullRouterStateSerializer` cannot be used when [serializability runtime checks](guide/store/configuration/runtime-checks) are enabled.
+With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer **must** be used. This also applies to Ivy with immutability runtime checks.


We don't freeze Ivy properties anymore, so this isn't correct anymore.

Suggested change

With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer **must** be used. This also applies to Ivy with immutability runtime checks.

With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer **must** be used.

timdeschryver · 2022-05-30T16:18:53Z

projects/ngrx.io/content/guide/store/configuration/runtime-checks.md

@@ -39,7 +39,7 @@ export class AppModule {}

 <div class="alert is-important">

-The serializability runtime checks cannot be enabled if you use `@ngrx/router-store` with the `DefaultRouterStateSerializer`. The [default serializer](guide/router-store/configuration) 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.
+The serializability runtime checks cannot be enabled if you use `@ngrx/router-store` with the `FullRouterStateSerializer`. The [full serializer](guide/router-store/configuration) 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.


Suggested change

This also applies to Ivy with immutability runtime checks.

timdeschryver · 2022-05-30T16:19:10Z

projects/ngrx.io/content/guide/router-store/configuration.md


-When this property is set to `RouterState.Full`, `@ngrx/router-store` will use the `DefaultRouterStateSerializer` serializer to serialize the Angular router event.
+`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.


Suggested change

`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.

`RouterState.Minimal` uses the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.

timdeschryver · 2022-05-30T16:19:34Z

projects/ngrx.io/content/guide/router-store/configuration.md


-`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.
+When this property is set to `RouterState.Full`, `@ngrx/router-store` will use the `FullRouterStateSerializer` serializer to serialize the Angular router event.


Suggested change

When this property is set to `RouterState.Full`, `@ngrx/router-store` will use the `FullRouterStateSerializer` serializer to serialize the Angular router event.

When this property is set to `RouterState.Full`, `@ngrx/router-store` uses the `FullRouterStateSerializer` serializer to serialize the Angular router event.

timdeschryver · 2022-05-30T16:19:43Z

projects/ngrx.io/content/guide/router-store/configuration.md


-The difference between `MinimalRouterStateSerializer` and the default `RouterStateSerializer` is that this serializer is fully serializable. To make the state and the actions serializable, the properties `paramMap`, `queryParamMap` and `component` are ignored.
+The metadata on the action will contain the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.


Suggested change

The metadata on the action will contain the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.

The metadata on the action contains the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.

timdeschryver · 2022-05-30T16:25:09Z

modules/router-store/src/serializers/full_serializer.ts

@@ -6,7 +6,10 @@ export interface SerializedRouterStateSnapshot extends BaseRouterStoreState {
  url: string;
 }

-export class DefaultRouterStateSerializer
+/**
+ * @deprecated The FullRouterStateSerializer cannot be used when serializability runtime checks are enabled. Use the {@link MinimalRouterStateSerializer} or a {@link https://ngrx.io/guide/router-store/configuration#custom-router-state-serializer custom serializer} instead.


I'm not sure if we should deprecate this.
It's recommended to use the minimal serializer, but some (a lot?) applications have serializability disabled, making this serializer valid.

Thoughts?

timdeschryver · 2022-05-30T16:25:50Z

modules/router-store/src/router_store_module.ts

+   * Set to `Full` to use the `FullRouterStateSerializer` and to set the angular router events as payload.
   * Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the navigation id and url as payload.


Because minimal is the default now:

Suggested change

* Set to `Full` to use the `FullRouterStateSerializer` and to set the angular router events as payload.

* Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the navigation id and url as payload.

* Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the navigation id and url as payload.

* Set to `Full` to use the `FullRouterStateSerializer` and to set the angular router events as payload.

timdeschryver · 2022-05-30T16:26:01Z

modules/router-store/src/router_store_module.ts

+ * Full = Serializes the router event with FullRouterStateSerializer
 * Minimal = Serializes the router event with MinimalRouterStateSerializer


Suggested change

* Full = Serializes the router event with FullRouterStateSerializer

* Minimal = Serializes the router event with MinimalRouterStateSerializer

* Minimal = Serializes the router event with MinimalRouterStateSerializer

* Full = Serializes the router event with FullRouterStateSerializer

timdeschryver

👍

david-shortman added 2 commits May 25, 2022 02:28

refactor(router-store): change serializer names

5ea580d

fixed lint for migration file

30dc1ba

timdeschryver added Breaking Change 14.x labels May 26, 2022

timdeschryver reviewed May 27, 2022

View reviewed changes

revert minimal serializer name

75efb22

Merge branch 'master' into rename-MinimalRouterStateSerializer

ea340fc

david-shortman requested a review from timdeschryver May 30, 2022 04:27

brandonroberts approved these changes May 30, 2022

View reviewed changes

timdeschryver reviewed May 30, 2022

View reviewed changes

david-shortman added 2 commits May 31, 2022 20:42

Merge branch 'master' into rename-MinimalRouterStateSerializer

0f01e06

applied PR feedback

9c6d631

david-shortman requested a review from timdeschryver June 1, 2022 00:47

timdeschryver approved these changes Jun 1, 2022

View reviewed changes

brandonroberts approved these changes Jun 6, 2022

View reviewed changes

brandonroberts merged commit d443f50 into ngrx:master Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(router-store): change serializer names #3430

refactor(router-store): change serializer names #3430

david-shortman commented May 25, 2022

timdeschryver left a comment

markostanimirovic commented May 27, 2022

david-shortman commented May 27, 2022

netlify bot commented May 30, 2022 •

edited

Loading

brandonroberts May 30, 2022

timdeschryver left a comment

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver May 30, 2022

timdeschryver left a comment

	With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer must be used. This also applies to Ivy with immutability runtime checks.
	With serializability runtime checks enabled, the `MinimalRouterStateSerializer` serializer must be used.


		When this property is set to `RouterState.Full`, `@ngrx/router-store` will use the `DefaultRouterStateSerializer` serializer to serialize the Angular router event.
		`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.

	`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.
	`RouterState.Minimal` uses the `MinimalRouterStateSerializer` serializer to serialize the Angular Router's `RouterState` and `RouterEvent`.


		The difference between `MinimalRouterStateSerializer` and the default `RouterStateSerializer` is that this serializer is fully serializable. To make the state and the actions serializable, the properties `paramMap`, `queryParamMap` and `component` are ignored.
		The metadata on the action will contain the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.

	The metadata on the action will contain the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.
	The metadata on the action contains the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.

		* Set to `Full` to use the `FullRouterStateSerializer` and to set the angular router events as payload.
		* Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the navigation id and url as payload.

		* Full = Serializes the router event with FullRouterStateSerializer
		* Minimal = Serializes the router event with MinimalRouterStateSerializer

refactor(router-store): change serializer names #3430

refactor(router-store): change serializer names #3430

Conversation

david-shortman commented May 25, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

timdeschryver left a comment

Choose a reason for hiding this comment

markostanimirovic commented May 27, 2022

david-shortman commented May 27, 2022

netlify bot commented May 30, 2022 • edited Loading

✅ Deploy Preview for ngrx-io ready!

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

netlify bot commented May 30, 2022 •

edited

Loading