Skip to content

refactor(router): Remove special logic for hybrid apps - #45240

Closed
atscott wants to merge 1 commit into
angular:masterfrom
atscott:speciallogic
Closed

atscott wants to merge 1 commit into
angular:masterfrom
atscott:speciallogic

Conversation

@atscott

@atscott atscott commented Mar 2, 2022 •

Copy link
Copy Markdown
Contributor

This commit removes special (undocumented) logic in the Router code that is
meant to prevent duplicate navigations that result from location syncs in
AngularJS/Angular hybrid applications.

The duplicate navigations can occur when both the Router and the AngularJS sync
code detect a location change via a popstate/hashchange event. When this
happens, the Angular Router schedules a navigation to sync itself with
the browser, but the hybrid listener may also schedule an additional
navigation. There are a few reasons this logic should not be included in
the Router:

  • This special logic is not tree shakeable so it introduces a bundle
    size cost for all applications, most of which don't need it.
  • There have been many updates to the routing pipeline to tolerate
    duplicate navigations. That is, duplicate navigations can happen and
    routing should still complete successfully.
  • The logic is really in the wrong place: The hybrid sync code should be
    the location to handle this. If duplicate navigations are meant to be
    avoided, the hybrid sync code should have handling to not trigger
    duplicate navs.
  • This logic also used to exist because the mock location
    helper used for test incorrectly triggered popstate events during
    router navigations. In order to avoid unexpected behavior in tests, this
    logic needed to be added. This incorrect mocking may also have been
    put in place because the upgrade module would see a location change
    event and trigger a duplicate navigation. The location mock has since been updated to
    match real browser behavior so this is no longer necessary. The
    upgrade module has also been updated to not trigger duplicate
    navigations. The following commits are related to this:

Side note: The setTimeout in the location change listener is used to
ensure the ordering of duplicate navigations was consistent. You can see
that the logic being removed here expects the imperative navigation to precede the
popstate/hashchange. With the removal of this code, the setTimeout no
longer serves a purpose. However, it has been found that tests can rely
on this behavior (incorrectly) because they expect the navigation to be
complete but in reality, it hasn't even started because the test has not
flushed the timeout. Removing the timeout would be a breaking change as
a result.

@ngbot ngbot Bot added this to the Backlog milestone Mar 2, 2022
@atscott

atscott commented Mar 2, 2022

Copy link
Copy Markdown
Contributor Author

green TGP

This commit removes special (undocumented) logic in the Router code that is
meant to prevent duplicate navigations that result from location syncs in
AngularJS/Angular hybrid applications.

The duplicate navigations can occur when both the Router and the AngularJS sync
code detect a location change via a popstate/hashchange event. When this
happens, the Angular Router schedules a navigation to sync itself with
the browser, but the hybrid listener may also schedule an additional
navigation. There are a few reasons this logic should not be included in
the Router:

* This special logic is not tree shakeable so it introduces a bundle
  size cost for all applications, most of which don't need it.
* There have been many updates to the routing pipeline to tolerate
  duplicate navigations. That is, duplicate navigations can happen and
  routing should still complete successfully.
    * angular@0e8548f
    * angular@9e039ca
* The logic is really in the wrong place: The hybrid sync code should be
  the location to handle this. If duplicate navigations are meant to be
  avoided, the hybrid sync code should have handling to _not_ trigger
  duplicate navs.
* This logic _also_ used to exist because the mock location
  helper used for test incorrectly triggered popstate events during
  router navigations. In order to avoid unexpected behavior in tests, this
  logic needed to be added. This incorrect mocking may also have been
  put in place because the upgrade module _would_ see a location change
  event and trigger a duplicate navigation.  The location mock has since been updated to
  match real browser behavior so this is no longer necessary. The
  upgrade module has also been updated to not trigger duplicate
  navigations.  The following commits are related to this:
    * angular@202a1a5
    * angular@c6a9300

Side note: The `setTimeout` in the location change listener is used to
ensure the ordering of duplicate navigations was consistent. You can see
that the logic being removed here expects the imperative navigation to precede the
popstate/hashchange. With the removal of this code, the `setTimeout` no
longer serves a purpose. However, it has been found that tests can rely
on this behavior (incorrectly) because they expect the navigation to be
complete but in reality, it hasn't even started because the test has not
flushed the timeout. Removing the timeout would be a breaking change as
a result.
@atscott
atscott marked this pull request as ready for review March 2, 2022 18:47
@atscott atscott added the target: patch This PR is targeted for the next patch release label Mar 2, 2022
@atscott
atscott requested a review from AndrewKushnir March 2, 2022 18:48

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great cleanup, thanks @atscott 馃憤

@atscott atscott added action: merge The PR is ready for merge by the caretaker risk: medium merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 2, 2022
@atscott

atscott commented Mar 2, 2022

Copy link
Copy Markdown
Contributor Author

caretaker note: Please merge and sync on its own.

@AndrewKushnir

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit 5883e05.

AndrewKushnir pushed a commit that referenced this pull request Mar 3, 2022
This commit removes special (undocumented) logic in the Router code that is
meant to prevent duplicate navigations that result from location syncs in
AngularJS/Angular hybrid applications.

The duplicate navigations can occur when both the Router and the AngularJS sync
code detect a location change via a popstate/hashchange event. When this
happens, the Angular Router schedules a navigation to sync itself with
the browser, but the hybrid listener may also schedule an additional
navigation. There are a few reasons this logic should not be included in
the Router:

* This special logic is not tree shakeable so it introduces a bundle
  size cost for all applications, most of which don't need it.
* There have been many updates to the routing pipeline to tolerate
  duplicate navigations. That is, duplicate navigations can happen and
  routing should still complete successfully.
    * 0e8548f
    * 9e039ca
* The logic is really in the wrong place: The hybrid sync code should be
  the location to handle this. If duplicate navigations are meant to be
  avoided, the hybrid sync code should have handling to _not_ trigger
  duplicate navs.
* This logic _also_ used to exist because the mock location
  helper used for test incorrectly triggered popstate events during
  router navigations. In order to avoid unexpected behavior in tests, this
  logic needed to be added. This incorrect mocking may also have been
  put in place because the upgrade module _would_ see a location change
  event and trigger a duplicate navigation.  The location mock has since been updated to
  match real browser behavior so this is no longer necessary. The
  upgrade module has also been updated to not trigger duplicate
  navigations.  The following commits are related to this:
    * 202a1a5
    * c6a9300

Side note: The `setTimeout` in the location change listener is used to
ensure the ordering of duplicate navigations was consistent. You can see
that the logic being removed here expects the imperative navigation to precede the
popstate/hashchange. With the removal of this code, the `setTimeout` no
longer serves a purpose. However, it has been found that tests can rely
on this behavior (incorrectly) because they expect the navigation to be
complete but in reality, it hasn't even started because the test has not
flushed the timeout. Removing the timeout would be a breaking change as
a result.

PR Close #45240
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: router merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants