Skip to content

refactor(router): Remove unnecessary setTimeout in UrlTree redirects#45735

Closed
atscott wants to merge 1 commit intoangular:masterfrom
atscott:removeNotNeededRedirectTimeout
Closed

refactor(router): Remove unnecessary setTimeout in UrlTree redirects#45735
atscott wants to merge 1 commit intoangular:masterfrom
atscott:removeNotNeededRedirectTimeout

Conversation

@atscott
Copy link
Contributor

@atscott atscott commented Apr 23, 2022

Using setTimeout in the Router navigation pipeline creates fragile and
unpredictable behavior. Additionally, it creates a new macrotask, which
would trigger an unnecessary change detection in the application.

This setTimeout was added in
15e3978.
Both tests added in that commit still pass. Additionally, the comment
for why the setTimeout was added doesn't really explain how the
described bug would occur. There has been a lot of work in the Router
since then to stabalize edge case scenarios so it's possible it existed
before but doesn't anymore.

Removing this setTimeout revealed tests that
relied on the navigation not completing. For example, the test suite did
not have a route which matched the redirect, but the test passed because
it ended before the redirect was flushed, so the Router never threw an
error. Similar situations exist for the other use of setTimeout in the Route
(the one in the location change listener).
There were no other failures in TGP other than incorrectly written
tests.

BREAKING CHANGE:
When a guard returns a UrlTree, the router would previously schedule
the redirect navigation within a setTimeout. This timeout is now removed,
which can result in test failures due to incorrectly written tests.
Tests which perform navigations should ensure that all timeouts are
flushed before making assertions. Tests should ensure they are capable
of handling all redirects from the original navigation.

@ngbot ngbot bot added this to the Backlog milestone Apr 23, 2022
Using `setTimeout` in the Router navigation pipeline creates fragile and
unpredictable behavior. Additionally, it creates a new macrotask, which
would trigger an unnecessary change detection in the application.

This `setTimeout` was added in
angular@15e3978.
Both tests added in that commit still pass. Additionally, the comment
for _why_ the `setTimeout` was added doesn't really explain how the
described bug would occur. There has been a lot of work in the Router
since then to stabalize edge case scenarios so it's possible it existed
before but doesn't anymore.

Removing this `setTimeout` revealed tests that
relied on the navigation not completing. For example, the test suite did
not have a route which matched the redirect, but the test passed because
it ended before the redirect was flushed, so the `Router` never threw an
error. Similar situations exist for the other use of `setTimeout` in the Route
(the one in the location change listener).
There were no other failures in TGP other than incorrectly written
tests.

BREAKING CHANGE:
When a guard returns a `UrlTree`, the router would previously schedule
the redirect navigation within a `setTimeout`. This timeout is now removed,
which can result in test failures due to incorrectly written tests.
Tests which perform navigations should ensure that all timeouts are
flushed before making assertions. Tests should ensure they are capable
of handling all redirects from the original navigation.
@atscott atscott force-pushed the removeNotNeededRedirectTimeout branch from 0e06921 to b7756cb Compare April 23, 2022 14:47
@atscott atscott added breaking changes target: major This PR is targeted for the next major release flag: breaking change labels Apr 23, 2022
@atscott
Copy link
Contributor Author

atscott commented Apr 23, 2022

TGP
As mentioned in the commit message, the 3 failures in TGP are actually tests which are written incorrectly. They rely on unresolved promises that would fail the test if flushed.

@atscott atscott modified the milestones: Backlog, v14-candidates Apr 25, 2022
@atscott atscott requested a review from AndrewKushnir April 25, 2022 18:07
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

This is awesome! 🎉 Thanks @atscott!

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Apr 27, 2022
@ngbot
Copy link

ngbot bot commented Apr 27, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@atscott
Copy link
Contributor Author

atscott commented Apr 27, 2022

This PR was merged into the repository by commit 7b367d9.

@angular-automatic-lock-bot
Copy link

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 May 28, 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 breaking changes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants