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

Toxicable · 2017-11-02T22:20:14Z

Remove prolyfill from error msg to avoid future issues such as
#20120 #12119 #19677 #18581 #11839 #20182

vicb · 2017-11-03T17:55:48Z

packages/core/src/zone/ng_zone.ts

@@ -118,6 +118,8 @@ export class NgZone {

  constructor({enableLongStackTrace = false}) {
    if (typeof Zone == 'undefined') {
+      // prolyfill = probably a polyfill ie you can not polyfill something that is not a standard
+      // yet
      throw new Error('Angular requires Zone.js prolyfill.');


I have just discussed with @mhevery, the best would be to remove "prolyfill" entirely here

lazarljubenovic · 2017-11-05T09:45:57Z

packages/core/src/zone/ng_zone.ts

@@ -118,7 +118,7 @@ export class NgZone {

  constructor({enableLongStackTrace = false}) {
    if (typeof Zone == 'undefined') {
-      throw new Error('Angular requires Zone.js prolyfill.');
+      throw new Error('Angular requires Zone.js');


Now it's missing the period at the end ;P

I wonder if it is worth updating the message to mention the noop option.
I.e. Angular does not require Zone.js any more so this error might be a result of someone (incorrectly) trying to use the ngZone: noop option.

I thought of this. However, the only situation this error can happen is when we aren't in the nooped zone right?

Yes, we will be in non-nooped zone without the Zone.js polyfill. The question is why did we end up in this situation? 😃

It could be either because we want to use Zone.js, but forgot to load the polyfill.
Or it could be that we want to not use Zone.js, but didn't know how (or forgot) to properly configure ngZone: 'noop'.

Ahhh right I see you point about the second part.
maybe something like

Angular requires Zone.js, unless you are using ngZone: 'noop'

I think that should be a reasonable prompt on what to google to find out more.
Would be nice to include things like links to docs but I don't think we have docs on this just yet and it would bloat bundle size more than we have to

SGTM (fwiw 😃)

vicb · 2017-11-09T20:31:17Z

packages/core/src/zone/ng_zone.ts

@@ -118,7 +118,7 @@ export class NgZone {

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


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.

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.

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.

vicb

see inline comments

vicb · 2017-11-10T04:15:47Z

Please make sure Travis gets green (can not restart the job right through the UI but I guess it's a flake)

Toxicable · 2017-11-10T07:46:08Z

@vicb good to go now, looks like it was a flake due to timeouts, not sure of the cause

PR Close #20121

angular-automatic-lock-bot · 2019-09-12T20:52:22Z

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.}

googlebot added the cla: yes label Nov 2, 2017

Toxicable force-pushed the prolyfill branch 7 times, most recently from dba4ba8 to 0029de6 Compare November 3, 2017 02:54

gkalpak added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer type: docs target: patch This PR is targeted for the next patch release labels Nov 3, 2017

vicb suggested changes Nov 3, 2017

View reviewed changes

Toxicable force-pushed the prolyfill branch from 0029de6 to 0444c95 Compare November 3, 2017 18:00

Toxicable changed the title ~~refactor(core): explain what a prolyfill is~~ refactor(core): remove prolyfill from error message Nov 3, 2017

trotyl mentioned this pull request Nov 4, 2017

fix(core): updating ngZone error message #20182

Closed

3 tasks

lazarljubenovic reviewed Nov 5, 2017

View reviewed changes

Toxicable force-pushed the prolyfill branch 2 times, most recently from 5dc5937 to 98fd88b Compare November 6, 2017 10:06

vicb reviewed Nov 9, 2017

View reviewed changes

vicb suggested changes Nov 9, 2017

View reviewed changes

Toxicable force-pushed the prolyfill branch from 98fd88b to 6007fcb Compare November 10, 2017 00:30

vicb approved these changes Nov 10, 2017

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: merge The PR is ready for merge by the caretaker labels Nov 10, 2017

vicb added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Nov 10, 2017

refactor(core): remove prolyfill from error message

f403b28

Toxicable force-pushed the prolyfill branch from 6007fcb to f403b28 Compare November 10, 2017 07:13

jasonaden pushed a commit that referenced this pull request Nov 10, 2017

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

bd70aec

PR Close #20121

jasonaden closed this in b55c2ba Nov 10, 2017

trotyl mentioned this pull request Nov 11, 2017

docs(@schematics/angular): adjust statement of Zone.js prolyfill angular/devkit#250

Merged

trotyl mentioned this pull request Feb 16, 2018

style(polyfill): correct type on polyfill word #15647

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Toxicable commented Nov 2, 2017 •

edited

vicb Nov 3, 2017

Toxicable Nov 3, 2017

lazarljubenovic Nov 5, 2017

gkalpak Nov 6, 2017

Toxicable Nov 6, 2017

gkalpak Nov 6, 2017

Toxicable Nov 6, 2017 •

edited

gkalpak Nov 6, 2017

Toxicable Nov 6, 2017

vicb Nov 9, 2017

Toxicable Nov 9, 2017

Toxicable Nov 10, 2017

vicb left a comment

vicb commented Nov 10, 2017

Toxicable commented Nov 10, 2017 •

edited

angular-automatic-lock-bot bot commented Sep 12, 2019

		@@ -118,7 +118,7 @@ export class NgZone {

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

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

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

Conversation

Toxicable commented Nov 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable Nov 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

vicb commented Nov 10, 2017

Toxicable commented Nov 10, 2017 • edited

angular-automatic-lock-bot bot commented Sep 12, 2019

Toxicable commented Nov 2, 2017 •

edited

Toxicable Nov 6, 2017 •

edited

Toxicable commented Nov 10, 2017 •

edited