X Tutup
The Wayback Machine - https://web.archive.org/web/20220610043157/https://github.com/angular/angular/pull/45735
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

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 Apr 23, 2022
@atscott atscott added breaking changes target: major flag: breaking change labels Apr 23, 2022
@atscott
Copy link
Contributor Author

@atscott 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 removed this from the Backlog milestone Apr 25, 2022
@atscott atscott added this to the v14-candidates milestone Apr 25, 2022
@atscott atscott requested a review from AndrewKushnir Apr 25, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

This is awesome! 🎉 Thanks @atscott!

@atscott atscott added the action: merge label Apr 27, 2022
@ngbot
Copy link

@ngbot 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 atscott commented Apr 27, 2022

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

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented May 28, 2022

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 breaking changes comp: router flag: breaking change target: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants
X Tutup