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
Allow await when it is not in AsyncArrowHead #11148
Allow await when it is not in AsyncArrowHead #11148
Conversation
eda041d
to
19aa381
Compare
19aa381
to
99c07ae
Compare
@@ -59,6 +59,7 @@ export default class State { | |||
// Flags to track | |||
inParameters: boolean = false; | |||
maybeInArrowParameters: boolean = false; | |||
maybeInAsyncArrowHead: boolean = false; |
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.
It's not clear to me what the difference between these two flags should be. Isn't the arrow head its parameters?
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.
The maybeInAsyncArrowHead
lives across function declarations.
async (foo = function (await) {}) => {}
When parsing await
, maybeInAsyncArrowHead
is true
but maybeInArrowParameters
is false
.
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.
We should add a comment in the code explaining it 👍
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.
@nicolo-ribaudo Done. Let me know if we can make it better.
0964918
to
bc35c6a
Compare
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.
The CI failures don't seem to be related to this PR.
if (state.maybeAsyncArrow) { | ||
this.state.maybeInAsyncArrowHead = state.maybeAsyncArrow; | ||
} |
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.
Nit: inside the if
state.maybeAsyncArrow
is always true
.
if (state.maybeAsyncArrow) { | |
this.state.maybeInAsyncArrowHead = state.maybeAsyncArrow; | |
} | |
if (state.maybeAsyncArrow) this.state.maybeInAsyncArrowHead = true; |
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.
@nicolo-ribaudo Ah, yeah. Just updated the PR.
bc35c6a
to
2d07de2
Compare
2d07de2
to
cc7cc42
Compare
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.
Thanks!
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.
Await parsing is tricky. We should definitely revisit it later. @arku Thanks!
This PR fixes the parser error thrown for the input
(await) => {}
. I tried to fix the other cases mentioned in the issue description and after struggling with delaying the await position check in case of async call expressions, I reached out to @JLHwung for help. He added that the delay check is indeed difficult and it is best to refactor the await position tracking with a scope based recording mechanism that is non-trivial.