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

strip: Class fields should be assigned after parameter properties #930

Closed
nayeemrmn opened this issue Aug 3, 2020 · 7 comments · Fixed by #949
Closed

strip: Class fields should be assigned after parameter properties #930

nayeemrmn opened this issue Aug 3, 2020 · 7 comments · Fixed by #949
Assignees
Labels
Milestone

Comments

@nayeemrmn
Copy link
Contributor

nayeemrmn commented Aug 3, 2020

Input code

TypeScript allows regular class fields to depend on parameter properties:

class A {
  b = this.a;
  constructor(public a: any) {}
}

Output code (actual)
The regular class field b is incorrectly initialised before the parameter property a:

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

Output code (expected)
The correct ordering is, parameter property assignment -> class field initialisation -> rest of the constructor. So if there are parameter properties present (or always), the class field initialisation must be moved below their assignments in the constructor:

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

This is the official compiler's output.


Seen in https://github.com/denoland/deno/blob/v1.2.2/std/mime/multipart.ts#L263-L271

cc @bartlomieju

@kdy1
Copy link
Member

kdy1 commented Aug 4, 2020

This is handled by class properties pass. Note that class properties pass inject some helpers and you need InjectHelper pass. (I don't know why. I just looked at the output from the babel and write code to emit same output.)

If it's not acceptable, I can modify typescript pass to do it or class properties pass not to inject helper

@Brooooooklyn
Copy link
Member

Seems like TypeScript default behavior is different between the ECMA class field proposal: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier.

So I think:

I can modify typescript pass to do it

is a better way to handler this situation. And because class properties need to be handled with parameter properties

@kdy1
Copy link
Member

kdy1 commented Aug 5, 2020

I think modifying class_properties pass to accept mode is better than modifying typescript_strip pass because typescript also supports useDefineForClassFields . If I modify typescript_strip pass, I have to do same work twice

@kdy1 kdy1 modified the milestones: v1.2.12, v1.2.13, v1.2.14, v1.2.15 Aug 6, 2020
@kdy1 kdy1 self-assigned this Aug 9, 2020
@kdy1 kdy1 mentioned this issue Aug 9, 2020
kdy1 added a commit to kdy1/swc that referenced this issue Aug 9, 2020
kdy1 added a commit to kdy1/swc that referenced this issue Aug 9, 2020
@kdy1 kdy1 closed this as completed in #949 Aug 9, 2020
kdy1 added a commit that referenced this issue Aug 9, 2020
swc_ecma_parser:
 - Allow `in` in class properties (#944)
 - Make `delete` with optional chaining valid (#947)

swc_ecma_transforms:
 - Add a `typescript_class_properties` pass (#930)
@kdy1
Copy link
Member

kdy1 commented Aug 9, 2020

Now there exists a typescript_class_properties pass for it.

@nayeemrmn
Copy link
Contributor Author

nayeemrmn commented Aug 21, 2020

@kdy1 I'm still seeing this issue in deno cache --no-check with Deno on master. Has something not been updated or configured properly? https://github.com/denoland/deno/blob/v1.3.1/Cargo.lock

@bartlomieju
Copy link
Contributor

@nayeemrmn I forgot to add typescript_class_properties to our setup 🤦

@swc-bot
Copy link
Collaborator

swc-bot commented Oct 27, 2022

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

5 participants