feat(Router-Store): New actions, config enhancement #1267

dummdidumm · 2018-08-21T19:44:22Z

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)
Docs have 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)
[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?

What is the new behavior?

Closes #1010, #975, #1262, #1263

New actions ROUTER_REQUEST and ROUTER_NAVIGATED that are dispatched at the very start/end of the navigation cycle
ROUTER_NAVIGATION action can now be configured to be dispatched after guards and resolvers
Custom router state serializer can now be passed as part of config
Internals: No longer using internal Angular router hook, instead official router events

Does this PR introduce a breaking change?

[x] Yes
[] No

StoreRouterConfigFunction is removed. It is no longer possible to pass a function returning a StoreRouterConfig to StoreRouterConnectingModule.forRoot

If you still need this, pass a provider like this:

{
      provide: ROUTER_CONFIG,
      useFactory: _createRouterConfig // your function
}

Other information

I will update the documentation if the changes are approved.

…rnal router hook, more typesafety for router state

…GATED Part of ngrx#1010 ngrx#1263

Part of ngrx#1010, fixes ngrx#1262

Part of ngrx#1263

coveralls · 2018-08-21T19:46:48Z

Coverage increased (+0.2%) to 88.769% when pulling 329b0b8 on dummdidumm:feature/router into 4223ff9 on ngrx:master.

brandonroberts · 2018-08-22T04:20:22Z

modules/router-store/src/serializer.ts

+ * Simple router state.
+ * All custom router states / state serializers should have at least this property.
+ */
+export interface RouterState {


This interface needs a different name as the @angular/router already exposes a RouterState. It also needs to be added to src/index.ts

brandonroberts · 2018-08-22T04:21:57Z

modules/router-store/src/router_store_module.ts

 } from './serializer';

+/**


These new exports need to be added to src/index.ts

brandonroberts · 2018-08-22T04:28:36Z

modules/router-store/src/router_store_module.ts

+   * there may be a navigation cancel due to a guard saying the navigation is not allowed.
+   * To run ROUTER_NAVIGATION after guards and resolvers, set this property to true.
+   */
+  dispatchNavActionOnEnd?: boolean;


I think this should be more flexible.

enum NavigationAction { PreActivation = 1, PostActivation = 2 }; navigationAction?: NavigationAction

The default would be PreActivation

brandonroberts · 2018-08-22T04:30:27Z

modules/router-store/src/router_store_module.ts

 export type StoreRouterConfigFunction = () => StoreRouterConfig;

+enum RouterTrigger {
+  NONE,


Provide explicit values

brandonroberts · 2018-08-22T04:32:27Z

modules/router-store/src/router_store_module.ts

+): RouterStateSerializer {
+  // This function gets handed a complete config-object from _createRouterConfig,
+  // so we know the serializer property exists
+  return new config.serializer!();


This doesn't support dependencies being injected into the serializer

Is this even possible without jumping through fire hoops? The problem is that we don't know the dependencies, so we somehow have to get them dynamically. But to my knowledge, getting these and making sure they exist (and those who are transitively required, too) is not really supported by Angular through some public API. It is only supported to statically define the dependencies.

So we would end up either using Angular internals which is brittle, or write the DI-mechanisms again.

If this is not really possible, maybe we should ditch the serializer option..

@timdeschryver any ideas?

I didn't have the time to take this all in, I'll try to get back at this later today.

It's already being injected in the constructor, which will resolve its dependencies. Just use the config to tell it which one to provide. Remove the StoreRouterConfigFunction and move all the providers into the forRoot method. It will be breaking, but more consistent

static forRoot( config: StoreRouterConfig = {} ): ModuleWithProviders { return { ngModule: StoreRouterConnectingModule, providers: [ { provide: _ROUTER_CONFIG, useValue: config }, { provide: RouterStateSerializer, useClass: config.serializer ? config.serializer : DefaultRouterStateSerializer } ], }; }

Ok when there is no function to execute it will work (with AOT, too).

brandonroberts · 2018-08-22T12:32:28Z

modules/router-store/src/serializer.ts

+ * Simple router state.
+ * All custom router states / state serializers should have at least this property.
+ */
+export interface SimpleRouterState {


What about BaseRouterState?

Or BaseRouterStoreState

brandonroberts · 2018-08-22T12:37:55Z

modules/router-store/src/router_store_module.ts

+): RouterStateSerializer {
+  // This function gets handed a complete config-object from _createRouterConfig,
+  // so we know the serializer property exists
+  return new config.serializer!();


It's already being injected in the constructor, which will resolve its dependencies. Just use the config to tell it which one to provide. Remove the StoreRouterConfigFunction and move all the providers into the forRoot method. It will be breaking, but more consistent

static forRoot( config: StoreRouterConfig = {} ): ModuleWithProviders { return { ngModule: StoreRouterConnectingModule, providers: [ { provide: _ROUTER_CONFIG, useValue: config }, { provide: RouterStateSerializer, useClass: config.serializer ? config.serializer : DefaultRouterStateSerializer } ], }; }

BREAKING CHANGE: StoreRouterConfigFunction is removed. It is no longer possible to pass a function returning a StoreRouterConfig to StoreRouterConnectingModule.forRoot If you still need this, pass a provider like this: { provide: ROUTER_CONFIG, useFactory: _createRouterConfig // you function }

brandonroberts · 2018-08-24T02:41:02Z

Thanks @dummdidumm!

sbkpilot1 · 2018-08-28T22:22:53Z

Hi Brandon, I am having this same issue, with which release is the ROUTER_NAVIGATED action going to be available? Thanks!!

brandonroberts · 2018-08-29T01:57:03Z

@sbkpilot1 it won't go out until the next release, which isn't ready yet. If you want to use it now you'll need to use the nightly builds

jasanst · 2018-09-04T06:58:44Z

Did the documentation get released before the functionality?

I'm having trouble using the serializer in StoreRouterConnectingModule.forRoot as shown in the api - it doesn't seem to exist yet?

timdeschryver · 2018-09-04T07:55:11Z

@jasanst yea it's an unreleased feature.
If you want you can install the packages from the nightly builds.

jasanst · 2018-09-05T00:57:36Z

Thanks, just getting started so would rather see documentation for the current prod version instead, is this still available?

brandonroberts · 2018-09-05T03:07:42Z

@jasanst you can get the latest release on npm through the tag. https://github.com/ngrx/platform/tree/6.1.0

kellymarjorie · 2018-10-02T20:26:06Z

@brandonroberts - ETA on next stable release?

sadsa · 2018-11-19T23:32:37Z

bump

DonatasD · 2019-01-22T03:55:03Z

@brandonroberts Is this fix available in any 6.x.x releases or is it only available in 7.x.x?

timdeschryver · 2019-01-22T20:30:19Z

@DonatasD this is available starting from 7.x.x

dummdidumm added 4 commits August 21, 2018 21:31

refactor(router-store): simplified logic a bit, not longer using inte…

a77427d

…rnal router hook, more typesafety for router state

feat(router-store): New router Actions ROUTER_REQUEST and ROUTER_NAVI…

f7985b2

…GATED Part of ngrx#1010 ngrx#1263

feat(router-store): Add custom serializer to config object

2a002a2

Part of ngrx#1010, fixes ngrx#1262

feat(router-store): config option to dispatch ROUTER_NAVIGATION later

cf6fade

Part of ngrx#1263

fix(router-store): Avoiding @ngrx/effects dependency inside tests

e66167a

brandonroberts requested changes Aug 22, 2018

View reviewed changes

fix(router-store): Added new imports to index.ts, codestyle

1a384fc

brandonroberts reviewed Aug 22, 2018

View reviewed changes

dummdidumm added 2 commits August 22, 2018 15:50

refactor(router-store): Rename base router store state interface

329b0b8

brandonroberts approved these changes Aug 24, 2018

View reviewed changes

brandonroberts merged commit 8a5e041 into ngrx:master Aug 24, 2018

This was referenced Aug 24, 2018

NgRx router integration dispatches the navigation event before the preactivation phase (Guards and Resolvers) executes. #975

Closed

Router-Store: Provide configuration for pre/post router navigation action #1263

Closed

timdeschryver mentioned this pull request Aug 24, 2018

Update documentation route store #1281

Closed

dummdidumm mentioned this pull request Aug 25, 2018

Router Store does not wait for deactivate guards to finish #816

Closed

timdeschryver mentioned this pull request Jan 13, 2019

feat(RouterStore): add routerState to action payload #1511

Merged

3 tasks

MaximSagan mentioned this pull request Apr 23, 2019

Router Store: Store confirmed state (state on NavigationEnd) explicitly #1784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Router-Store): New actions, config enhancement #1267

feat(Router-Store): New actions, config enhancement #1267

dummdidumm commented Aug 21, 2018 •

edited

Loading

coveralls commented Aug 21, 2018 •

edited

Loading

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018

dummdidumm Aug 22, 2018

timdeschryver Aug 22, 2018

brandonroberts Aug 22, 2018 •

edited

Loading

dummdidumm Aug 22, 2018

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018

brandonroberts Aug 22, 2018 •

edited

Loading

brandonroberts commented Aug 24, 2018

sbkpilot1 commented Aug 28, 2018 •

edited

Loading

brandonroberts commented Aug 29, 2018

jasanst commented Sep 4, 2018

timdeschryver commented Sep 4, 2018

jasanst commented Sep 5, 2018

brandonroberts commented Sep 5, 2018

kellymarjorie commented Oct 2, 2018

sadsa commented Nov 19, 2018

DonatasD commented Jan 22, 2019 •

edited

Loading

timdeschryver commented Jan 22, 2019

feat(Router-Store): New actions, config enhancement #1267

feat(Router-Store): New actions, config enhancement #1267

Conversation

dummdidumm commented Aug 21, 2018 • 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

coveralls commented Aug 21, 2018 • edited Loading

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

brandonroberts Aug 22, 2018 • edited Loading

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

brandonroberts Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

brandonroberts commented Aug 24, 2018

sbkpilot1 commented Aug 28, 2018 • edited Loading

brandonroberts commented Aug 29, 2018

jasanst commented Sep 4, 2018

timdeschryver commented Sep 4, 2018

jasanst commented Sep 5, 2018

brandonroberts commented Sep 5, 2018

kellymarjorie commented Oct 2, 2018

sadsa commented Nov 19, 2018

DonatasD commented Jan 22, 2019 • edited Loading

timdeschryver commented Jan 22, 2019

dummdidumm commented Aug 21, 2018 •

edited

Loading

coveralls commented Aug 21, 2018 •

edited

Loading

brandonroberts Aug 22, 2018 •

edited

Loading

brandonroberts Aug 22, 2018 •

edited

Loading

sbkpilot1 commented Aug 28, 2018 •

edited

Loading

DonatasD commented Jan 22, 2019 •

edited

Loading