closes #183: `evaluation(sourceText, options)` to support evaluating modules and scripts #224

caridy · 2020-02-26T21:41:31Z

No description provided.

caridy · 2020-02-26T21:41:55Z

spec/index.emu

-    <emu-clause id="sec-performRealmEvaluation" aoid="PerformRealmEvaluation">
-      <h1>PerformRealmEvaluation ( _x_, _evalRealm_ )</h1>
+
+    <emu-clause id="sec-perform-realm-script-evaluation" aoid="PerformRealmScriptEvaluation">


just renaming things to match the new abstract operation for modules

caridy · 2020-02-26T21:43:46Z

spec/index.emu

+          1. Else if _sourceType_ is *"script"*, then
+            1. Return ? PerformRealmScriptEvaluation(_sourceText_, _realm_).
+          1. Else
+            1. Throw a RangeError exception.


@erights @ljharb for now I'm assuming that we want to throw if options.type is not provided or is invalid value. We can definitely relax this, and add a default value. Any thoughts?

const r = new Realm(); r.evaluate(`1`, { type: 'module' }); r.evaluate(`2`, { type: 'script' });

seems better to keep it explicit, but at that point why have it in the options bag instead of as a required second argument?

good question! two main reasons that we have discussed:

evaluate() is also a thing in evaluator/compartment, just with more stuff on it, like endowments.

maybe in the future we will have more options here, like strict or sloppy, etc.

But I'm fine either way. We can overload the arguments if we need more options in the future. @erights any opinion here?

I'd generally expect options bags to only hold optional arguments; in the case of "evaluate", the parse goal it's evaluating in seems pretty first-class to me. In other words, I'd expect an optional options bag (now or in the future) to be the third argument.

feedback from the SES meeting yesterday about this question:

majority of the group will like to have a default value. I still think being explicit about this is a safer (it is more restrictive, and can be relaxed in the future).

default value to strict-script, which is kinda a new concept, their argument was to not favor sloppy mode, which is still possible, but only via eval from globalThis. No capability lost there, just less convenience.

if there is a default value, then it is better to have the options bag. which I believe all agree, but first lets get to some consensus about that first question.

there was some discussion about tc53 use cases, and whether or not some folks will simply be able to throw when type=module is provided, we need more info about this use-case, and how that impact the API.

@erights did I miss anything?

@erights did I miss anything?

That's what I remember, yes. For reference, the mtg is at https://www.youtube.com/watch?v=hqKGKDw-FCY&list=PLzDw4TTug5O1jzKodRDp3qec8zl88oxGd

Did the SES meeting consider

Using separate method names, as these really have different types, and for the Promise vs not issue @zenparsing identified?

Having an expression form, as @erights initially proposed at the start of the thread?

caridy · 2020-02-26T21:45:31Z

spec/index.emu

-      <h1>PerformRealmEvaluation ( _x_, _evalRealm_ )</h1>
+
+    <emu-clause id="sec-perform-realm-script-evaluation" aoid="PerformRealmScriptEvaluation">
+      <h1>PerformRealmScriptEvaluation ( _sourceText_, _realm_ )</h1>


this operation must match https://tc39.es/ecma262/#sec-scriptevaluationjob

caridy · 2020-02-26T21:46:05Z

spec/index.emu

      1. If _s_ is a List of errors, then
        1. Perform HostReportErrors(_s_).
        1. Return NormalCompletion(*undefined*).
      1. Return ? ScriptEvaluation(_s_).
      </emu-alg>
    </emu-clause>
+
+    <emu-clause id="sec-perform-realm-module-evaluation" aoid="PerformRealmModuleEvaluation">
+      <h1>PerformRealmModuleEvaluation ( _sourceText_, _realm_ )</h1>


this operation must match https://tc39.es/ecma262/#sec-toplevelmoduleevaluationjob

I don't think this should be taken as a requirement. That abstract operation is going away in https://github.com/tc39/ecma262/pull/1597/files#diff-3540caefa502006d8a33cb1385720803L24193 , which TC39 has reached consensus on.

once that's merged, we can adjust here I think.

This wording seems fine to me, FWIW.

littledan · 2020-02-27T19:13:23Z

What's the motivation for this PR?

littledan · 2020-02-28T16:47:36Z

Note, I think we'll need some host integration here. Right now, hosts can assume that all module evaluation starts with them (since currently, hosts have to provide modules to ES), so they have some time to set modules up and add appropriate metadata, e.g., so that dynamic import() inside them works. If JS itself is creating modules, then we should consider adding a host hook to allow the appropriate logic to be inserted.

spec/index.emu

…dules and scripts

Part of addressing tc39#225 In HTML, this host hook would probably do something related to [setting up a window environment settings object](https://html.spec.whatwg.org/#set-up-a-window-environment-settings-object). I wouldn't propose that this would add properties to the global object in HTML. This PR makes a new host hook section, as another hook may be needed for new modules (in the context of tc39#224)

* Layering: Host hook to initialize new Realms Part of addressing #225 In HTML, this host hook would probably do something related to [setting up a window environment settings object](https://html.spec.whatwg.org/#set-up-a-window-environment-settings-object). I wouldn't propose that this would add properties to the global object in HTML. This PR makes a new host hook section, as another hook may be needed for new modules (in the context of #224) * Note the purpose of the hook

Rationale: To get to a minimal Realms MVP, the functionality can be accessed by `realm.globalThis.eval` (stash off to the side soon after creating realm to use reliably). Omitting this method for now would maximize the flexibility of the continued development of the Compartment/Evaluator API; that proposal could add an analogous method to Realm.prototype as part of the same proposal. Closes tc39#226 Alternative to tc39#224

caridy · 2020-03-05T22:56:53Z

closing in favor of #235

Rationale: To get to a minimal Realms MVP, the functionality can be accessed by `realm.globalThis.eval` (stash off to the side soon after creating realm to use reliably). Omitting this method for now would maximize the flexibility of the continued development of the Compartment/Evaluator API; that proposal could add an analogous method to Realm.prototype as part of the same proposal. Closes #226 Alternative to #224

caridy commented Feb 26, 2020

View reviewed changes

caridy requested review from ljharb, erights and littledan February 26, 2020 21:44

caridy commented Feb 26, 2020

View reviewed changes

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

littledan reviewed Feb 28, 2020

View reviewed changes

spec/index.emu Outdated Show resolved Hide resolved

spec/index.emu Outdated Show resolved Hide resolved

spec/index.emu Show resolved Hide resolved

zenparsing reviewed Feb 28, 2020

View reviewed changes

spec/index.emu Outdated Show resolved Hide resolved

caridy added a commit that referenced this pull request Feb 28, 2020

PR #224: feedback

f6e61b7

caridy added 2 commits February 28, 2020 12:45

closes #183: evaluation(sourceText, options) to support evaluating mo…

8cd87aa

…dules and scripts

PR #224: feedback

722a3dd

caridy force-pushed the caridy/issue-183 branch from f6e61b7 to 722a3dd Compare February 28, 2020 17:46

littledan mentioned this pull request Feb 28, 2020

Layering: Host hook to initialize new Realms #230

Merged

littledan mentioned this pull request Mar 2, 2020

New evaluators should always have suffix clarifying their start production #183

Closed

littledan mentioned this pull request Mar 5, 2020

Normative: Remove evaluate method #235

Merged

caridy closed this Mar 5, 2020

caridy deleted the caridy/issue-183 branch March 5, 2020 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

closes #183: `evaluation(sourceText, options)` to support evaluating modules and scripts #224

closes #183: `evaluation(sourceText, options)` to support evaluating modules and scripts #224

caridy commented Feb 26, 2020

caridy Feb 26, 2020

caridy Feb 26, 2020

ljharb Feb 26, 2020

caridy Feb 26, 2020

ljharb Feb 26, 2020

caridy Feb 28, 2020 •

edited

Loading

erights Feb 28, 2020

littledan Feb 28, 2020

caridy Feb 26, 2020 •

edited

Loading

caridy Feb 26, 2020

littledan Feb 28, 2020

caridy Feb 28, 2020

littledan Feb 28, 2020

littledan commented Feb 27, 2020

littledan commented Feb 28, 2020

caridy commented Mar 5, 2020

closes #183: evaluation(sourceText, options) to support evaluating modules and scripts #224

closes #183: evaluation(sourceText, options) to support evaluating modules and scripts #224

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

caridy Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy Feb 26, 2020 • edited Loading

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 commented Feb 27, 2020

littledan commented Feb 28, 2020

caridy commented Mar 5, 2020

closes #183: `evaluation(sourceText, options)` to support evaluating modules and scripts #224

closes #183: `evaluation(sourceText, options)` to support evaluating modules and scripts #224

caridy Feb 28, 2020 •

edited

Loading

caridy Feb 26, 2020 •

edited

Loading