Closed Bug 1317422 (globalThis) Opened 8 years ago Closed 5 years ago

Implement "globalThis" proposal

Categories

(Core :: JavaScript: Standard Library, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: anba, Assigned: evilpie)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

"global" [1] is currently a stage 3 proposal [2, 3].

test262 already tests for the new "global" property [4] and we're currently failing some tests because of this.


[1] https://github.com/tc39/proposal-global
[2] https://github.com/tc39/proposals
[3] https://github.com/tc39/ecma262/pull/702
[4] https://github.com/tc39/test262/pull/765
Attached patch Patch (obsolete) — Splinter Review
#jslang suckered me into it.
Attachment #8815143 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
<3 Thanks!
Comment on attachment 8815143 [details] [diff] [review]
Patch

Review of attachment 8815143 [details] [diff] [review]:
-----------------------------------------------------------------

Let's find out if this breaks any websites! :-D

::: js/src/tests/ecma_2017/Global/global-property-enumeration.js
@@ +28,5 @@
> +assertEq(desc.configurable, true);
> +assertEq(desc.writable, true);
> +assertEq(desc.value, this);
> +
> +global = 42;

The other test case already tests that `global = 42` works, should this one instead test with `this.global = 42`?
Attachment #8815143 - Flags: review?(andrebargull) → review+
Patch works fine for shell, doesn't work for browser.  Turns out I need a ToWindowProxyIfWindow in there.  But even that's not enough, because in the browser embedding, nsGlobalWindow::SetNewDocument invokes Object class init (in the process of setting up the window's prototype chain) before it creates the WindowProxy wrapper and calls js::SetWindowProxy to store the wrapper in the WINDOW_PROXY slot in the Window.

The right fix to me seems to be to refactor things so the slot's filled early enough that Object class init doesn't have these oddities to deal with.  But SetNewDocument looks an awful lot like Mos Eisley -- a wretched hive of scum and villainy -- and doesn't look particularly refactorable, nor does it seem like a good use of time to do so.  (I want to refactor global object creation at *some* point, so that embeddings can basically declaratively specify what they want their global object to look like, including its prototype chain.  So I'll probably get back to this eventually.  Just, not now.)

With the Object init approach out, I stole the approach evilpie mentioned on IRC (aside: it's truly awful that there are multiple ways to define a new one-off global property), fixed the one moderately obvious bug in it, and am tryservering it now.  Will post for a fresh review if it comes back okay.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7cf02615566f272bd0ad7a89c865262a2a2ce62
Attached patch Patch, v2Splinter Review
Pretty sure jandem dealt with the ToWindow* gunk at one point, so let's throw this at him this time around.
Attachment #8816662 - Flags: review?(jdemooij)
Attachment #8815143 - Attachment is obsolete: true
Comment on attachment 8816662 [details] [diff] [review]
Patch, v2

Review of attachment 8816662 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8816662 - Flags: review?(jdemooij) → review+
Reminder to land this.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d37fc752b0af
Implement a global 'global' property whose value is the global object.  r=jandem
https://hg.mozilla.org/mozilla-central/rev/d37fc752b0af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Yay! When will this land in nightly and stable?
(In reply to ljharb from comment #10)
> Yay! When will this land in nightly and stable?

It will be in today's Nightly. Firefox 53 will be released April 18th, according to https://wiki.mozilla.org/RapidRelease/Calendar
Depends on: 1325907
Depends on: 1326032
Depends on: 1328218
Let's back this out. This is not web-compatible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch BackoutSplinter Review
Attachment #8823427 - Flags: review?(jwalden+bmo)
Comment on attachment 8823427 [details] [diff] [review]
Backout

Review of attachment 8823427 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +1076,5 @@
>      // Object.prototype yet. Force it now.
> +    if (!global->getOrCreateObjectPrototype(cx))
> +        return false;
> +
> +    return true;

Mind also landing a separate patch, rs=me, that undoes this undoing, and keeps |global| as a Handle?
Attachment #8823427 - Flags: review?(jwalden+bmo) → review+
Is there a way we could still include `global` behind a flag in nightly, so that continued testing can be done as sites fix their issues?
Sure, we could add something to CompartmentOptions to frob it on, easily enough.  Invoking the frob other than as an all-or-nothing approach, tho, seems difficult.  I'm not sure exactly how useful such a thing is.  We've used such frobs to feature-guard stuff in progress, like some Intl functionality, but that was only with the aim of having an opt-in in shell tests to enable it.  And obviously, we can't expose such a thing to web content.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5f92e32f1d
Backout the global property propsal. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/89388a238c25
Keep nicer code in JS_ResolveStandardClass. rs=Waldo
How about enabling this just for 'strict' contexts, wouldn't it be more compatible ir current web content?
https://hg.mozilla.org/mozilla-central/rev/aa5f92e32f1d
https://hg.mozilla.org/mozilla-central/rev/89388a238c25
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
Status: REOPENED → ASSIGNED
After implementing it in Safari Tech Preview 21
> Added support for the global property on the global object (r210052)
https://webkit.org/blog/7265/release-notes-for-safari-technology-preview-21/

It has been reverted for Web Compatibility reasons
See https://bugs.webkit.org/show_bug.cgi?id=166915

Ryosuke Niwa said
> We had to rollout this feature it broke Polymer tests.
https://bugs.webkit.org/show_bug.cgi?id=165171#c22
Depends on: 1422557
Priority: -- → P5
QA Contact: jorendorff
From Alexandre Folle de Menezes in bug 1496748
> The 'global' proposal was renamed to 'globalThis'.  I believe this was
> already implemented with the old name, but disabled due to compatibility
> issues.
> 
> Chrome 70 already has the renamed implementation.
QA Contact: jorendorff
Should be easy to implement this. I would volunteer to do it.
Assignee: jwalden+bmo → evilpies
Oh the original patch was actually not quite correct: We would resolve "global" again after it was deleted. Thanks test262 for catching this!
Summary: Implement "global" proposal → Implement "globalThis" proposal
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e6a2dbfa104
Implement JavaScript globalThis proposal. r=jandem
https://hg.mozilla.org/mozilla-central/rev/1e6a2dbfa104
Status: ASSIGNED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Alias: globalThis
Note to MDN writers:

I have added to note to the Fx65 rel notes to cover this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#JavaScript

It would be great to get this documented fairly promptly in the new year. The Trello card is here:
https://trello.com/c/aRcYBQqA/150-globalthis-fx-65

Awesomely enough, we already seem to have it in Chinese, added by a volunteer:
https://developer.mozilla.org/zh-CN/docs/Web/JavaScript/Reference/Global_Objects/globalThis

We need to do the following tasks on MDN:

Add browser compat data:

Add reference docs:

Add interactive example:

Links from other docs

We should probably link to the globalThis page from these MDN pages:

Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: