-
Notifications
You must be signed in to change notification settings - Fork 67
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
adding step for implementation-defined globals as part of the creation of the realm #236
Conversation
@@ -74,6 +74,7 @@ location: https://rawgit.com/tc39/proposal-realms/master/index.html | |||
1. Set _O_.[[Realm]] to _realmRec_. | |||
1. Perform ? SetRealmGlobalObject(_realmRec_, *undefined*, *undefined*). | |||
1. Perform ? SetDefaultGlobalBindings(_O_.[[Realm]]). | |||
1. Create any implementation-defined global object properties on _globalObj_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matching https://tc39.es/ecma262/#sec-initializehostdefinedrealm - step 11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the common steps between this, and InitializeHostDefinedRealm, could be factored out into a shared abstract operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we discussed this extensibly today in a bigger context. @erights raised valid concerns about clause 18 (https://tc39.es/ecma262/#sec-global-object) last bullet point:
may have host defined properties in addition to the properties defined in this specification. This may include a property whose value is the global object itself.
That is part of SetDefaultGlobalBindings
abstract operation (which iterates over every property of each property of the Global Object as defined by clause 18.
The two main concerns are:
- "implementation-defined properties" vs "host defined properties". that on itself is confusing.
- having that as part of the global object definition makes it hard for compartments, and other ways to create a global object in the future if we want to introduce user-defined properties via some hooks.
The recommendation at this point is to create a new abstract operation, and call that right after SetDefaultGlobalBindings()
, and remove the last bullet of clause 18 entirely. This change has no impact on implementation, it is just spec refactor.
@ljharb @littledan can we do that on a separate PR for 262? or should we continue with that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo anything that can hit 262 separate from a proposal, should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb what? Can you rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Any editorial refactorings that can land in 262 without consensus, and make nonzero sense without a proposal, should imo land prior to the proposal".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good, thanks.
I'm having trouble understanding this patch. I never interpreted that line to be "invoke Annex B" but rather "add host-defined globals like window, fetch, etc". I thought the Annex B properties were added the same way as non-Annex B properties. My initial thought is, we should not have this line. The typical global object has a bunch of properties added by the host, but in Realms, we should not have these properties. Therefore, we shouldn't copy the logic from the initialization of the global object that adds these properties. To close #22, I'd simply clarify, in the introduction to Annex B, that it can be included in Realms. In the introduction to Annex B, something like the following:
(I'd be happy with weaker wording as well, but this stronger wording is what seems to fit naturally with what's written there already. I'd also be fine with this addition being in a note, as it could be seen to be logically implied from existing text.) |
@littledan while I do agree about #22, and that paragraph for Annex B looks great to me, I do believe that the change in this PR is more about specifying when can the host make such determination in preparation for the compartment/evaluator to provide a hook for it in the future. As for the wording of that specific step, I don't feel attached to it, we can rewording it. |
I'd rather use host hooks to indicate where the host can add behavior, as we did in #230. Host hooks are nice because they give us an opportunity to may out invariants for the hook. For example, that documentation makes it clear that the host should not add properties. This PR does the opposite, specifically encouraging the host to add properties, which risks making Realms less interoperable. At a high level: we shouldn't copy this language from the ordinary global object, since it has host-created properties that Realms are intended not to have! |
Closing this in favor of the current spec text. |
closes #22