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.

Sign up for GitHub

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

Jump to bottom

fix(router-store): Handle navigation error, cancel/error action contain real previous state #1294

Merged
merged 2 commits into from
Aug 27, 2018
Merged

fix(router-store): Handle navigation error, cancel/error action contain real previous state #1294

merged 2 commits into from
Aug 27, 2018

Conversation

dummdidumm
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] 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 new behavior?

Fixes #1292, #1293

  • Error during navigation initiated by store is caught and handed to ErrorHandler
  • ROUTER_ERROR / ROUTER_CANCEL action now contain previos router and store state

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@coveralls
Copy link

coveralls commented Aug 25, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 88.75% when pulling 7894950 on dummdidumm:feature/router into 5dac795 on ngrx:master.

timdeschryver
timdeschryver reviewed Aug 26, 2018
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some small remarks.

Also I'm not really at home (yet!) with router-store but I see that this.trigger !== RouterTrigger.STORE is used everywhere, is this done for a reason?
I tested a few cases and my trigger was always NONE, so I would find it safer to write this.trigger === RouterTrigger.NONE (after this change the tests were also passing). Not saying you have to change it, just wanting to understand it more.

modules/router-store/spec/integration.spec.ts Outdated
@@ -119,7 +126,7 @@ describe('integration spec', () => {
});
});

it('should support rolling back if navigation gets canceled', (done: any) => {
it('should support rolling back if navigation gets canceled (navigation initialized through roter)', (done: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, roter -> router

modules/router-store/src/router_store_module.ts Outdated
@@ -301,7 +303,8 @@ export class StoreRouterConnectingModule {
private store: Store<any>,
private router: Router,
private serializer: RouterStateSerializer<SerializedRouterStateSnapshot>,
@Inject(ROUTER_CONFIG) private config: StoreRouterConfig
@Inject(ROUTER_CONFIG) private config: StoreRouterConfig,
private errorHandler: ErrorHandler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to above the @Inject parameter.

modules/router-store/src/router_store_module.ts Outdated
this.trigger = RouterTrigger.STORE;
this.router.navigateByUrl(url);
this.router.navigateByUrl(url).catch(e => {
this.errorHandler.handleError(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should write a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There already is, kind of, the "...if navigation errors (navigation initiated by store)"-test. I will enhance it to check the error handler is invoked correctly.

modules/router-store/src/router_store_module.ts Outdated
this.trigger = RouterTrigger.STORE;
this.router.navigateByUrl(url);
this.router.navigateByUrl(url).catch(e => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename e to err

modules/router-store/src/router_store_module.ts
),
});
}

private dispatchRouterCancel(event: NavigationCancel): void {
this.dispatchRouterAction(ROUTER_CANCEL, {
routerState: this.routerState,
routerState: this.routerState!,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this.routerState! is used?
Is it to make clear the router state can't be null at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the only reason.

@dummdidumm
Copy link
Contributor Author

The RouterTrigger enum indicates who initiated the navigation and who is currently in control of it. If the navigation is initiated through the router, f.e. with router.navigate(...), then the trigger is ROUTER and changes to the router store state should not trigger another router.navigateByUrl(..). If the navigation is initiated through the store, then the trigger is STORE, router.navigateByUrl(...) is invoked, but we don't want router actions changing the store state to be dispatched during that. NONE means that either one of them can now initiate a new cycle. That's why this.trigger !== RouterTrigger.STORE / this.trigger !== RouterTrigger.ROUTER is important.

@brandonroberts brandonroberts mentioned this pull request Aug 27, 2018
Closed
@brandonroberts brandonroberts merged commit 5300e7d into ngrx:master Aug 27, 2018
@brandonroberts brandonroberts mentioned this pull request Aug 27, 2018
Closed
@dummdidumm dummdidumm mentioned this pull request Aug 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timdeschryver timdeschryver timdeschryver left review comments

@brandonroberts brandonroberts brandonroberts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Router-Store: Navigation error resulting from store initiated route change is unhandled
4 participants
@dummdidumm @coveralls @brandonroberts @timdeschryver