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: unify class properties transform in ts and js #3766

Merged
merged 17 commits into from
Mar 5, 2022

Conversation

Austaras
Copy link
Member

@Austaras Austaras commented Feb 27, 2022

Description:
And also expose an use_define_for_class_fields somewhere

BREAKING CHANGE:
It will break rust users

It won't be possible to use [[Set]] for pulibc fields and preserve private fields.

It will be a break change for ts users who want es2022/esnext output, but this is aligned with babel behaviour. And consider tsc doesn't have target env and would set useDefineForPublicFields to true in es2022, this shall also be aligned with tsc.

Related issue (if exists):
closes #3827, closes #3389, closes #3317, closes #3228, supersedes #3764

@Austaras
Copy link
Member Author

One major difference would be class exprs where ts transform uses a sequence expr while js transform uses a iife.

This should be fixed.

@Austaras Austaras force-pushed the main branch 5 times, most recently from 60f00e9 to 7f34729 Compare March 1, 2022 07:11
@Austaras
Copy link
Member Author

Austaras commented Mar 2, 2022

Ah, I've encountered with microsoft/TypeScript#45995

@Austaras Austaras force-pushed the main branch 2 times, most recently from 78e1ac2 to 0e740e9 Compare March 3, 2022 09:39
@Austaras
Copy link
Member Author

Austaras commented Mar 3, 2022

Here's some observerations about initialized order of parameter properties:

  1. tsc would init parameter properties before normal properties in target before es2022, however in target es2022 and esnext tsc would preserve class properties, so noraml ones would be initialized first. As tsc doesn't follow semantic versioning, this may soon changes.
  2. babel simply transform parameter properties into assignments and preserve class normal properties, so in any case normal properties would be initialized first
  3. esbuild would transform parameter and noraml properties into assignments so the initialized order is preserved, but it doesn't support useDefineForClassFields

I originally would like to choose route 3, but consider private properties

class A {
  a = 123
  #a = 456
  constructor(public b) {
    console.log(this.#a)
  }
}

a cannot be simply transformed into assignments, because it needs to be initialized before #a, but only if we bring whole class_properties in(and surprisingly, this is not impossible!), private properties could not be safely transformed. So now I choose to turn it into

class A {
 a
  constructor(b) {
    this.b = b
    this.a = 123
    this.#a = 456
    console.log(this.#a)
  }
}

This would generate many useless boilerplate, but at least it's consisent and doesn't break current tests. Hopefully not many people would use private properties in TypeScript.

@kdy1
Copy link
Member

kdy1 commented Mar 3, 2022

Hmm... Yeah I think your decision is right although I'm a bit confused by other tools.

@Austaras
Copy link
Member Author

Austaras commented Mar 3, 2022

Oh, and ts has same bug I fixed in previous pr

@Austaras Austaras force-pushed the main branch 7 times, most recently from d9b45e7 to d2f44d2 Compare March 4, 2022 07:57
@Austaras Austaras marked this pull request as ready for review March 4, 2022 07:57
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.

I didn't review fully yet.
I will do it soon, but I think I found a bug

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 so much!


swc-bump:

  • swc_ecma_transforms_base --breaking

@@ -1,3 +1,3 @@
class C {
}
console.log(C.f1, C.f2, C.f3), console.log(C.f1, C.f2, C.f3), C.f1 = 1, C.f2 = 2, C.f3 = 3;
C.f1 = 1, console.log(C.f1, C.f2, C.f3), C.f2 = 2, console.log(C.f1, C.f2, C.f3), C.f3 = 3;
Copy link
Member

Choose a reason for hiding this comment

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

const a = 1;

class C {
    static f1 = 1;

    static {
        console.log(C.f1, C.f2, C.f3)
    }

    static f2 = 2;

    static {
        console.log(C.f1, C.f2, C.f3)
    }

    static f3 = 3;
}

and output of original file is

1 undefined undefined
1 2 undefined

Copy link
Member

Choose a reason for hiding this comment

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

Okay, it's correct.
Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the original output is wrong

@kdy1 kdy1 enabled auto-merge (squash) March 5, 2022 06:27
@kdy1 kdy1 added this to the v1.2.149 milestone Mar 5, 2022
@kdy1 kdy1 disabled auto-merge March 5, 2022 06:37
@kdy1 kdy1 enabled auto-merge (squash) March 5, 2022 06:37
@kdy1 kdy1 merged commit 6f076e4 into swc-project:main Mar 5, 2022
@swc-project swc-project locked and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants