Various editorial improvements #1

littledan · 2019-02-09T22:20:17Z

See individual commit messages for details.

This reverts commit e4ee72a. The spec-level Promise.all here was well-written! Let's keep it, but consider follow-on editorial tweaks to match ES spec style.

- Evaluate() method returns a Promise which settles when the module is ready. - Move the [[ExecPromise]] internal slot to Source Text Module Record (eventual intended home: Circular Module Record). - Avoid using constructs like AwaitWithReturn, BuiltinPromiseThen and even Await, and instead deal with Promise reactions directly for clarity. - Specialize the spec-level Await to the particular case of continuing on to execute the module. - Various small editorial cleanups/fixes.

The purpose of this patch is to nail down how HTML would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready, and this will be passed to FinishDynamicImport to wait on before resolving the Promise it returns. It doesn't seem like anyone else needs to wait for a module script to "finish" evaluating besides dynamic import. This PR is just a sketch, not to be landed. Top-level await should reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

The purpose of this patch is to nail down how dynamic import would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready, and this will be passed to FinishDynamicImport to wait on before resolving the Promise it returns. This PR is just a sketch, not to be landed. For one, the "Upon fulfillment" language will need to be changed to be more ECMASpeak-like. Top-level await should also reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

littledan · 2019-02-10T01:54:13Z

See matching HTML and dynamic import PRs.

Closes tc39#17

guybedford

Great work, thanks @littledan so much for picking this up, would love to see this move forward.

Please feel free to close out my PR and continue this as your own PR directly against the top-level-await repo. I will gladly collaborate further as well.

guybedford · 2019-02-17T13:26:31Z

spec.html

+        1. Perform ! ModuleExecution(_module_, _capability_).
+        1. Return.
+      1. Let _index_ be 0.
+      1. Let _fullfilledCount_ be 0.


Could abstract this machinery as a spec-based Promise.all since it is so common?

Maybe? I would rather wait for a second use case to come along so I can figure out what is needed in general.

Sure, that is understandable. If I recall this pattern already exists in some other async spec code, but I can't recall where exactly.

Within the JS spec, or in higher-level specifications? I have only seen it in the W3C Promise guide.

I doubt the W3C Promise guide would reference a Stage 2 draft; maybe we could make this editorial change at some later point (e.g., when integrating into the main spec) when it would make sense for that reference to be added.

Actually I think the only other similar pattern is in the implementation of Promise.all itself :)

guybedford · 2019-02-17T13:27:46Z

spec.html

+      1. Perform ? _m_.Instantiate().
+      1. Assert: All dependencies of _m_ have been transitively resolved and _m_ is ready for evaluation.
+      1. <del>Return</del><ins>Let _promise_ be</ins> ? _m_.Evaluate().
+      1. <ins>Upon rejection of _promise_ with reason _r_,</ins> <!--TODO(littledan): Upgrade to ECMASpeak-->


Would be good to get this updated for merge.

guybedford

I'd be glad to merge into the PR if you're ready to go here?

guybedford · 2019-02-17T16:44:28Z

spec.html

+
+    <p>Now consider a case where _A_ has an instantiation error; for example, it tries to import a binding from _C_ that does not exist. In that case, the above steps still occur, including the early return from the second call to InnerModuleInstantiation on _A_. However, once we unwind back to the original InnerModuleInstantiation on _A_, it fails during ModuleDeclarationEnvironmentSetup, namely right after _C_.ResolveExport(). The thrown *SyntaxError* exception propagates up to _A_.Instantiate, which resets all modules that are currently on its _stack_ (these are always exactly the modules that are still `"instantiating"`). Hence both _A_ and _B_ become `"uninstantiated"`. Note that _C_ is left as `"instantiated"`.</p>
+
+    <p>Finally, consider a case where _A_ has an evaluation error; for example, its source code throws an exception. In that case, the evaluation-time analog of the above steps still occurs, including the early return from the second call to InnerModuleEvaluation on _A_. However, once we unwind back to the original InnerModuleEvaluation on _A_, it fails by assumption. The exception thrown propagates up to _A_.Evaluate() <del>, which records the error in all modules that are currently on its _stack_ (i.e., the modules that are still `"evaluating"`)</del><ins>via Promise rejections, which form a chain through the whole dependency graph due to ExecuteModuleWhenImportsReady</ins>. Hence both _A_ and _B_ become `"evaluated"` and the exception is recorded in both _A_ and _B_'s <del>[[EvaluationError]]</del><ins>[[ExecPromise]]</ins> fields, while _C_ is left as `"evaluated"` with <del>no [[EvaluationError]]</del><ins>its [[ExecPromise]] set to a Promise resolved to *undefined*.</p>


I guess we are doing this one as well though.

guybedford · 2019-02-17T16:44:55Z

spec.html

+
+    <p>Consider then cases involving instantiation errors. If InnerModuleInstantiation of _C_ succeeds but, thereafter, fails for _B_, for example because it imports something that _C_ does not provide, then the original _A_.Instantiate() will fail, and both _A_ and _B_'s [[Status]] remain `"uninstantiated"`. _C_'s [[Status]] has become `"instantiated"`, though.</p>
+
+    <p>Finally, consider a case involving evaluation errors. If InnerModuleEvaluation of _C_ succeeds but, thereafter, fails for _B_, for example because _B_ contains code that throws an exception, then the original _A_.Evaluate() will fail<ins>, returning a Promise resolving to *undefined*</ins>. The resulting exception will be recorded in both _A_ and _B_'s <del>[[EvaluationError]]</del><ins>[[ExecPromise]]</ins> fields <ins>as a rejected Promise</ins>, and their [[Status]] will become `"evaluated"`. _C_ will also become `"evaluated"` but, in contrast to _A_ and _B_, will <del>remain without an [[EvaluationError]]</del><ins>have its [[ExecPromise]] internal slot set to a Promise resolved to *undefined*</ins>, as it successfully completed evaluation. Storing the exception <ins>in [[ExecPromise]] as a Promse rejection</ins> ensures that any time a host tries to reuse _A_ or _B_ by calling their Evaluate() method, it will encounter the same exception. (Hosts are not required to reuse Source Text Module Records; similarly, hosts are not required to expose the exception objects thrown by these  methods. However, the specification enables such uses.)</p>


You don't think we could just include this line to avoid noise here?

Sorry, what are you suggesting?

Just that there is a lot of text here from the spec, and it wasn't easy to see the diff. But I suppose that will be clearer in the rendered output.

guybedford · 2019-02-17T16:45:40Z

spec.html

+        1. Perform ! ModuleExecution(_module_, _capability_).
+        1. Return.
+      1. Let _index_ be 0.
+      1. Let _fullfilledCount_ be 0.


Sure, that is understandable. If I recall this pattern already exists in some other async spec code, but I can't recall where exactly.

littledan · 2019-02-17T18:33:59Z

Yeah feel free to merge when you think it's ready. I'd rather we keep using the same PR against top-level await, as your PR represents the change to Variant B, and this PR just edits some details. Thanks for the review!

guybedford · 2019-02-17T18:39:08Z

Amazing, thanks. Updated. What do you think would be suitable next steps to keep this moving forward?

littledan · 2019-02-17T23:07:12Z

I will talk with other TC39 folks about what we can do to get this presented at the March or May meeting (hopefully without me actually giving the presentation).

The purpose of this patch is to nail down how HTML would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready. Top-level await should reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

The purpose of this patch is to nail down how dynamic import would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready, and this will be passed to FinishDynamicImport to wait on before resolving the Promise it returns. This PR is just a sketch, not to be landed. For one, the "Upon fulfillment" language will need to be changed to be more ECMASpeak-like. Top-level await should also reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

The purpose of this patch is to nail down how HTML would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready. Top-level await should reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

The purpose of this patch is to nail down how HTML would adjust to the needs of top-level await. The relevant change is, the module.Evaluate() method will return a Promise that resolves when the module is ready, and this will be passed to FinishDynamicImport to wait on before resolving the Promise it returns. It doesn't seem like anyone else needs to wait for a module script to "finish" evaluating besides dynamic import. This PR is just a sketch, not to be landed. Top-level await should reach stage 3 before this change lands. The PR where Evaluate returns a Promise is guybedford/proposal-top-level-await#1

littledan added 2 commits February 9, 2019 15:52

Revert "Revert "do use awaitall""

a76b778

This reverts commit e4ee72a. The spec-level Promise.all here was well-written! Let's keep it, but consider follow-on editorial tweaks to match ES spec style.

Point to the explainer

8883a90

littledan mentioned this pull request Feb 9, 2019

Variant B promise refactoring tc39/proposal-top-level-await#33

Merged

littledan force-pushed the sibling-execution branch from 96fe3f4 to 1983581 Compare February 10, 2019 01:47

This was referenced Feb 10, 2019

[WIP] Top-level await integration tc39/proposal-dynamic-import#71

Closed

Top-level await integration whatwg/html#4352

Merged

This was referenced Feb 10, 2019

Use of Await in Evaluate() Concrete Method is invalid tc39/proposal-top-level-await#32

Closed

[Spec] figure out how to return/propagate promises properly tc39/proposal-top-level-await#18

Closed

Editorial: Update parts of the spec to refer to [[ExecPromise]]

f0a444d

Closes tc39#17

littledan mentioned this pull request Feb 10, 2019

Promise in Evaluate() Concrete Method makes no sense tc39/proposal-top-level-await#31

Closed

guybedford approved these changes Feb 17, 2019

View reviewed changes

Editorial: Upgrade "upon rejection" to ecmaspeak

8c62b55

guybedford approved these changes Feb 17, 2019

View reviewed changes

guybedford merged commit bca2895 into guybedford:promise-refactoring Feb 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various editorial improvements #1

Various editorial improvements #1

littledan commented Feb 9, 2019

littledan commented Feb 10, 2019

guybedford left a comment

guybedford Feb 17, 2019

littledan Feb 17, 2019

guybedford Feb 17, 2019

littledan Feb 17, 2019

guybedford Feb 17, 2019

guybedford Feb 17, 2019

littledan Feb 17, 2019

guybedford left a comment

guybedford Feb 17, 2019

littledan Feb 17, 2019

guybedford Feb 17, 2019

littledan Feb 17, 2019

guybedford Feb 17, 2019

guybedford Feb 17, 2019

littledan commented Feb 17, 2019

guybedford commented Feb 17, 2019

littledan commented Feb 17, 2019


		<p>Now consider a case where _A_ has an instantiation error; for example, it tries to import a binding from _C_ that does not exist. In that case, the above steps still occur, including the early return from the second call to InnerModuleInstantiation on _A_. However, once we unwind back to the original InnerModuleInstantiation on _A_, it fails during ModuleDeclarationEnvironmentSetup, namely right after _C_.ResolveExport(). The thrown SyntaxError exception propagates up to _A_.Instantiate, which resets all modules that are currently on its _stack_ (these are always exactly the modules that are still `"instantiating"`). Hence both _A_ and _B_ become `"uninstantiated"`. Note that _C_ is left as `"instantiated"`.</p>

		<p>Finally, consider a case where _A_ has an evaluation error; for example, its source code throws an exception. In that case, the evaluation-time analog of the above steps still occurs, including the early return from the second call to InnerModuleEvaluation on _A_. However, once we unwind back to the original InnerModuleEvaluation on _A_, it fails by assumption. The exception thrown propagates up to _A_.Evaluate() <del>, which records the error in all modules that are currently on its _stack_ (i.e., the modules that are still `"evaluating"`)</del><ins>via Promise rejections, which form a chain through the whole dependency graph due to ExecuteModuleWhenImportsReady</ins>. Hence both _A_ and _B_ become `"evaluated"` and the exception is recorded in both _A_ and _B_'s <del>[[EvaluationError]]</del><ins>[[ExecPromise]]</ins> fields, while _C_ is left as `"evaluated"` with <del>no [[EvaluationError]]</del><ins>its [[ExecPromise]] set to a Promise resolved to undefined.</p>


		<p>Consider then cases involving instantiation errors. If InnerModuleInstantiation of _C_ succeeds but, thereafter, fails for _B_, for example because it imports something that _C_ does not provide, then the original _A_.Instantiate() will fail, and both _A_ and _B_'s [[Status]] remain `"uninstantiated"`. _C_'s [[Status]] has become `"instantiated"`, though.</p>

		<p>Finally, consider a case involving evaluation errors. If InnerModuleEvaluation of _C_ succeeds but, thereafter, fails for _B_, for example because _B_ contains code that throws an exception, then the original _A_.Evaluate() will fail<ins>, returning a Promise resolving to undefined</ins>. The resulting exception will be recorded in both _A_ and _B_'s <del>[[EvaluationError]]</del><ins>[[ExecPromise]]</ins> fields <ins>as a rejected Promise</ins>, and their [[Status]] will become `"evaluated"`. _C_ will also become `"evaluated"` but, in contrast to _A_ and _B_, will <del>remain without an [[EvaluationError]]</del><ins>have its [[ExecPromise]] internal slot set to a Promise resolved to undefined</ins>, as it successfully completed evaluation. Storing the exception <ins>in [[ExecPromise]] as a Promse rejection</ins> ensures that any time a host tries to reuse _A_ or _B_ by calling their Evaluate() method, it will encounter the same exception. (Hosts are not required to reuse Source Text Module Records; similarly, hosts are not required to expose the exception objects thrown by these methods. However, the specification enables such uses.)</p>

Various editorial improvements #1

Various editorial improvements #1

Conversation

littledan commented Feb 9, 2019

littledan commented Feb 10, 2019

guybedford left a 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

guybedford left a 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 commented Feb 17, 2019

guybedford commented Feb 17, 2019

littledan commented Feb 17, 2019