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.

Sign up for GitHub

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

Jump to bottom

refactor(core): remove prolyfill from error message #20121

Closed
wants to merge 1 commit into from
Closed

refactor(core): remove prolyfill from error message #20121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/zone/ng_zone.ts
View file
Open in desktop
Expand Up @@ -118,7 +118,7 @@ export class NgZone {

constructor({enableLongStackTrace = false}) {
if (typeof Zone == 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to reads "unless you are using ngZone: 'noop'" to figure out what you mean...
I think I got it now but does "unless you are using ngZone: 'noop'" make any sense ?

  • it is not explicit enough IMO,
  • we would not throw in this case anyway.

So what about throw new Error('In this configuration Angular requires Zone.js'); or something along the line ? If you feel like we need more details then we should add something to angular.io and a short link here.

Copy link
Author

Choose a reason for hiding this comment

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

The reason why I chose "unless" was becasue of the situation @gkalpak mentioned above

  • You want to not use zones so you removed zone.js but you forgot to configure { ngZone: 'noop'}

My idea with being slightly vague here "unless you are using ngZone: 'noop'" is that devs who don't know what this means would be more likely to disregard it, since this will not apply to the vast majority of situations.

In saying that I do think we need some docs around this area.

Copy link
Author

Choose a reason for hiding this comment

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

After discussing it a little more I now think that @vicb 's solution is preferable. Since it is very unlikely for the top situation to occur.

throw new Error('Angular requires Zone.js prolyfill.');
throw new Error(`In this configuration Angular requires Zone.js`);
}

Zone.assertZonePatched();
Expand Down