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.

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

Stage 3 reviews #77

Closed
littledan opened this issue May 20, 2019 · 20 comments

Comments

Projects
None yet
9 participants
@littledan
Copy link
Member

commented May 20, 2019

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:

Editors:

@littledan

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

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 .

@waldemarhorwat

This comment has been minimized.

Copy link

commented May 28, 2019

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.

@waldemarhorwat

This comment has been minimized.

Copy link

commented May 28, 2019

Once you fix the note, you can mark this as reviewed by me.

@GeorgNeis

This comment has been minimized.

Copy link

commented May 29, 2019

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.

@waldemarhorwat

This comment has been minimized.

Copy link

commented May 29, 2019

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

@littledan

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@waldemarhorwat @GeorgNeis I've tried to fix these issues at #83 .

@zenparsing

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

👍

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.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

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 .

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

(Don't forget to update instances of instantiate to link)

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Why is [[PendingAsyncDependencies]] an integer, instead of something more explicit (like something that describes the async dependency modules)?

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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.

ljharb added a commit to ljharb/proposal-top-level-await that referenced this issue Jun 5, 2019

add missing serial commas
(re tc39#77)

also add a missing backtick.
@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

My editor review is done; once these 5 comments, plus #106, are resolved, consider me signed off.

@guybedford

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

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.

@robpalme

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Given #105 fix has landed, please consider my review signed off.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@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

@littledan

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

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?

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Oh sure, not a stage 3 blocker, just seems more clear what it’s tracking.

ljharb added a commit to ljharb/proposal-top-level-await that referenced this issue Jun 6, 2019

Editorial: add missing serial commas
(re tc39#77)

also add a missing backtick.

littledan added a commit that referenced this issue Jun 6, 2019

Editorial: add missing serial commas
(re #77)

also add a missing backtick.
@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I guess this can be closed now?

@MylesBorins MylesBorins closed this Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.