src: add `globalThis` #22835

ljharb · 2018-09-13T05:56:39Z

Per https://github.com/tc39/proposal-global, the global value globalThis should be configurable, writable, and non-enumerable, and be the global object.

I assume this PR is semver-minor.

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This PR replaces #10405.

targos · 2018-09-13T06:41:18Z

I think it needs to be defined in https://github.com/nodejs/node/blob/master/lib/internal/per_context.js so that all V8 contexts have it, even the ones created by the vm module.

mscdex · 2018-09-13T06:44:32Z

Isn't this a bit premature? There doesn't even seem to be any real (recent) activity on this from any other browser vendor except Chrome/V8?

ljharb · 2018-09-13T06:59:35Z

@targos i ended up reverting it and directly backporting the v8 commits.

@mscdex i don't see why; this time we gathered web compat metrics beforehand.

devsnek · 2018-09-13T07:21:46Z

I was hoping this could just be backported from V8 upstream but it looks won't build...

@targos globalThis is flagged in V8 70 and unflagged in 71, we could push out an alias in per_context but i worry it would be removed before it was used. Do you have any insight there?

Per https://github.com/tc39/proposal-global, the global value `globalThis` should be configurable, writable, and non-enumerable.

tniessen · 2018-09-13T09:39:51Z

From the repository you linked:

it is on hold pending a new global identifier name

and

We are currently exploring identifier naming alternatives.

Is this not true anymore?

ljharb · 2018-09-13T15:14:36Z

No, neither are true anymore; i just missed a spot when updating it.

jasnell · 2018-09-13T15:30:10Z

lib/internal/per_context.js

+    value: global,
+    writable: true,
+    enumerable: false,
+    configurable: true,


would it be worthwhile emitting an experimental warning while this is still in early stages?

I don’t see why; it’s not in early stages, it’s in stage 3, and v8 is shipping it unflagged.

jdalton · 2018-09-13T19:41:29Z

If this is coming to V8 does Node have to do anything?

ljharb · 2018-09-13T19:54:27Z

@jdalton nope, but if node wants to add it sooner than they update to v8 71 (or whichever version it's in), then this PR would be helpful for that - and i'd really like to see this global backported to v6 and v8.

Trott

Test LGTM. No opinion on the implementation or whether it makes sense to wait for V8 71.

(I think that's our first use of Function() in tests!)

Trott

Oops, I spoke too soon. Can we get rid of the string literals as the message values for the calls to assert.strictEqual(). (I have a lint rule ready to go to enforce this. PR coming probably later today or early tomorrow. There's a few things I want to get Just Right.)

Options include:

Just use the first two arguments to assert.strictEqual() which will use default message (which will show the values of the first two arguments when there is an AssertionError). Optionally, change your third argument to a comment above the assert.strictEqual() call.
Make the third argument a template string that will show the actual value along with your text.
If you feel 100% confident that configurable, enumerable, and writable will be booleans and will never be something else, switch to assert.ok(actual, message).

jdalton

It looks like V8 v7 is coming ~Oct 14 #22754 (which would be flagged support) and then unflagged the update after. I'm fine waiting ~month for this as experimental and the follow up as unflagged. There's no rush here.

ljharb · 2018-09-14T00:26:49Z

@jdalton at that point tho, wouldn't we still want to backport it to node 6 and 8? or are you saying it'd be easier at that point since the commits would already be in node master?

jdalton · 2018-09-14T00:56:17Z

We can revisit back-porting when it's unflagged.

devsnek · 2018-09-14T01:24:37Z

I don't think its crazy that we could ship es features independently of our underlying engine. maybe this is something worth discussing in the big picture?

ljharb · 2018-09-14T17:35:14Z

It is unflagged in v8. That that particular commit hasn't landed in node yet doesn't mean it's conceptually not ready to be shipped.

jdalton · 2018-09-14T17:41:17Z

I see no reason to step outside the normal rails of consuming V8 for this. We'll get this feature when it lands in the V8 Node consumes soon enough.

addaleax · 2018-09-14T17:43:02Z

@jdalton Being able to backport this to LTS release lines seems like a valid reason to me… any thoughts on that?

devsnek · 2018-09-14T18:03:14Z

@addaleax are we able to backport v8 70 and 71 in their entirety? the patches to v8 use apis only present in those versions. at that point we either have to make our own patch to v8, and at that point we might as well make the change like this pr does it.

I'm not against waiting for V8 versions, but rather unsure if we are able to backport it.

addaleax · 2018-09-14T18:07:14Z

@devsnek I would not think so. We may or may not be able to get 69 into Node 10, but 70 would be too late for the initial LTS release of Node 10, I think.

jdalton · 2018-09-14T18:21:53Z

@addaleax

Being able to backport this to LTS release lines seems like a valid reason to me… any thoughts on that?

See #22835 (comment).

ljharb · 2018-09-14T18:22:23Z

It’s already unflagged - so that means it’s time it revisit it now.

jdalton · 2018-09-14T18:30:54Z

I think we're talking past each other. I'm OK with globalThis but not OK implementing it here. I rather wait for the V8 consumed by Node to implement it. I'm OK back-porting a Node shim for this when the time comes (but it's not the time and this is not the PR).

ljharb · 2018-09-14T18:50:06Z

Can you elaborate on why? The proposal is stage 3; that’s the time for platforms to ship it.

jdalton · 2018-09-14T19:06:57Z

Can you elaborate on why? The proposal is stage 3; that’s the time for platforms to ship it.

Node will get it as soon as the V8 it consumes does. It's coming... just not-this-very-second and there's no real rush. It's a nice to have that's coming in a nice-to-have time.

ljharb · 2018-09-14T19:13:23Z

I hear that you don’t feel a need to rush - but since i (and likely others) do, perhaps you have more reasons then “i don’t feel it’s needed now” to push back against it?

jdalton · 2018-09-14T19:23:20Z

I'm familiar with the effort of globalThis (as a TC39 member I even OK'd its name at the last meeting) and I know you're the spec champion for it. The effort has been in the works for years off and on. I know you want it yesterday, but I'm saying no. No cuts. It will come soon enough.

ljharb · 2018-09-14T19:56:17Z

Can you provide a reason for saying no, though, besides "i don't want to rush"?

Also, I'm not sure why node needs to be held hostage to v8's release schedule for new APIs that don't require native support.

jdalton · 2018-09-14T20:27:19Z

@ljharb

Can you provide a reason for saying no, though, besides "i don't want to rush"?

I rather we get this without shimming (and it's coming soon). Adding the shim just to turn around and remove it is unnecessary and more overhead/juggling than it's worth. I'd like to avoid relying on per_context.js for it too since that has cross-engine issues.

devsnek · 2018-09-14T20:30:27Z

@jdalton per_context.js isn't v8 specific. it actually means node in chakra and v8 and whatever else would have globalThis. also I don't consider this a polyfill.

jdalton · 2018-09-14T20:38:15Z

@devsnek

per_context.js isn't v8 specific

I didn't say it was.

devsnek · 2018-09-14T20:55:37Z

@jdalton

I'd like to avoid relying on per_context.js for it too since that has cross-engine issues.

can you clarify?

jdalton · 2018-09-14T21:06:15Z

@devsnek

can you clarify?

Related problems: nodejs/node-chakracore#566.

TimothyGu · 2018-09-14T21:48:06Z

If node-chakracore cannot adopt the strategy used by this PR to add globalThis, then node-chakracore can certainly wait until ChakraCore adds support for it. I don’t remember seeing a policy where the feature set of node and node-chakracore must be in sync. Also, from an end user’s perspective there’s no difference in adding it now to node’s code versus V8’s code.

@jdalton would you be okay with cherry-picking the V8 patch that adds support for globalThis rather than doing it on the Node.js side?

As discussed at the TC39 meeting (which I am sure you remember) we want to aim to ship the feature as expediently as possible. I would really appreciate it if we could not block on what’s in my opinion a minor implementation detail.

jdalton · 2018-09-14T22:00:28Z

@TimothyGu

As discussed at the TC39 meeting (which I am sure you remember) we want to aim to ship the feature as expediently as possible.

I remember. However, I also remember the tail end of the presentation in which it was covered that there is no real smoosh or ticking clock concern (the TC39 won't be held up by malicious or bogus packages).

would you be okay with cherry-picking the V8 patch that adds support

If it's in the V8 we consume (cherry-picked in or otherwise) then it's in the V8 we consume.

TimothyGu · 2018-09-14T22:26:17Z

@jdalton

If it's in the V8 we consume (cherry-picked in or otherwise) then it's in the V8 we consume.

Awesome! @ljharb do you think you could backport the relevant V8 changes?

https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#backporting-to-abandoned-branches should provide enough documentation on how to do so.

ljharb · 2018-09-14T22:33:30Z

@TimothyGu absolutely; i did that here for 2 of them but didn't follow the proper commit format. I'll update the PR to include those backports.

refack · 2018-09-14T23:40:29Z

/CC @nodejs/chakracore

bugs181 · 2018-12-23T19:47:49Z

Where are we at with this? No activity since September, it's now 3 months later.

devsnek · 2018-12-23T20:07:19Z

I think the next V8 update has this unflagged (you can use it in node rn with --harnony-global)

joyeecheung · 2018-12-24T05:02:27Z

I think it's already unflagged on master?

Trott · 2018-12-24T05:12:55Z

I think it's already unflagged on master?

Correct. It's behind a flag in Node.js 11.5.0 (most recent release) but unflagged in master. This seems closable but a PR against 11.x-staging to float the relevant V8 patches may be desirable.

Trott · 2018-12-24T05:14:35Z

I'm going to close this, but please re-open or comment if you disagree that this ought to be closed at this time. I'm just tidying up and not acting on a super-strong opinion or anything like that.

ljharb · 2018-12-24T05:18:35Z

I’d still like to backport it if possible; I’m not sure if it’s worth repurposing this PR or not.

Issue on Jest jsdom/jsdom#2961 globalThis in Node.js nodejs/node#22835

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 13, 2018

devsnek added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 13, 2018

src: add globalThis

c26f2bd

Per https://github.com/tc39/proposal-global, the global value `globalThis` should be configurable, writable, and non-enumerable.

devsnek force-pushed the globalThis branch from 3096ae7 to c26f2bd Compare September 13, 2018 07:28

jasnell reviewed Sep 13, 2018

View reviewed changes

Trott approved these changes Sep 13, 2018

View reviewed changes

Trott requested changes Sep 13, 2018

View reviewed changes

jdalton suggested changes Sep 13, 2018

View reviewed changes

devsnek mentioned this pull request Sep 14, 2018

src,test: disable per_context.js shim nodejs/node-chakracore#566

Merged

2 tasks

kfarnung mentioned this pull request Sep 14, 2018

Atomics notify vs. wake nodejs/node-chakracore#565

Open

refack added lib / src Issues and PRs related to general changes in the lib or src directory. refactor to ES6+ labels Sep 14, 2018

ljharb mentioned this pull request Dec 19, 2018

Path to Stage 4! tc39/proposal-global#12

Closed

31 tasks

Trott closed this Dec 24, 2018

ljharb deleted the globalThis branch July 18, 2019 00:16

trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Oct 5, 2021

fix: set up globalThis for jest tests if not defined

64997cd

Issue on Jest jsdom/jsdom#2961 globalThis in Node.js nodejs/node#22835

trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Jan 4, 2022

fix: set up globalThis for jest tests if not defined

db212a4

Issue on Jest jsdom/jsdom#2961 globalThis in Node.js nodejs/node#22835

trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Jan 4, 2022

fix: set up globalThis for jest tests if not defined

14ad1a3

Issue on Jest jsdom/jsdom#2961 globalThis in Node.js nodejs/node#22835

src: add globalThis #22835

src: add globalThis #22835

Conversation

ljharb commented Sep 13, 2018

targos commented Sep 13, 2018

mscdex commented Sep 13, 2018 • edited Loading

ljharb commented Sep 13, 2018

devsnek commented Sep 13, 2018

tniessen commented Sep 13, 2018

ljharb commented Sep 13, 2018

jasnell Sep 13, 2018

Choose a reason for hiding this comment

ljharb Sep 13, 2018

Choose a reason for hiding this comment

jdalton commented Sep 13, 2018

ljharb commented Sep 13, 2018

Trott left a comment

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

jdalton left a comment • edited Loading

Choose a reason for hiding this comment

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018

devsnek commented Sep 14, 2018

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018

addaleax commented Sep 14, 2018

devsnek commented Sep 14, 2018 • edited Loading

addaleax commented Sep 14, 2018

jdalton commented Sep 14, 2018

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018

ljharb commented Sep 14, 2018

jdalton commented Sep 14, 2018 • edited Loading

devsnek commented Sep 14, 2018

jdalton commented Sep 14, 2018

devsnek commented Sep 14, 2018

jdalton commented Sep 14, 2018

TimothyGu commented Sep 14, 2018 • edited Loading

jdalton commented Sep 14, 2018

TimothyGu commented Sep 14, 2018

ljharb commented Sep 14, 2018

refack commented Sep 14, 2018

bugs181 commented Dec 23, 2018 • edited Loading

devsnek commented Dec 23, 2018

joyeecheung commented Dec 24, 2018

Trott commented Dec 24, 2018

Trott commented Dec 24, 2018

ljharb commented Dec 24, 2018

src: add `globalThis` #22835

src: add `globalThis` #22835

mscdex commented Sep 13, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

jdalton left a comment •

edited

Loading

devsnek commented Sep 14, 2018 •

edited

Loading

jdalton commented Sep 14, 2018 •

edited

Loading

TimothyGu commented Sep 14, 2018 •

edited

Loading

bugs181 commented Dec 23, 2018 •

edited

Loading