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

caridy · 2020-02-26T20:50:28Z

Realm.prototype.import(specifier):

equivalent to import(specifier)
works even when CSP blocks any script evaluation
matches API for the compartments/evaluator proposal

caridy · 2020-02-26T20:52:41Z

spec/index.emu

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


@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?

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.

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.

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.

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?

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.

@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.

@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

caridy · 2020-02-26T20:54:45Z

spec/index.emu

+          1. Let _specifierString_ be ? ToString(_specifier_).
+          1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
+          1. IfAbruptRejectPromise(_specifierString_, _promiseCapability_).
+          1. Perform ! HostImportModuleDynamically(_referencingScriptOrModule_, _specifierString_, _promiseCapability_).


@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.

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

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

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.

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

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.

Fix proposed in #233

littledan

This API looks good to me, but it seems like we have some spec work ahead of us, e.g., in #225 .

littledan · 2020-02-27T19:09:56Z

spec/index.emu

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


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.

littledan · 2020-02-27T22:10:31Z

spec/index.emu

+          1. Let _specifierString_ be ? ToString(_specifier_).
+          1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
+          1. IfAbruptRejectPromise(_specifierString_, _promiseCapability_).
+          1. Perform ! HostImportModuleDynamically(_referencingScriptOrModule_, _specifierString_, _promiseCapability_).


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.

* Normative: Run HostImportModuleDynamically in the correct Realm * Update spec/index.emu Co-Authored-By: Jordan Harband <ljharb@gmail.com> Co-authored-by: Caridy Patiño <caridy@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>

littledan · 2020-03-05T09:43:23Z

Happy to see that this landed; retroactive LGTM!

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

b69afc3

caridy commented Feb 26, 2020

View reviewed changes

caridy mentioned this pull request Feb 26, 2020

CSP compatibility - evaluate code from resource uri #214

Closed

caridy requested a review from jdalton February 26, 2020 21:46

littledan mentioned this pull request Feb 27, 2020

Define host environment interactions for modules #225

Closed

littledan reviewed Feb 27, 2020

View reviewed changes

Merge branch 'master' into caridy/issue-218

eacb36f

littledan mentioned this pull request Feb 28, 2020

Ideas for making Realms more usable without eval'ing strings #218

Closed

caridy merged commit 3bffb70 into master Mar 4, 2020

caridy deleted the caridy/issue-218 branch March 4, 2020 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

caridy commented Feb 26, 2020

caridy Feb 26, 2020

ljharb Feb 26, 2020

erights Feb 26, 2020

littledan Feb 27, 2020

caridy Feb 28, 2020

erights Feb 28, 2020

littledan Feb 28, 2020

caridy Feb 28, 2020

caridy Feb 26, 2020

ljharb Feb 26, 2020

erights Feb 26, 2020

littledan Feb 27, 2020

caridy Feb 28, 2020

littledan Feb 28, 2020 •

edited

Loading

littledan Mar 4, 2020

littledan left a comment

littledan Feb 27, 2020

littledan Feb 27, 2020

littledan commented Mar 5, 2020

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

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

Conversation

caridy commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Mar 5, 2020

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

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

littledan Feb 28, 2020 •

edited

Loading