fix(zone.js): remove global declaration by JiaLiPassion · Pull Request #37861 · angular/angular

JiaLiPassion · 2020-06-30T22:05:47Z

Close #37531

Remove global declaration in zone.ts to avoid compile error when
upgrade to @types/node.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

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: 37531

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

mhevery · 2020-07-16T18:08:45Z

presubmit

mhevery · 2020-07-16T18:14:11Z

CARETAKER: Needs cl/321604430

mhevery · 2020-07-24T22:42:12Z

This is failing with

third_party/javascript/angular2/rc/packages/zone.js/lib/zone.ts:1437:86 - error TS2304: Cannot find name 'global'.

1437 })(typeof window !== 'undefined' && window || typeof self !== 'undefined' && self || global);

I think adding

(typeof window !== 'undefined' && window || typeof self !== 'undefined' && self || typeof global !== 'undefined' && global);

solves the problem.

JiaLiPassion · 2020-07-24T22:52:44Z

@mhevery , got it, thanks, I just updated the zone.ts.

mhevery · 2020-07-27T18:02:21Z

presubmit

mhevery · 2020-07-27T18:04:50Z

Argh! Seems like that did not help. :-(

third_party/javascript/angular2/rc/packages/zone.js/lib/zone.ts:1438:11 - error TS2304: Cannot find name 'global'.

1438    typeof global !== 'undefined' && global);
               ~~~~~~
third_party/javascript/angular2/rc/packages/zone.js/lib/zone.ts:1438:37 - error TS2304: Cannot find name 'global'.

1438    typeof global !== 'undefined' && global);
                                         ~~~~~~

It seems that we can't remove the global declaration from google3.

What is the motivation behind this change?

JiaLiPassion · 2020-07-27T23:28:20Z

@mhevery, The motivation is the issue #37531,

In DefinitelyTyped/DefinitelyTyped#44700, the type definition of the global variable was updated to NodeJS.Global & typeof globalThis. Which makes the Zone.js package not compatible with @types/node 14.0.5 or up.

I will update the global type definition to match the new nodejs definition.

mhevery · 2020-07-30T20:32:38Z

Hmm, it seems like the build system does not like that.

dag03tsc · 2020-08-15T23:00:40Z

The @types/node module loads before the zone.js one and it declares the global variable so its type can't be changed later.

Therefore, I think the solution could be, in fact, to exactly mirror the global declaration (Or maybe remove it instead?).

I've bypassed this since the bug appeared by applying the next patch after npm install.

--- node_modules/zone.js/dist/zone.js.d.ts
+++ node_modules/zone.js/dist/zone.js.d.ts
@@ -600,34 +600,54 @@
-declare var global: NodeJS.Global;
+declare var global: NodeJS.Global & typeof globalThis;

--- node_modules/zone.js/lib/zone.d.ts
+++ node_modules/zone.js/lib/zone.d.ts
@@ -600,34 +600,54 @@
-declare var global: NodeJS.Global;
+declare var global: NodeJS.Global & typeof globalThis;

It doesn't generate any side effects and everything seems alright so far.

I guess the global type should be declared as NodeJS.Global & typeof globalThis instead of any.

Maybe removing it could solve it too, just by letting the @types/node handle it all, but I'm not sure if it'd carry any unwanted side effects due to the missing declaration if, somehow, @types/node isn't available, so the first option seems safer.

ghost · 2020-08-16T02:14:38Z

packages/zone.js/lib/zone.ts

Oh! I see! I didn't knew about the suggestion feature. Thanks @kenaidri!

dag03tsc · 2020-08-16T03:22:58Z

packages/zone.js/lib/zone.ts

Suggested change

declare var global: any;

declare var global: NodeJS.Global & typeof globalThis;

Make sure that the global type matches the one provided by the current @types/node module to solve type conflicts.

dag03tsc · 2020-08-16T03:23:56Z

packages/zone.js/package.json

Suggested change

"@types/node": "^14.0.5",

"@types/node": "^14.0.27",

Upgrade @types/node to last version.

Yeah, we have to wait for node in angular repo update to 14.0.27+ and continue on this PR later.

I see. Nevermind!

dag03tsc · 2020-08-16T03:36:18Z

packages/zone.js/test/common/zone.spec.ts

Suggested change

(global as any).Promise = WrongPromise;

global.Promise = WrongPromise;

I'm not actually sure about this one.
Redeclaration with different type is a no go but that doesn't prevent recasting as far as I know.
How do you think it should be done?

Leave global uncasted as it was before.

Replace the any global cast with a (NodeJS.Global & typeof globalThis) one.

Don't change anything at all and leave the cast as any.

dag03tsc

Upgrade @types/node from v14.0.5 to v14.0.27.
Mirror the global declaration provided by the @types/node module (breaking change).

Edit: I apologize for sending 4 reviews in a row. It's been an error due to my lack of experience with the suggestion feature! I've learnt the lesson and next times I'll start a review instead of adding single comments!

JiaLiPassion · 2020-08-16T04:25:59Z

@dag03tsc , no problem, thank you for the review, nodejs update may not happen right now, will continue working on this PR after that.

josephperrott

LGTM

Reviewed-for: dev-infra

Close angular#37531 Remove `global` declaration in `zone.ts` to avoid compile error when upgrade to `@types/node` v12.12.68. Since the new type of global become `NodeJS.global & typeof globalThis` and not compatible with `zone.ts` declaration.

josephperrott · 2020-10-22T14:33:57Z

Caretaker Note: This will require cl/338344996 to land before it can be merged.

AndrewKushnir · 2020-10-22T23:06:03Z

@josephperrott @JiaLiPassion based on the "target: major" label and the current state of release train, this PR will be included into v12 (and will not land in upcoming v11). The target was set a couple months ago, so I believe the intention was to land it in v11, so should we change the label to "target: rc" (to target 11.0.x branch and master) to reflect that? Please confirm.

JiaLiPassion · 2020-10-23T00:15:29Z

@AndrewKushnir , yes, this one is targeting v11, I have updated the label to target: rc.

Close #37531 Remove `global` declaration in `zone.ts` to avoid compile error when upgrade to `@types/node` v12.12.68. Since the new type of global become `NodeJS.global & typeof globalThis` and not compatible with `zone.ts` declaration. PR Close #37861

angular-automatic-lock-bot · 2020-11-23T16:30:16Z

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 Jun 30, 2020

JiaLiPassion requested a review from mhevery June 30, 2020 22:06

JiaLiPassion added area: zones Issues related to zone.js target: major This PR is targeted for the next major release labels Jul 1, 2020

ngbot bot modified the milestone: needsTriage Jul 1, 2020

JiaLiPassion force-pushed the type-global branch from 65369a3 to eea4b57 Compare July 1, 2020 05:49

mhevery approved these changes Jul 15, 2020

View reviewed changes

mhevery added the action: merge The PR is ready for merge by the caretaker label Jul 24, 2020

JiaLiPassion force-pushed the type-global branch from eea4b57 to 3417b9f Compare July 24, 2020 22:52

JiaLiPassion added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 30, 2020

JiaLiPassion force-pushed the type-global branch 2 times, most recently from b104a80 to b26e108 Compare July 30, 2020 12:08

JiaLiPassion added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 30, 2020

mhevery approved these changes Jul 30, 2020

View reviewed changes

AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Aug 10, 2020

ghost approved these changes Aug 16, 2020

View reviewed changes

dag03tsc reviewed Aug 16, 2020

View reviewed changes

dag03tsc suggested changes Aug 16, 2020

View reviewed changes

JiaLiPassion force-pushed the type-global branch from b26e108 to 0b1b1ed Compare August 30, 2020 05:25

pullapprove bot requested a review from filipesilva August 30, 2020 05:25

pullapprove bot requested review from devversion and removed request for filipesilva September 17, 2020 17:51

pullapprove bot requested a review from josephperrott September 28, 2020 20:30

josephperrott approved these changes Sep 28, 2020

View reviewed changes

josephperrott removed the request for review from devversion September 28, 2020 21:05

fix(zone.js): remove global declaration

9211fbb

Close angular#37531 Remove `global` declaration in `zone.ts` to avoid compile error when upgrade to `@types/node` v12.12.68. Since the new type of global become `NodeJS.global & typeof globalThis` and not compatible with `zone.ts` declaration.

JiaLiPassion force-pushed the type-global branch from 0b1b1ed to 9211fbb Compare October 18, 2020 06:31

mhevery self-assigned this Oct 19, 2020

josephperrott added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 22, 2020

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 22, 2020

josephperrott removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 22, 2020

JiaLiPassion added target: rc This PR is targeted for the next release-candidate and removed target: major This PR is targeted for the next major release labels Oct 23, 2020

alxhub closed this in 90c0772 Oct 23, 2020

JiaLiPassion mentioned this pull request Oct 27, 2020

release: cut the zone.js-0.11.3 release #39440

Closed

sarunint mentioned this pull request Oct 27, 2020

feat: update AngularFire to be compatible with Firebase JS SDK v8 angular/angularfire#2620

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 23, 2020

	declare var global: any;
	declare var global: NodeJS.Global & typeof globalThis;

	(global as any).Promise = WrongPromise;
	global.Promise = WrongPromise;

Conversation

JiaLiPassion commented Jun 30, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

mhevery commented Jul 16, 2020

Uh oh!

mhevery commented Jul 16, 2020

Uh oh!

mhevery commented Jul 24, 2020

Uh oh!

JiaLiPassion commented Jul 24, 2020

Uh oh!

mhevery commented Jul 27, 2020

Uh oh!

mhevery commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Jul 27, 2020

Uh oh!

mhevery commented Jul 30, 2020

Uh oh!

dag03tsc commented Aug 15, 2020

Uh oh!

ghost Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

dag03tsc Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

dag03tsc Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dag03tsc Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

dag03tsc Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

dag03tsc Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dag03tsc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Aug 16, 2020

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

josephperrott commented Oct 22, 2020

Uh oh!

AndrewKushnir commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Oct 23, 2020

Uh oh!

angular-automatic-lock-bot bot commented Nov 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhevery commented Jul 27, 2020 •

edited

Loading

dag03tsc Aug 16, 2020 •

edited

Loading

dag03tsc Aug 16, 2020 •

edited

Loading

dag03tsc left a comment •

edited

Loading

AndrewKushnir commented Oct 22, 2020 •

edited

Loading