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
add async-explicit-resource-management #300
add async-explicit-resource-management #300
Conversation
```js | ||
extend interface VariableDeclaration { | ||
kind: "usingAwait"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative design is
extend interface VariableDeclaration {
await: boolean;
}
We have the same design in ForOfStatement
. However, in this case, the other kinds, namely var
, let
and const
do not have await counterparts. So I figure it would be more efficient to pack the await
flag into the kind
string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the correct course of action given that constraint.
Would we also have a kind: "using"
for sync disposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we already have https://github.com/estree/estree/blob/master/stage3/explicit-resource-management.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I think my only question then is, in the final version, do we want kind: "using await"
instead of kind: "usingAwait"
to match the actual syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly prefer usingAwait
or asyncUsing
if other syntax is adopted, because "camelCase" is slightly shorter than "camel case". I think we can trust TC39 that usingAwait variable
will never be approved alongside the using await variable
, so either kind: "using await"
or kind: "usingAwait"
should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly more in favor of kind: "using await"
to have it more closely match the actual syntax. I was also looking through the spec, and it seems like we don't have other instances of string values in camel case. Though, to be fair, we also don't have other instances of string values with spaces, so that's probably not a great argument. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think I’m more in favor of “using await” to match actual syntax. This is important for code generators that take in the AST and output source code. If we use “usingAwait”, every code generator will need to be updated to catch this special case. With “using await”, no changes will be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong preference. Either "using await"
or "usingAwait"
is fine. I am waiting for resolution of the next meeting, since we may have to change that again if "async using"
, after that I will go with your approach.
Speaking of code generator, if they don't handle edge cases like using /* feeling lucky */ await foo = ...
, they can proceed with printing node.kind
as-is.
Converting to a draft pending discussions on the syntax |
Per babel/proposals#87 (comment), this PR is ready for review. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
View Rendered Text
This PR adds Async Explicit Resource Management support.