refactor(router): compute correct history restoration when navigation is cancelled by aahmedayed · Pull Request #38884 · angular/angular

aahmedayed · 2020-09-17T15:16:34Z

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 · 2020-09-17T17:00:21Z

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 · 2020-09-17T18:20:08Z

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

aahmedayed · 2020-09-23T19:56:58Z

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 · 2020-09-23T23:52:24Z

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

aahmedayed · 2021-05-05T07:28:44Z

Hi @atscott i made the requested changes.
Also, i created a new commit e3da2c6eb3 but i'm not sure if we should add it in a new Pr?

atscott · 2021-05-06T17:27:51Z

@aahmedayed Thanks, I'll take another look today. I think we should not include e3da2c6 because it would make this TODO even harder to resolve. It might be that it's something that doesn't make sense to change anymore but we should limit the changes in this PR to what's absolutely necessary to accomplish our goal here.

atscott

Another round of comments with one noticeable bug.

Sorry, I know this has been a ton of work for you but you're doing a really great job here! This change is incredibly complex with a lot of moving parts and edge cases to test.

atscott · 2021-05-06T17:53:35Z

I would prefer not adding ngRouterPageId here because it makes it more public than I would want. Accessing it is still possible even without it because of the {[k: string]: any}. I don't think navigationId should ever really have been included here because it makes the public API more confusing since "navigationId" wasn't meant to be public in this way.

Maybe this was missed in one of the squashes. Let's remove ngRouterPageId here so we don't change the public API.

atscott · 2021-05-06T18:35:14Z

Could you add the else case with a comment as to why it's a no-op?

Unresolving. Please add the else case and add a comment as to why nothing is done there (if update is not 'eager', then the navigation was cancelled before any browser url change so nothing needs to be restored).

aahmedayed · 2021-05-11T15:55:33Z

Hi @atscott, no problem at all, I’ll take care of it ASAP.

atscott · 2021-05-25T15:35:46Z

Hi @aahmedayed, have you gotten a chance to address the last round of feedback? I have some ideas that would allow us to merge this as a non-breaking change after you've pushed your latest updates.

aahmedayed · 2021-05-25T16:44:42Z

Hi @atscott, I'll try to finish with the requested changes this week at the latest.

aahmedayed · 2021-06-01T16:10:32Z

Hi @atscott, I'm done with the requested changes, but i think we can't do this #38884 (comment) change now because the test_ivy_aot failed and the error is that cannot find name RestoredState in router.d.ts.

atscott

@aahmedayed thanks for the updates! I'll take a look at the changes this week. No problem on the @internal comment.

My plan for landing this change is:

Submit the change first without a public API option so we can continue to test and offer the solution to the interested parties on the bug as a private preview (the option in the Router will be set to @internal but users can opt-in with (Router as any).canceledNavigationResolution = 'computed'). We can consider a follow-up change to add it as public in a minor version.
Move the changes to location_mock into a local copy used by the router tests. This avoids making this a breaking change and we can remove it when #41730 is submitted.
clean up the commits: squash most/all of them into a single commit and update the commit message(s)
Move the commit that updated the navigationId and pageId to be in the ng namespace to a follow-up PR (again to avoid making this a breaking change). I like this change but it would prevent us from submitting this until v13.

Sorry for requesting reverts to changes I had you do; we should get them merged eventually, but getting this PR submitted sooner rather than later (in v13) seems like a good idea. Do you mind if I make those changes and push up to your branch as a co-author or would you like to do the updates yourself?

aahmedayed · 2021-06-01T20:12:08Z

Ok @atscott there is no probleme, I start making those changes right now. But I'm not sure I understand the second point correctly. Can you please provide me with more details ? Thank you.

atscott · 2021-06-01T20:18:59Z

@aahmedayed - Absolutely. There are changes here that depend on your updates to the back/forward methods in SpyLocation. Those changes will be considered a breaking change and can't be submitted until v13. To get around that, we can copy the whole implementation of SpyLocation to a class in the router's integration.spec.ts and make the update there (and also add {provide: Location, useClass: RouterSpyLocation} to the providers in the beforeEach). That way we can submit this change now and then later update the router test to point to the real SpyLocation in v13 after #38884 is submitted.

aahmedayed · 2021-06-01T20:25:42Z

Ah ok! I understand, thank you @atscott

aahmedayed · 2021-06-02T15:23:54Z

Hi @atscott - I'm done with the requested changes.

atscott

Looking good. A few more minor comments. Also, can you amend the commit message to be:

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

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.

We should change the scope of this commit to be a refactor since we haven't provided a public API for the fix (so nothing was actually fixed yet). We should also remove the fix # comment since, again, we haven't made the fix available.

atscott · 2021-06-03T17:09:25Z

Maybe this was missed in one of the squashes. Let's remove ngRouterPageId here so we don't change the public API.

aahmedayed · 2021-06-04T22:11:49Z

Hi @atscott - I'm done with the requested changes.

atscott

LGTM with two minor comments on the state restoration function.

reviewed-for: public-api
reviewed-for: size-tracking

atscott · 2021-06-07T17:05:01Z

Unresolving. Please add the else case and add a comment as to why nothing is done there (if update is not 'eager', then the navigation was cancelled before any browser url change so nothing needs to be restored).

AndrewKushnir · 2021-06-07T17:13:25Z

Presubmit.

jessicajaniuk

LGTM 🍪
reviewed-for: size-tracking

aahmedayed · 2021-06-09T22:25:03Z

Hi guys, i have finished the changes that have been asked of me, but i haven’t had the chance to work on the docs since i’m having a very busy week.
@atscott if you don’t mind, could you please take it from here to fix the documentations so that i don’t cause a delay in the workflow. Thank you in advance.

… is cancelled 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. implementation for angular#13586

AndrewKushnir

@aahmedayed thanks for addressing the feedback, the changes look good 👍

atscott · 2021-06-09T23:06:42Z

another presubmit for a sanity check

aahmedayed · 2021-06-09T23:55:29Z

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

atscott · 2021-06-18T05:28:40Z

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:

urlUpdateStrategy: 'eager'
navigate to /a
navigate to /b
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 · 2021-07-02T19:35:40Z

@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 · 2021-07-08T19:51:57Z

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 · 2021-08-08T16:06:06Z

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.}

googlebot added the cla: yes label Sep 17, 2020

pullapprove Bot requested review from atscott, kyliau and pkozlowski-opensource September 17, 2020 15:16

aahmedayed force-pushed the fix-deactivation-guard-breaks-routing-history branch from e2b10a5 to add1b74 Compare September 17, 2020 15:49

atscott requested changes Sep 17, 2020

View reviewed changes

Comment thread packages/common/src/location/location.ts Outdated

Comment thread packages/common/testing/src/location_mock.ts Outdated

Comment thread packages/common/src/location/location_strategy.ts Outdated

Comment thread packages/common/src/location/platform_location.ts Outdated

atscott requested changes Sep 21, 2020

View reviewed changes

Comment thread packages/router/src/router.ts Outdated

Comment thread packages/router/src/router.ts Outdated

aahmedayed force-pushed the fix-deactivation-guard-breaks-routing-history branch 3 times, most recently from 6e7416a to 894f5d0 Compare September 23, 2020 15:30

aahmedayed requested a review from atscott September 23, 2020 17:57

atscott requested changes Sep 23, 2020

View reviewed changes

atscott added area: router breaking changes labels Sep 23, 2020

ngbot Bot modified the milestone: needsTriage Sep 23, 2020

atscott added the target: major This PR is targeted for the next major release label Sep 23, 2020

atscott requested changes Sep 24, 2020

View reviewed changes

Comment thread packages/router/src/router.ts Outdated

atscott reviewed Sep 24, 2020

View reviewed changes

Comment thread packages/router/src/router.ts Outdated

aahmedayed force-pushed the fix-deactivation-guard-breaks-routing-history branch 7 times, most recently from 4a1fb91 to 524762e Compare September 27, 2020 09:59

atscott requested changes May 6, 2021

View reviewed changes

atscott requested changes Jun 1, 2021

View reviewed changes

Comment thread goldens/public-api/router/router.d.ts Outdated

atscott requested changes Jun 3, 2021

View reviewed changes

atscott approved these changes Jun 7, 2021

View reviewed changes

jessicajaniuk approved these changes Jun 7, 2021

View reviewed changes

AndrewKushnir reviewed Jun 7, 2021

View reviewed changes

AndrewKushnir approved these changes Jun 9, 2021

View reviewed changes

atscott mentioned this pull request Jul 1, 2021

Deactivation Guard breaks the routing history #13586

Closed

atscott mentioned this pull request Jul 2, 2021

refactor(router): Adjust behavior for computed navigation restoration #42751

Closed

Conversation

aahmedayed commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aahmedayed commented Sep 17, 2020

Uh oh!

aahmedayed commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aahmedayed commented Sep 23, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atscott commented Sep 23, 2020

Uh oh!

Uh oh!

Uh oh!

aahmedayed commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented May 6, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atscott May 6, 2021

Choose a reason for hiding this comment

Uh oh!

atscott Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atscott May 6, 2021

Choose a reason for hiding this comment

Uh oh!

atscott Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aahmedayed commented May 11, 2021

Uh oh!

atscott commented May 25, 2021

Uh oh!

aahmedayed commented May 25, 2021

Uh oh!

aahmedayed commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aahmedayed commented Sep 17, 2020 •

edited

Loading

aahmedayed commented Sep 17, 2020 •

edited

Loading

aahmedayed commented May 5, 2021 •

edited

Loading

aahmedayed commented Jun 1, 2021 •

edited

Loading

atscott commented Jul 2, 2021 •

edited

Loading