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(es/compat): Handle useDefineForClassFields: false #7055

Merged
merged 19 commits into from
Mar 12, 2023

Conversation

magic-akari
Copy link
Member

@magic-akari magic-akari commented Mar 10, 2023

Description:

BREAKING CHANGE:

IMPORTANT NOTE: Users of decorators are recommended to configure "useDefineForClassFields": false to ensure that your code is properly transpiled.

Related issue (if exists):

@magic-akari
Copy link
Member Author

/// Align the semantics of TS class fields with TC39 class fields. Defaults
/// to `false`.
///
/// See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier.
///
/// When running `tsc` with configuration `"target": "ESNext",
/// "useDefineForClassFields": true`, TS class fields are preserved as JS
/// class fields. We target ESNext, so this our behavior with
/// `use_define_for_class_fields: true`.
///
/// When running `tsc` with configuration `"target": "<ES6-ES2020>",
/// "useDefineForClassFields": true`, TS class fields are transformed to
/// `Object.defineProperty()` statements. You must additionally apply the
/// [swc_ecma_transforms_compat::es2022::class_properties()] pass to
/// get this backward-compatible output.
#[serde(default)]
pub use_define_for_class_fields: bool,

#[serde(default)]
pub use_define_for_class_fields: BoolConfig<false>,
}

Currently, the default value for use_define_for_class_fields is false. Should we change it?

@kdy1
Copy link
Member

kdy1 commented Mar 11, 2023

I think so. Thank you!

@magic-akari
Copy link
Member Author

I think so. Thank you!

useDefineForClassFields true false None
ES2022 Native [[Set]] Native
<= ES2021(TS) [[Define]] [[Set]] [[Define]]
<= ES2021(JS) [[Define]] [[Set]] [[Define]]

So, we choose the Define semantics when there is no input value from the user, right?
This may deviate from TS behavior, however, it ensures consistency in the final column of our table.

@kdy1
Copy link
Member

kdy1 commented Mar 11, 2023

Yes, I think it's more important to be consistent than to align with tsc

@magic-akari
Copy link
Member Author

It appears that microsoft/TypeScript#45995 is a bug in TypeScript that occurs when useDefineForClassFields is absent.

When its value is present, the order of the TS constructor parameter properties always precedes the ECMAScript class field for the four cases of ES Version and useDefineForClassFields: true/false.

Please refer to the following table for more detailed information.

useDefineForClassFields true false
ES2022 TS Playground TS Playground
<= ES2021 TS Playground TS Playground

@magic-akari magic-akari changed the title fix(es/compat): Handle useDefineForClassFields: false in ecmascript fix(es/compat): Handle useDefineForClassFields: false Mar 11, 2023
@kdy1
Copy link
Member

kdy1 commented Mar 11, 2023

🤣 It was a bug of tsc. What should we do, in your opinion?

@magic-akari
Copy link
Member Author

The microsoft/TypeScript#45995 issue arises only when useDefineForClassFields is missing. However, if it is explicitly set to either true or false, then TS's constructor parameter properties consistently appear first. Hence, we must adhere to TS's behavior.

Moreover, there is another issue in TS where the initialization of constructor parameter properties does not always take effect.

Check the following code.

TS Playground

// @target: es2022
// @useDefineForClassFields: true
class Foo {
  b = this.a;
  constructor(public a = 1){

  }
}

output:

class Foo {
    a;
    b = this.a;
    constructor(a = 1) {
        this.a = a;
    }
}

The value obtained by b is undefined. However, if we reduce the ES version to below ES2022,

TS Playground

class Foo {
    constructor(a = 1) {
        Object.defineProperty(this, "a", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: a
        });
        Object.defineProperty(this, "b", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: this.a
        });
    }
}

B will get the correct value. I believe that such a situation is more reasonable. Therefore, for the scenarios above ES2022, I have made an improvement. SWC will output the following code to ensure correctness.

class Foo {
    a;
    b;
    constructor(a = 1) {
        this.a = a;
        this.b = this.a;
    }
}

@kdy1
Copy link
Member

kdy1 commented Mar 11, 2023

Sounds good, thank you so much for your hard work.

@magic-akari magic-akari marked this pull request as ready for review March 11, 2023 15:14
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!


swc-bump:

  • swc_ecma_ast
  • swc --breaking

@kdy1 kdy1 enabled auto-merge (squash) March 12, 2023 03:21
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

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

Automated review comment generated by auto-rebase script

@kdy1 kdy1 disabled auto-merge March 12, 2023 03:55
@kdy1 kdy1 merged commit bb6dde7 into swc-project:main Mar 12, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.41 Mar 16, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Apr 16, 2023
@magic-akari magic-akari deleted the fix/issue-6985 branch September 18, 2023 08:42
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.

Inconsistent useDefineForClassFields behavior
3 participants