Stage 3 reviews
littledan opened this issue · 20 comments
For the June 2019 meeting, top-level await may be proposed for Stage 3. For this, we need reviews. There are no more planned changes for the specification, so now's a great time to do a review!
Reviewers:
- @GeorgNeis (or @adamk)
- @waldemarhorwat (for the syntax)
- @robpalme (didn't sign up at the meeting, but later agreed to review)
Editors:
- @zenparsing (who specifically signed up to review)
- @ljharb
Note, the big change since last meeting was following up on @robpalme 's and @guybedford 's suggestion to make dependencies of asynchronous modules be invoked synchronously, not through Promise reactions. See more details about the new algorithm in #74, and motivation in #69 .
The grammar itself is fine, but there is a bug in one this proposal's notes that I mentioned during the March meeting:
await is parsed as an AwaitExpression when the [Await] parameter is present. The [Await] parameter is present in the following contexts:
...
When Module is the syntactic goal symbol
Reading that note literally, [Await] would be present even inside a regular function body if Module is the syntactic goal symbol, which is not correct. The fix is to reword the note to make it clear that this only applies in contexts outside things like inner functions.
Once you fix the note, you can mark this as reviewed by me.
I think the grammar for ExportDeclaration needs to be changed to allow await, otherwise the motivating example export const output = process((await dynamic).default, await data); is invalid.
@GeorgNeis: You're right. There are a bunch places in the export productions which have ~Await grammar parameters that should be changed if we want to permit those motivating examples. We might want to think about what the ImportedBinding production's [~Await] should be as well: do we want to allow "await" as an identifier there?
@waldemarhorwat @GeorgNeis I've tried to fix these issues at #83 .
👍
The direction of the current text looks good to me. I'll hold off on a more detailed review until the refactoring changes have landed.
We're not planning on landing any more changes at this point. The text is ready for detailed review.
Edit: The PRs out for review won't land soon. But a bug fix landed at #105 .
https://tc39.github.io/proposal-top-level-await/#table-36 lists 4 slots, and the following editor's note mentions a [[EvaluationPromise]] slot that no longer seems to exist.
(Don't forget to update instances of instantiate to link)
Why is [[PendingAsyncDependencies]] an integer, instead of something more explicit (like something that describes the async dependency modules)?
https://tc39.github.io/proposal-top-level-await/#sec-source-text-module-record-execute-module lacks an assertion that capability, when provided, is a PromiseCapability Record.
Similarly, https://tc39.github.io/proposal-top-level-await/#sec-asyncblockstart lacks any assertions about capability, specifically that it's a PromiseCapability Record.
Typically, we don't create functions with inline "steps" as in step 8 of https://tc39.github.io/proposal-top-level-await/#sec-toplevelmoduleevaluationjob - they're a separate block, like https://tc39.github.io/ecma262/#sec-promise-reject-functions or https://tc39.github.io/ecma262/#sec-promise-resolve-functions. Probably best to keep to that format.
My editor review is done; once these 5 comments, plus #106, are resolved, consider me signed off.
Many thanks @ljharb for the review here. I've posted a PR with the initial work on this feedback at #107. To provide feedback on the other point:
Why is [[PendingAsyncDependencies]] an integer, instead of something more explicit (like something that describes the async dependency modules)?
This integer effectively forms the counter to indicate the ready event of the dependencies. This is a fairly standard approach for waiting on multiple events before firing the next event, as we do with the resolutions of async dependencies having to all complete before executing the module itself.
@guybedford sure, but surely a list of module records would also work? it's all spec fiction, so it seems like we should err on the side of legibility rather than efficiency
I'm not sure what a list would give us, given that we never inspect the contents, just the length. But anyway, can we draw a conclusion on these editorial questions during Stage 3?
Oh sure, not a stage 3 blocker, just seems more clear what it’s tracking.
I guess this can be closed now?