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

fix(router): better ngZone checking for warning #25839

Closed
wants to merge 1 commit into from

Conversation

trotyl
Copy link
Contributor

@trotyl trotyl commented Sep 6, 2018

Closes #25837

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] 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: #25837

NgZone.isInAngularZone() results to false in testing, and cause warning.

What is the new behavior?

Checking NgZone.isInAngularZone() when constructing Router, considering NgZone not enabled when result being false.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@atscott
Copy link
Contributor

atscott commented Aug 5, 2020

@JiaLiPassion Can you review this PR?

@JiaLiPassion
Copy link
Contributor

@atscott , sure, I will check this one.

Do not warn that navigation was triggered outside Angular zone if the
Router was created outside Angular zone in the first place.

Closes angular#25837
@pullapprove pullapprove bot removed the area: zones label Dec 29, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 29, 2020
@atscott atscott added the target: patch This PR is targeted for the next patch release label Dec 29, 2020
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

LGTM. Adding my approval and marking as ready for merge, pending presubmit results.

@JiaLiPassion - please let me know if you notice anything particularly alarming about this change. It seems reasonable to me and at this point I'm in favor of the "merge and see what happens" strategy if we can't make a definitive decision one way or another; this is only a dev-mode warning that wouldn't affect production applications but does appear to cause a lot of friction in tests as it is.

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Jan 8, 2021
@atscott atscott removed the request for review from JiaLiPassion January 8, 2021 22:29
atscott pushed a commit that referenced this pull request Jan 11, 2021
Do not warn that navigation was triggered outside Angular zone if the
Router was created outside Angular zone in the first place.

Closes #25837

PR Close #25839
@atscott atscott closed this in 7695468 Jan 11, 2021
@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 Feb 11, 2021
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 cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Navigation triggered outside Angular zone" warning in unit tests
5 participants