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.

Sign up for GitHub

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

closes #218: adding Realm.prototype.import(specifier) #223

Merged
merged 3 commits into from
Mar 4, 2020
Merged

closes #218: adding Realm.prototype.import(specifier) #223

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b69afc3
closes #218: adding Realm.prototype.import(specifier)
caridy Feb 26, 2020
eacb36f
Merge branch 'master' into caridy/issue-218
caridy Feb 28, 2020
7cd1790
Normative: Run HostImportModuleDynamically in the correct Realm (#233)
caridy Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions spec/index.emu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,38 @@ location: https://rawgit.com/tc39/proposal-realms/master/index.html
</emu-note>
</emu-clause>

<emu-clause id="sec-realm.prototype.import">
<h1>Realm.prototype.import ( _specifier_ )</h1>

The following steps are taken:

<emu-alg>
1. Let _O_ be *this* value.
1. Perform ? RequireInternalSlot(_O_, [[Realm]]).
1. Let _referencingScriptOrModule_ be *null*.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erights @littledan @ljharb this is an interesting question to be answered. What is the referencing scriptOrModule when calling r.import()? should be null? should be the code that creates the Realm object? if so, what are the implications?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be an optional parameter passed to Realm.prototype.import? otherwise it'd have to be "null", because this isn't syntactic, it's API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import method called here is a first class function, not a special form. When it calls the underlying operation, it should pass null as the referrer. The first class import method should not take an optional referrer parameter, nor should it provide a referrer based on any relationship to the creator of the Realm or Compartment. Just null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard for me to understand what kind of value would be passed into this second argument. We need a script or module record, but those aren't exposed as first-class values to JavaScript.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes from SES meeting (yesterday) about this question:

  • @erights convinced me that a second argument is problematic. the convincing factor was that if evaluation is available, then myRealm.globalThis.eval('import("./something")') should work, and at that point, you will have no way to provide a value to the referencing scriptOrModule.
  • we need to decide whether the value should be *null* or *undefined*.
  • in the meeting, we reached consensus about the behavior here, the realm object will ask the host for the resolution, by passing null, and it is the host's responsibility to decide what value to use for the referencing scriptOrModule.
  • eventually, once we have a way to control the host (via compartment/evaluator), you will be able to control the behavior of such resolution.

@erights did I miss anything here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the Realm proposal, yes, I think that's everything relevant.

For reference, the meeting is at https://www.youtube.com/watch?v=hqKGKDw-FCY&list=PLzDw4TTug5O1jzKodRDp3qec8zl88oxGd which includes further considerations relevant to the SES / Compartment proposal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erights convinced me that a second argument is problematic. the convincing factor was that if evaluation is available, then myRealm.globalThis.eval('import("./something")') should work, and at that point, you will have no way to provide a value to the referencing scriptOrModule.

So what did you think of my idea to provide a "base" string to hosts?

we need to decide whether the value should be null or undefined.

I think the choice of null vs undefined is just editorial--this is only visible to hosts. I'm fine with either.

in the meeting, we reached consensus about the behavior here, the realm object will ask the host for the resolution, by passing null, and it is the host's responsibility to decide what value to use for the referencing scriptOrModule.

I think we should think through what to do for some environments before Stage 3 (#225). For HTML, for example, do you think this should be relative to where the whole page is, or should paths be forced to be absolute? And what should this be in Node.js? (I think the experience with ES6 modules showed that putting these decisions off has cost.)

eventually, once we have a way to control the host (via compartment/evaluator), you will be able to control the behavior of such resolution.

Would this have a way to tell the host the "base URL" or equivalent, for environments where this is meaningful? Are there proposals that I could review on this?

For reference, the meeting is at ...

FWIW, if there are written notes, these are generally easier for me to follow than a video.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledan yes, we discussed the resolution in browsers as well, sorry I didn't add that part to the relevant notes above. The reasoning was that doing r.import('...') works just like doing <script type="module" src="..."> in browsers. In other words, the module that creates the realm does not have a saying on the resolution or base referral definition. These two features will be available as part of the compartment/evaluator where you control every aspect of the host for any realm object underneath. In other words, "relative to where the whole page" if the specifier is relative. If it is absolute, then the base has no saying in the resolution.

As for the notes, no, we don't have written notes for SES meetings as far as I can tell, only the video recorded by @erights

1. Let _specifierString_ be ? ToString(_specifier_).
1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
1. IfAbruptRejectPromise(_specifierString_, _promiseCapability_).
1. Let _callerContext_ be the running execution context.
1. Let _newContext_ be a new execution context.
1. Set the Function of _newContext_ to *null*.
1. Set the Realm of _newContext_ to _O_.[[Realm]].
1. Set the ScriptOrModule of _newContext_ to *null*.
1. Push _newContext_ onto the execution context stack; _newContext_ is now the running execution context.
1. Perform ! HostImportModuleDynamically(_referencingScriptOrModule_, _specifierString_, _promiseCapability_).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erights @littledan @ljharb this is another surprising part for me, I could not find any reference to the realm in the import call specification. Is it underspecified? or am I missing something? I'm talking about https://tc39.es/ecma262/#sec-import-calls and the abstract operations called from there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably assumed to be implicit? not sure tho.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"assumed implicit" === underspecified. I suspect the spec has a lot of these bugs that we'll need to fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML figures this out using the ES spec's current realm concept. I imagine other hosts could do the same. That works today (there's nothing unspecified, and even import() within an evaluate in a Realm should work fine), but wouldn't work for the implementation of this method, and needs to be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledan let's work on that particular piece in another PR to avoid confusion.

Copy link
Member

@littledan littledan Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this a bit more, I think the simplest way through this issue would be to add a dummy execution context stack entry to set the current realm appropriately. This can be done by cribbing lines 2-6 of InitializeHostDefinedRealm, and saving and restoring the surrounding calling execution context as in steps 1, 9 and 11 of [[Call]]. What would you think of applying that change as part of this patch? We could do the more comprehensive refactoring as part of upstreaming the Realm proposal into the main spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix proposed in #233

1. Remove _newContext_ from the execution context stack and restore _callerContext_ as the running execution context.
1. Return _promiseCapability_.[[Promise]].
</emu-alg>

<emu-note>
Extensible web: This is equivalent to dynamic import without having to evaluate a script source, which might not be available (e.g.: when CSP is blocking source evaluation).
</emu-note>

<emu-note type=editor>
An extra execution context is created around the call to HostImportModuleDynamically in order to execute within the appropriate Realm--the host can reference the current Realm to figure out how the import should work. An alternative phrasing would be to pass the Realm as a parameter to HostImportModuleDynamically, which could be revisited when merging into the main specification.
</emu-note>
</emu-clause>

<emu-clause id="sec-realm.prototype.globalthis">
<h1>get Realm.prototype.globalThis</h1>

Expand Down