feat(router-store): add routerState config option #1847

timdeschryver · 2019-05-09T19:17:45Z

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
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

By default, the router event exists on the router-store action e.g. NavigationStart and RoutesRecognized. The problem is that these events are not serializable.

Closes #1834

What is the new behavior?

This changes introduces a new config parameter, routerState which has two options : RouterState.Full and RouterState.Minimal. If set to Full, the current behavior remains. When set to Minimal, a new router serializer MinimalRouterStateSerializer will be used to serialize the router event, it will also only use the navigation id and the url from the router event to append to the @ngrx/router-store action.

If a serializer is already provided, it will continue to use the custom serializer.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

projects/example-app/src/app/app.module.ts

modules/router-store/src/router_store_module.ts

timdeschryver · 2019-05-10T20:32:54Z

Updated the whole PR with feedback given. I'm not quite happy on the docs part... feedback is welcome 😃

ngrxbot · 2019-05-10T20:38:55Z

Preview docs changes for d7a486e at https://previews.ngrx.io/pr1847-d7a486e/

brandonroberts · 2019-05-10T20:44:35Z

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

+
+### RouterState.Minimal
+
+`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize Angular router event.


Suggested change

`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize Angular router event.

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

brandonroberts · 2019-05-10T20:44:47Z

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

+
+The metadata on the action consists of the navigation id and the url.
+
+```ts


Suggested change

```ts

```json

brandonroberts · 2019-05-10T20:45:54Z

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.
+
+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 will contain the Angular router event, e.g. `NavigationStart` and `RoutesRecognized`.

brandonroberts · 2019-05-10T20:46:08Z

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

@@ -5,12 +5,14 @@ interface StoreRouterConfig {
  stateKey?: string | Selector&lt;any, RouterReducerState&lt;T&gt;&gt;;
  serializer?: new (...args: any[]) => RouterStateSerializer;
  navigationActionTiming?: NavigationActionTiming;
+  state?: RouterState;


Suggested change

state?: RouterState;

routerState?: RouterState;

brandonroberts · 2019-05-10T20:46:45Z

modules/router-store/src/router_store_module.ts

+  /**
+   * Decides which router serializer should be used, if there is none provided, and the metadata on the dispatched @ngrx/router-store action payload.
+   * Set to `Full` to use the `DefaultRouterStateSerializer` 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 nagivation id and url as payload.


Suggested change

* Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the nagivation 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.

brandonroberts · 2019-05-10T20:47:06Z

modules/router-store/spec/router_store_module.spec.ts

+    });
+
+    describe('Minimal', () => {
+      it('should dispatch the navidation id with url', async () => {


Suggested change

it('should dispatch the navidation id with url', async () => {

it('should dispatch the navigation id with url', async () => {

timdeschryver · 2019-05-10T20:54:24Z

Thanks Brandon!

ngrxbot · 2019-05-10T21:03:53Z

Preview docs changes for 78137b3 at https://previews.ngrx.io/pr1847-78137b3/

brandonroberts · 2019-05-13T14:09:01Z

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

+import { BaseRouterStoreState, RouterStateSerializer } from './shared';
+
+export interface MinimalRouteSnapshot {
+  params: RouterStateSnapshot['root']['params'];


Should all these be the same?

No they should not 😐

brandonroberts · 2019-05-14T12:26:48Z

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

@@ -88,3 +90,52 @@ StoreRouterConnectingModule.forRoot({
  navigationActionTiming: NavigationActionTiming.PostActivation,
 });
 </code-example>
+
+## routerState


Suggested change

## routerState

## Router State Snapshot

brandonroberts · 2019-05-14T12:28:25Z

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

+  params: RouterStateSnapshot['root']['params'];
+  url: RouterStateSnapshot['root']['params'];
+  queryParams: RouterStateSnapshot['root']['params'];
+  data: RouterStateSnapshot['root']['params'];


Suggested change

data: RouterStateSnapshot['root']['params'];

data: RouterStateSnapshot['root']['data'];

ngrxbot · 2019-05-14T19:40:25Z

Preview docs changes for 7c50710 at https://previews.ngrx.io/pr1847-7c50710/

brandonroberts · 2019-05-14T19:50:59Z

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

+  url: string;
+}
+
+export class MinimalRouterStateSerializer


After taking a second look at this, I think this should match what the custom serializer does in the docs. If we just give them the root snapshot, the params will be empty, the data will be empty, only leaving the url and global queryParams.

That would make more sense

I've pushed these changes. One caveat is that users won't be able to access params from higher up. Another implementation I've encountered a few times is to loop over all of the children and pluck the params from each child and build up an object with all of the params.

ngrxbot · 2019-05-14T20:52:55Z

Preview docs changes for 93266b0 at https://previews.ngrx.io/pr1847-93266b0/

timdeschryver · 2019-05-16T16:44:25Z

modules/router-store/spec/serializers.spec.ts

+    delete snapshot.queryParamMap;
+    delete snapshot.component;
+
+    // properties that do not exist on the minimal serializer


Is this OK to leave out?
I did this because these are always undefined

timdeschryver · 2019-05-16T16:44:54Z

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

+import { BaseRouterStoreState, RouterStateSerializer } from './shared';
+
+export interface MinimalActivatedRouteSnapshot {
+  routeConfig: ActivatedRouteSnapshot['routeConfig'];


Is there a better way to type this?

Other than to use its actual type of Route?

Yea, I tried to just use ActivatedRouteSnapshot and override some properties but I'm not sure if this is possible.

I'm not sure either

ngrxbot · 2019-05-16T16:54:22Z

Preview docs changes for 2f23e4c at https://previews.ngrx.io/pr1847-2f23e4c/

timdeschryver commented May 9, 2019

View reviewed changes

projects/example-app/src/app/app.module.ts Outdated Show resolved Hide resolved

adrianfaciu reviewed May 9, 2019

View reviewed changes

modules/router-store/src/router_store_module.ts Outdated Show resolved Hide resolved

feat(router-store): add routerState config option

d7a486e

timdeschryver force-pushed the pr/router-navigation-id branch from 07bd0cb to d7a486e Compare May 10, 2019 20:29

timdeschryver changed the title ~~feat(router-store): add onlyEventNavigationId config property~~ feat(router-store): add routerState config option May 10, 2019

brandonroberts requested changes May 10, 2019

View reviewed changes

chore: review changes

78137b3

brandonroberts reviewed May 13, 2019

View reviewed changes

brandonroberts requested changes May 14, 2019

View reviewed changes

chore: review changes

7c50710

brandonroberts requested changes May 14, 2019

View reviewed changes

feat: change MinimalRouterStateSerializer implementation

93266b0

feat: change MinimalRouterStateSerializer implementation

2f23e4c

timdeschryver commented May 16, 2019

View reviewed changes

brandonroberts approved these changes May 20, 2019

View reviewed changes

brandonroberts merged commit d874cfc into master May 20, 2019

brandonroberts deleted the pr/router-navigation-id branch May 20, 2019 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router-store): add routerState config option #1847

feat(router-store): add routerState config option #1847

timdeschryver commented May 9, 2019 •

edited

Loading

timdeschryver commented May 10, 2019 •

edited

Loading

ngrxbot commented May 10, 2019

brandonroberts May 10, 2019

brandonroberts May 10, 2019

brandonroberts May 10, 2019

brandonroberts May 10, 2019

brandonroberts May 10, 2019

brandonroberts May 10, 2019

timdeschryver commented May 10, 2019

ngrxbot commented May 10, 2019

brandonroberts May 13, 2019

timdeschryver May 14, 2019

brandonroberts May 14, 2019

brandonroberts May 14, 2019

ngrxbot commented May 14, 2019

brandonroberts May 14, 2019

timdeschryver May 14, 2019

timdeschryver May 14, 2019

ngrxbot commented May 14, 2019

timdeschryver May 16, 2019

brandonroberts May 16, 2019

timdeschryver May 16, 2019

brandonroberts May 16, 2019 •

edited

Loading

timdeschryver May 16, 2019

brandonroberts May 16, 2019

ngrxbot commented May 16, 2019


		### RouterState.Minimal

		`RouterState.Minimal` will use the `MinimalRouterStateSerializer` serializer to serialize Angular router event.


		The metadata on the action consists of the navigation id and the url.

		```ts


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

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

	* Set to `Minimal` to use the `MinimalRouterStateSerializer` and to set a minimal router event with the nagivation 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.

	it('should dispatch the navidation id with url', async () => {
	it('should dispatch the navigation id with url', async () => {

	data: RouterStateSnapshot['root']['params'];
	data: RouterStateSnapshot['root']['data'];

feat(router-store): add routerState config option #1847

feat(router-store): add routerState config option #1847

Conversation

timdeschryver commented May 9, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

timdeschryver commented May 10, 2019 • edited Loading

ngrxbot commented May 10, 2019

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 commented May 10, 2019

ngrxbot commented May 10, 2019

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

ngrxbot commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonroberts May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented May 16, 2019

timdeschryver commented May 9, 2019 •

edited

Loading

timdeschryver commented May 10, 2019 •

edited

Loading

brandonroberts May 16, 2019 •

edited

Loading