fix(router): ensure duplicate popstate/hashchange events are handled … by atscott · Pull Request #37674 · angular/angular

atscott · 2020-06-22T17:27:07Z

…correctly

The current method of handling duplicate navigations caused by 'hashchange' and 'popstate' events for the same url change does not correctly handle cancelled navigations. Because scheduleNavigation is called in a setTimeout in the location change subscription, the duplicate navigations are not flushed at the same time. This means that if the initial navigation hits a guard that schedules a new navigation, the navigation for the duplicate event will not compare to the correct transition (because we inserted another navigation between the duplicates). See #16710 (comment)

Fixes #16710

Change summary:

Move the 'hashchange' / 'popstate' duplicate nav checks from scheduleNavigation to the location subscription. This is where the events are initially handled / detected
Use pairwise to compare with the previously detected 'hashchange' / 'popstate'
Save the current router transition id at the time of the location event to aid in comparison

atscott · 2020-06-22T18:24:15Z

presubmit

atscott · 2020-06-23T15:00:15Z

global presubmit

jelbourn

Is this a do-over of the previous router change that was rolled back? If so, I would reference that original change and its revert in the commit message for reference.

atscott · 2020-06-23T18:58:06Z

Is this a do-over of the previous router change that was rolled back? If so, I would reference that original change and its revert in the commit message for reference.

This change is unrelated to the one that was rolled back.

jelbourn

LGTM

jelbourn

LGTM

Reviewed-for: size-tracking

JoostK

This is generally a great cleanup! I left some suggestions in the code. Otherwise LGTM.

IgorMinar

Could we fix this bug without the ~800b penalty? @JoostK's suggestion looks reasonable to me. I know that it sucks, but 800b is quite a bit especially if it can be avoided.

Reviewed-for: size-tracking

atscott · 2020-06-29T16:56:38Z

updated presubmit
May want to do another global as well

edit: global presubmit

pkozlowski-opensource

LGTM

Reviewed-for: size-tracking

pkozlowski-opensource

LGTM

Reviewed-for: size-tracking

…correctly The current method of handling duplicate navigations caused by 'hashchange' and 'popstate' events for the same url change does not correctly handle cancelled navigations. Because `scheduleNavigation` is called in a `setTimeout` in the location change subscription, the duplicate navigations are not flushed at the same time. This means that if the initial navigation hits a guard that schedules a new navigation, the navigation for the duplicate event will not compare to the correct transition (because we inserted another navigation between the duplicates). See angular#16710 (comment) Fixes angular#16710

alxhub

Reviewed-for: size-tracking

Igor is OOO and can't re-review. His request has been addressed (changes no longer introduce new rxjs operators)

angular-automatic-lock-bot · 2020-08-13T16:17:00Z

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

…correctly (angular#37674) The current method of handling duplicate navigations caused by 'hashchange' and 'popstate' events for the same url change does not correctly handle cancelled navigations. Because `scheduleNavigation` is called in a `setTimeout` in the location change subscription, the duplicate navigations are not flushed at the same time. This means that if the initial navigation hits a guard that schedules a new navigation, the navigation for the duplicate event will not compare to the correct transition (because we inserted another navigation between the duplicates). See angular#16710 (comment) Fixes angular#16710 PR Close angular#37674

atscott added area: router target: major This PR is targeted for the next major release risk: medium router: LocationStrategy router: navigation pipe events/scheduleNavigation/transitions observable labels Jun 22, 2020

ngbot Bot modified the milestone: needsTriage Jun 22, 2020

googlebot added the cla: yes label Jun 22, 2020

atscott force-pushed the hashchange branch 2 times, most recently from a0371aa to 9b09f4d Compare June 22, 2020 18:22

atscott force-pushed the hashchange branch from 9b09f4d to e5d1ac3 Compare June 22, 2020 19:10

atscott marked this pull request as ready for review June 23, 2020 16:41

pullapprove Bot requested review from IgorMinar, alxhub, jelbourn and pkozlowski-opensource June 23, 2020 16:41

jelbourn reviewed Jun 23, 2020

View reviewed changes

pullapprove Bot requested a review from jelbourn June 23, 2020 18:54

atscott force-pushed the hashchange branch from 41b781b to e721478 Compare June 23, 2020 20:57

jelbourn approved these changes Jun 23, 2020

View reviewed changes

pullapprove Bot requested a review from jelbourn June 23, 2020 22:21

jelbourn approved these changes Jun 23, 2020

View reviewed changes

atscott requested a review from JoostK June 25, 2020 16:57

JoostK approved these changes Jun 25, 2020

View reviewed changes

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

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

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

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

JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 25, 2020

IgorMinar previously requested changes Jun 25, 2020

View reviewed changes

atscott force-pushed the hashchange branch from e721478 to 2189e41 Compare June 29, 2020 16:39

atscott force-pushed the hashchange branch 5 times, most recently from 79a7fb9 to 3e25e46 Compare June 29, 2020 16:49

atscott added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 29, 2020

atscott force-pushed the hashchange branch from 3e25e46 to 2f29dc0 Compare June 29, 2020 17:13

pkozlowski-opensource approved these changes Jun 30, 2020

View reviewed changes

atscott requested review from IgorMinar and pkozlowski-opensource June 30, 2020 18:06

pkozlowski-opensource approved these changes Jul 1, 2020

View reviewed changes

atscott force-pushed the hashchange branch 2 times, most recently from b3138ce to 52aaf45 Compare July 7, 2020 21:57

atscott force-pushed the hashchange branch from 52aaf45 to b0e5760 Compare July 7, 2020 22:34

alxhub approved these changes Jul 9, 2020

View reviewed changes

atscott removed the request for review from IgorMinar July 10, 2020 16:22

atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 13, 2020

atscott closed this in 9185c6e Jul 13, 2020

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Aug 13, 2020

Conversation

atscott commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jun 22, 2020

Uh oh!

atscott commented Jun 23, 2020

Uh oh!

jelbourn 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!

atscott commented Jun 23, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IgorMinar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atscott commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

atscott commented Jun 22, 2020 •

edited

Loading

IgorMinar left a comment •

edited

Loading

atscott commented Jun 29, 2020 •

edited

Loading