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

feat(es/transforms/compat): Add loose mode to parameters #2911

Merged
merged 4 commits into from
Dec 2, 2021

Conversation

Austaras
Copy link
Member

@Austaras Austaras commented Nov 29, 2021

closes #2825, closes #2800

@Austaras
Copy link
Member Author

I would need some help @kdy1
The test case of issue_2811 is failing because it inject _this to wrong scope, so the result will be something like

var f = function () { ... } 
if (true) {
} else {
   var _this = this
}

Why is this happening? I guess it's because of Parallel, how does that work and how should I fix it?

@kdy1
Copy link
Member

kdy1 commented Nov 30, 2021

#[parallel] only works if the number of statements (at the same level) is more than 64, so I don't think it's the case.

I think visit_mut_stmts should store some 'shared' variables like self.vars to somewhere, because otherwise variables from different statements will be injected by visit_mut_stmts.

(That's why I mentioned creating a new visitor can be better than storing variables)

@kdy1
Copy link
Member

kdy1 commented Nov 30, 2021

var f = function () { ... } 
if (true) {
} else {
   var _this = this
}

for this case, visit_mut_stmts for the top level generates variable, and visit_mut_stmts for

{
   var _this = this
}

takes the variables, just as those variables are generated by the nested block.

@Austaras Austaras force-pushed the main branch 4 times, most recently from 07b6847 to 9f8cd45 Compare November 30, 2021 17:59
@kdy1 kdy1 added this to the v1.2.116 milestone Dec 1, 2021
@Austaras
Copy link
Member Author

Austaras commented Dec 1, 2021

But parallel doesn't allow a custom visit_mut_stmts, what should I do?

@kdy1
Copy link
Member

kdy1 commented Dec 1, 2021

You can remove #[parallel] for now. I'll modify #[parallel] in future.

@Austaras Austaras force-pushed the main branch 2 times, most recently from 41add7b to ab5d8de Compare December 1, 2021 10:16
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! Almost LGTM.

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!
I'm bit worried because circular dependency (in dev-dependencies) may block publising, but let's try.

@kdy1 kdy1 enabled auto-merge (squash) December 2, 2021 00:36
@kdy1 kdy1 merged commit 1555ceb into swc-project:main Dec 2, 2021
dsherret pushed a commit to dsherret/swc that referenced this pull request Dec 2, 2021
…ct#2911)

swc_ecma_utils:
 - Make `WrapperState` implement `Clone`.

swc_ecma_transforms_compat:
 - `paramters`: Fix handling of non-loose mode. (Closes swc-project#2800, Closes swc-project#2825)
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants