refactor(router): compute correct history restoration when navigation is cancelled

[aahmedayed]

We can’t determine whether the user actually meant the back or
the forward using the popstate event (triggered by a browser
back/forward)
so we instead need to store information on the state and compute the
distance the user is traveling withing the browser history.
So by using the History#go method,
we can bring the user back to the page where he is supposed to be after
performing the action.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/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
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[atscott]

Hi @aahmedayed - this looks promising! What do you think of breaking this up into two PRs? I think a PR to add go/goTo as a feature separately would be valuable on its own. Then have a separate PR to fix the routing history with guards. This would make the review process much easier.

I haven't reviewed the changes to the router yet, as that will take a little more time and careful review.

[aahmedayed]

Hi @atscott - thank you for your review, and yes i think it’s a good idea to have the solution into two PR’s.
I’ve already started to work on it.

[aahmedayed]

@atscott - i creat a new feature PR #38890 for the go/goTo methods

[aahmedayed]

Hi @atscott - I’ve already finished the adjustments.

[atscott]

This is looking better :) I've added another round of comments.

I think you may have missed one of my comments about a request to add some more testing around the history in some scenarios:

• An error during navigation (this was why we moved some logic to finalize)
• a route being cancelled by a canLoad guard
• a route being cancelled by a resolver returning EMPTY

[atscott]

Adding the breaking change flag since this would break tests that do assertions on the state object.

[aahmedayed]

You're the most welcome @AndrewKushnir and thanks a lot @atscott

[atscott]

Hi @aahmedayed, I had a thought tonight about this and realized we missed a pretty important scenario. It's very common for guards to schedule a new navigation before returning false. This could be a bit of an issue since history.go causes a popstate event which will schedule another navigation that will compete with the one triggered by the guard. What we might want to do in the finalize/cancelNavigationTransition is to treat this scenario (!completed && !errored) more like what's done when a guard returns a UrlTree. This logic is here and if you look through it, you'll see that it's treated as a redirect and resetStateAndUrl is not called.

A specific scenario where this doesn't work:

1. urlUpdateStrategy: 'eager'
2. navigate to /a
3. navigate to /b
4. click back to go to /a, which has a canActivate guard that navigates to /
   ===> expectation: end on / but we end on /b instead because of the history.go call that triggers a navigation.

There might be other scenarios I haven't thought of. I'll be on vacation until July. If you get a chance to take a look and send a PR before then, I can review it when I'm back. Otherwise I'll see about investigating other scenarios, making some adjustments to the code, and then start thinking about making the option public opt-in.

[atscott]

@aahmedayed I've been working with this more today and I'm a little concerned that this won't work in the end. We can always figure out how to handle things when navigations go through the back/forward buttons or through router.navigate. However, navigations from outside of that can't be handled well (Location.go, any regular href on the page, browser navigation with the navbar, AngularJS/Angular hybrid apps where navigations come from AngularJS). All of these situations could result in something worse than what's already in place because these won't have the correct routerPageId and the computation for history.go will be incorrect.

Edit: Playing with this in a real application, it seems like browsers retain information about the origin of the navigation. This means that if I am going between pages that cross a manual navigation boundary, it treats that as a non-SPA navigation so the Router won't try to compute the history.go delta. I'll keep testing things out, but this is a bit more promising.

[aahmedayed]

HI @atscott - Sorry for the delay I’m taking some days off, I’ll be back Monday. Thank you for all the fixes you made.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}