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

fix(strip): Combine typescript_class_properties() into strip() #1478

Merged

Conversation

nayeemrmn
Copy link
Contributor

@kdy1 This implements use_define_for_class_fields: true as correctly specified in #1472. It also adds the behaviour of the typescript_class_properties() pass directly into strip() in case of use_define_for_class_fields: false. This aligns with tsc. The typescript_class_properties() is redundant and should be removed.

However, I can't figure out how to copy the static class field handling from typescript_class_fields() to strip(). So I left some TODOs and haven't removed it yet. Help would be appreciated.

Copy link
Contributor Author

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

TODOs for static class field handling when use_define_for_class_fields: false.

Comment on lines 223 to 225
// TODO(nayeemrmn): Static fields should also be moved to
// assignments after the class. Figure out how. They are
// preserved for now.
Copy link
Contributor Author

@nayeemrmn nayeemrmn Mar 19, 2021

Choose a reason for hiding this comment

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

I need to be able to move the static class fields here to assignments after the class.

Comment on lines +751 to +752
// TODO(nayeemrmn): This output should be achieved without
// `typescript_class_properties()`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test.

@nayeemrmn nayeemrmn marked this pull request as draft March 20, 2021 03:43
@nayeemrmn nayeemrmn marked this pull request as ready for review March 20, 2021 05:21
@kdy1
Copy link
Member

kdy1 commented Mar 21, 2021

Sorry for late response. I was too busy because of other projects.

The changes seems ok to me. But I have a question.
Doesn't this change the default behavior of swc even when target < es2020 ?

@nayeemrmn
Copy link
Contributor Author

Doesn't this change the default behavior of swc even when target < es2020 ?

Not totally sure what you mean by "when target < es2020", but any change in behaviour is non-breaking and aligns with tsc. The behaviour of chain!(strip(), typescript_class_properties()) should be exactly the same.

@kdy1
Copy link
Member

kdy1 commented Mar 21, 2021

@nayeemrmn Thank you very much for the pr and clarification :)

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.

LGTM.
I'll merge it today.

@kdy1 kdy1 merged commit 9bc074e into swc-project:master Mar 21, 2021
@kdy1 kdy1 added this to the v1.2.51 milestone Mar 21, 2021
@kdy1 kdy1 self-assigned this Mar 21, 2021
@nayeemrmn nayeemrmn deleted the typescript-class-properties-into-strip branch March 21, 2021 14:46
kdy1 added a commit to kdy1/swc that referenced this pull request Mar 31, 2021
…roject#1478)

swc_ecma_utils:
 - Use `&mut` for inject_after_super.

swc_ecma_transforms_typescript:
 - Merge `typescript_class_properties` into `strip`.

Co-authored-by: 강동윤 <kdy1997.dev@gmail.com>
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants