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.

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

Normative: add `globalThis` #702

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@ljharb
Copy link
Member

ljharb commented Sep 29, 2016

  • Adds intrinsic %GlobalThisValue%
  • Sets it in SetRealmGlobalObject
  • Adds global name globalThis

Per tc39/proposal-global#12

Normative: add `globalThis` (#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12

@ljharb ljharb referenced this pull request Sep 29, 2016

Open

Path to Stage 4! #12

25 of 30 tasks complete
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Sep 29, 2016

Note: do not merge until stage 4 - an approved PR is now a pre-stage-4 requirement.

@ljharb ljharb force-pushed the ljharb:global branch from d051137 to 23fa426 Oct 1, 2016

@ljharb ljharb force-pushed the ljharb:global branch from 23fa426 to 300fd51 Dec 21, 2016

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Mar 5, 2018

For context, the name global has web compatibility issues. @ljharb is working on switching to a new name. This patch will not land in its current state.

@ljharb ljharb self-assigned this Mar 5, 2018

@ljharb ljharb added the proposal label Mar 21, 2018

@ljharb ljharb force-pushed the ljharb:global branch from 300fd51 to ad0f815 Sep 12, 2018

ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 12, 2018

Normative: add `globalThis` (tc39#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12

@ljharb ljharb changed the title Normative: add `global` Normative: add `globalThis` Sep 12, 2018

@ljharb ljharb requested review from bterlson , bmeck and tc39/ecma262-editors Sep 12, 2018

Show resolved Hide resolved spec.html Outdated

@ljharb ljharb force-pushed the ljharb:global branch from ad0f815 to bd87080 Sep 12, 2018

ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 12, 2018

Normative: add `globalThis` (tc39#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12

@ljharb ljharb assigned bterlson and unassigned ljharb Dec 19, 2018

@ljharb ljharb requested a review from zenparsing Dec 19, 2018

@ljharb ljharb requested review from tc39/ecma262-editors and removed request for bterlson Feb 28, 2019

Show resolved Hide resolved spec.html Outdated
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Mar 7, 2019

FYI to TC39: there is an interesting question of what globalThis should be for discarded windows. (I.e. those whose parent iframe was .remove()ed, or whose window was .close()ed.) It appears Chrome/Safari/Firefox all continue to return the global object in that case. However, for self/window/frames, Safari returns null instead.

@annevk filed a bug on Safari to align their window/self/frames with Chrome/Firefox, and also with their own globalThis implementation.

So, there is no action here for TC39, but I thought it'd be interesting to know.

@ljharb ljharb force-pushed the ljharb:global branch from bd87080 to 930c246 Mar 8, 2019

ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 8, 2019

Normative: add `globalThis` (tc39#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Mar 8, 2019

@annevk @domenic I've added a second commit that uses a slot on the Realm instead of an intrinsic. What are your thoughts on this approach?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 8, 2019

Looks reasonable. I'm not super familiar with data properties though.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Mar 8, 2019

This approach made it clearer that there's actually some serious redundancy we could reduce here. The spec already has a [[GlobalThisValue]] on the realm's [[GlobalEnv]]. So my suggestion would be to remove the new slot, and either change GetGlobalThis() to go through the current Realm Record's [[GlobalEnv]]'s [[GlobalThisValue]], or perhaps just inline it.

Also, I don't think there's a need to go from running execution context to that context's realm; you can just use "the current Realm Record".

@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Mar 8, 2019

@domenic

That makes sense too - I went with the new slot to avoid the indirection, but if the redundancy is considered messier, I can update it to do that.

Also, I don't think there's a need to go from running execution context to that context's realm; you can just use "the current Realm Record".

I followed the existing pattern in GetGlobalObject; I'm fine to change both though if that phrase would be sufficient.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Mar 8, 2019

That makes sense too - I went with the new slot to avoid the indirection, but if the redundancy is considered messier, I can update it to do that.

Yeah, I definitely would. Otherwise it's not obvious to the reader that the two are always in sync.

I followed the existing pattern in GetGlobalObject; I'm fine to change both though if that phrase would be sufficient.

It looks like that phrase is sufficient. The current Realm record is literally defined to be the Realm of the running execution context. So this was just missed previously.

Separately, it's unclear whether you need GetGlobalThis(), since its only "caller" is not really a caller. GetGlobalObject() has an actual caller so that makes sense there. Indeed, the definition is a bit weird, since there's not really a running execution context when the global properties are being set up: the spec explicitly says there isn't one in fact.

Finally I'll note that this arguably belongs in https://tc39.github.io/ecma262/#sec-value-properties-of-the-global-object instead of in https://tc39.github.io/ecma262/#sec-other-properties-of-the-global-object

So I think this could all reduce to "The value of globalThis is the Realm's [[GlobalEnv]]'s [[GlobalThisValue]]." The fuzzy part here is "the Realm"; it's alluding to the fact that each global has an associated Realm, and assuming we can reference it while defining properties of that global. I haven't seen that done before, but I can't think of any other way to do things, and as I noted above, using "the current Realm" would technically be incorrect since there is no running execution context.

@ljharb ljharb force-pushed the ljharb:global branch from 930c246 to a1ce27b Mar 8, 2019

ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 8, 2019

Normative: add `globalThis` (tc39#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Mar 8, 2019

Thanks, I've updated the second commit, using the existing terminology "the current Realm record", and using the longer form (described here) of obj.[[slot]], "the field of obj named [[slot]]" - PTAL.

Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated

@ljharb ljharb force-pushed the ljharb:global branch from a1ce27b to 495a51e Mar 9, 2019

ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 9, 2019

Normative: add `globalThis` (tc39#702)
 - Adds intrinsic `%GlobalThisValue%`
 - Sets it in `SetRealmGlobalObject`
 - Adds global name `globalThis`

Per tc39/proposal-global#12
@@ -23338,6 +23337,12 @@ <h1>The Global Object</h1>
<emu-clause id="sec-value-properties-of-the-global-object">
<h1>Value Properties of the Global Object</h1>

<emu-clause id="sec-global-this">
<h1>globalThis</h1>
<p>The initial value of `globalThis` is the value of the field of (the value of the field of the Realm Record named [[GlobalEnv]]) named [[GlobalThisValue]].

This comment has been minimized.

@ljharb

ljharb Mar 9, 2019

Author Member

this phrasing is super awkward. Any recommendations of how to do this cleaner without algorithm steps?

This comment has been minimized.

@jmdyck

jmdyck Mar 9, 2019

Collaborator
Suggested change
<p>The initial value of `globalThis` is the value of the field of (the value of the field of the Realm Record named [[GlobalEnv]]) named [[GlobalThisValue]].
<p>The initial value of the `globalThis` property of the global object of a realm _realm_ is the value of `_realm_.[[GlobalEnv]].[[GlobalThisValue]]`.</p>

That fudges the distinction between a realm and a Realm Record, so you could say ... of a realm represented by the Realm Record _realm_ ..., but I'm doubtful that'd be better.

@@ -23338,6 +23337,12 @@ <h1>The Global Object</h1>
<emu-clause id="sec-value-properties-of-the-global-object">
<h1>Value Properties of the Global Object</h1>

<emu-clause id="sec-global-this">

This comment has been minimized.

@jmdyck

jmdyck Mar 11, 2019

Collaborator

Sorry, now I'm thinking this clause shouldn't be "Value Properties" at all, it should be in "Other Properties".

The term "value property" isn't defined, but it seems to be used as a shorthand for "primitive value property", so globalThis doesn't belong.

This comment has been minimized.

@ljharb

ljharb Mar 11, 2019

Author Member

I don’t read that at all - it happens to be just primitives, but “value” to me doesn’t imply that. @allenwb do you have context here on the intention here between “value” and “other”?

This comment has been minimized.

@jmdyck

jmdyck Mar 11, 2019

Collaborator

(The term "value" doesn't imply "primitive value" to me either, I'm just saying that seems to be the criterion of division.)

This comment has been minimized.

@jmdyck

jmdyck Mar 11, 2019

Collaborator

(BTW, the heading "Value properties of the global object" goes all the way back to ES1.)

This comment has been minimized.

@domenic

domenic Mar 11, 2019

Member

My reading was that "Other Properties" was for namespaces and "Value Properties" was for everything else.

Maybe best to just merge the two sections.

This comment has been minimized.

@ljharb

ljharb Mar 11, 2019

Author Member

Hmm - "Other" contains only namespaces that are referenced elsewhere - maybe that's the distinction?

Whether they're merged or not, I think "Value Properties" is the proper place for it.

This comment has been minimized.

@jmdyck

jmdyck Mar 11, 2019

Collaborator

Okay, plausible enough for me.

@ljharb ljharb force-pushed the ljharb:global branch from 495a51e to 109593b Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.