Narrow the token to async when checking default exports #648
Conversation
|
@alangpierce Gentle ping! This is a really nice unblocker for us to get our ESM support in place. Thank you. |
|
It would be great to get this one looked at! Thanks |
There was a problem hiding this comment.
Thanks! Unfortunately, this fix ends up introducing a bug in the regular export default async function case, but I think I have ideas on how to fix.
If you look at the the tokens for regular export default async function foo() {}, that async is actually a "contextual keyword" token: one of type tt.name with the raw text "async" and contextual keyword 4 (ContextualKeyword._async): https://sucrase.io/#compareWithBabel=false&showTokens=true&code=%0Aexport%20default%20async%20function%20foo()%20%7B%7D
The whole "contextual keyword" concept is a nuance in JS where some words can be a variable name sometimes and a keyword other times, and the Acorn and Babel parsers (from which Sucrase is forked) have the distinction as well. Unfortunately, async is a bit inconsistent right now in that it's a contextual keyword in most contexts but sometimes a regular keyword, which is probably something that could be cleaned up in the long run. In the meantime, probably best to think of it as a contextual keyword for the purposes of this PR.
In terms of impact, the PR right now breaks recognition of the export default async function case, which makes it so this code:
export default async function blah() {}
blah();transpiles to this:
exports. default = async function blah() {}
blah();when it should transpile to this:
async function blah() {} exports.default = blah;
blah();The syntax export default async function blah() {} is supposed to declare the name blah for use elsewhere in the file, so the PR as-is makes the code crash due to blah no longer being a valid name.
I think the best fix here is to make the check more specific using a variant of this.tokens.matchesContextual. For example, you could do something like:
... || (
this.tokens.matches5(...) &&
this.tokens.matchesContextualAtIndex(this.tokens.currentIndex() + 2, ContextualKeyword._async)
)
(There's probably room for improvement in this tokens API, but seems fine to call that out of scope here.)
To get this working, it would be great to see a few updates here:
- Add a test for the
export default async function blah() {}case. Sorry, that was missing coverage before. - Change the code here to use some variant of
matchesContextualin addition to the check that it's already doing. - I think it would be nice to update the
// export async functionline in this file similarly and also the// export default async functionline inESMImportTransformer.ts, but I won't consider that blocking since those are separate issues.
I also see that there's a pre-existing CI failure with the babel example project in the test suite, so I'll try to get that fixed.
Signed-off-by: blam <ben@blam.sh>
|
OK 🤞 Added some more tests to cover some of the other bits too like the |
Benchmark resultsBefore this PR: 242.2 thousand lines per second Measured change: 0.6% slower (1.19% slower to 1.34% faster) |
Codecov Report
@@ Coverage Diff @@
## main #648 +/- ##
==========================================
+ Coverage 82.58% 82.65% +0.07%
==========================================
Files 56 56
Lines 5690 5928 +238
Branches 1344 1344
==========================================
+ Hits 4699 4900 +201
- Misses 708 745 +37
Partials 283 283
Continue to review full report at Codecov.
|
|
Looks like npm authentication is down right now, but I'll publish when it's back up. |
|
Released as 3.20.2 |
Needed to use
tt._asynchere to make sure that we check if it's an async function in default exports.Fixes #543