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.

Sign up for GitHub

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

Jump to bottom

fix(es/transforms/compat): apply regenerator transform for default expr #2681

Merged
merged 3 commits into from
Nov 9, 2021
Merged

fix(es/transforms/compat): apply regenerator transform for default expr #2681

merged 3 commits into from
Nov 9, 2021

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Nov 8, 2021

PR updates transform to check defaultexportdecl and fold if there's fn expression to transpile default exported async fn.

One thing bit unsure is unlike other fn's FnDecl, if it's ExportDefaultDecl fn is set to FnExpr instead. PR doesn't touches those node but only touches transform instead for now.

@kwonoj kwonoj force-pushed the fix-async-default-fn-regenerator branch from 83964c5 to c7764b3 Compare November 8, 2021 08:53
@kwonoj
Copy link
Member Author

kwonoj commented Nov 8, 2021

Thinking this more, I'm curious if there's specific reason it has to be FnExpr for the default exported fn: it looks like babel treats it as declaration regardless of async or not for the default export: https://astexplorer.net/#/gist/4e88e71b8c3f7e7094dc5a2bc5085a36/666b57be5110383e94f552dbd2e0f976b003251e

@kdy1
Copy link
Member

kdy1 commented Nov 8, 2021

Thinking this more, I'm curious if there's specific reason it has to be FnExpr for the default exported fn: it looks like babel treats it as declaration regardless of async or not for the default export: >https://astexplorer.net/#/gist/4e88e71b8c3f7e7094dc5a2bc5085a36/666b57be5110383e94f552dbd2e0f976b003251e

Identifier of default function declaration is optional, while identifier of FnDecl is not optional.

@kwonoj
Copy link
Member Author

kwonoj commented Nov 8, 2021

Got it. thoughts on current approach?

@kwonoj kwonoj force-pushed the fix-async-default-fn-regenerator branch from c7764b3 to 286e00d Compare November 8, 2021 16:41
kdy1
kdy1 requested changes Nov 9, 2021
View reviewed changes
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I found some small performance issues.

ecmascript/transforms/compat/src/es2017/async_to_generator.rs Outdated
// if fn is ExportDefaultDecl, fn is not FnDecl but FnExpr
ModuleItem::ModuleDecl(ModuleDecl::ExportDefaultDecl(export_default)) => {
if let DefaultDecl::Fn(fn_expr) = export_default.decl {
let expr = fn_expr.fold_children_with(self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this double-fold?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, updated.

ecmascript/transforms/compat/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
@kwonoj kwonoj force-pushed the fix-async-default-fn-regenerator branch from 286e00d to 7e1f7ab Compare November 9, 2021 06:11
@kwonoj kwonoj force-pushed the fix-async-default-fn-regenerator branch from 7e1f7ab to 95f0f06 Compare November 9, 2021 06:26
@kwonoj kwonoj force-pushed the fix-async-default-fn-regenerator branch from 95f0f06 to 85a4373 Compare November 9, 2021 06:35
kdy1
kdy1 approved these changes Nov 9, 2021
View reviewed changes
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
LGTM.

@kdy1 kdy1 added this to the v1.2.108 milestone Nov 9, 2021
@kdy1 kdy1 enabled auto-merge (squash) November 9, 2021 06:50
@kdy1 kdy1 merged commit 8fe0d25 into swc-project:master Nov 9, 2021
@kwonoj kwonoj deleted the fix-async-default-fn-regenerator branch November 9, 2021 07:45
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kdy1 kdy1 kdy1 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v1.2.108
Development

Successfully merging this pull request may close these issues.

default exported async fn does not transpiled into generator
2 participants
@kwonoj @kdy1