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

useDefineForClassFields defaults to true in 4.2.4 #45584

Closed
KeithHenry opened this issue Aug 26, 2021 · 13 comments
Closed

useDefineForClassFields defaults to true in 4.2.4 #45584

KeithHenry opened this issue Aug 26, 2021 · 13 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@KeithHenry
Copy link

KeithHenry commented Aug 26, 2021

Bug Report

πŸ”Ž Search Terms

useDefineForClassFields

πŸ•— Version & Regression Information

  • This changed sometime between versions 3.7 and 4.2.4
  • Upgrading the version of Typescript in Visual Studio 2019 introduced this issue

⏯ Playground Link

Same code compiles in VS Code and command lines, it breaks in Visual Studio 2019+Typescript 4.2.4 only

πŸ’» Code

Use a @decorator on class properties or any code that is incompatible with useDefineForClassFields: true

Something like:

class MyClass {
    @myDecorator()
    myField: any;
}

Do not include useDefineForClassFields in your tsconfig.json at all.

Run Typescript compile.

When VS2019 with TS 4.2.4 compiles JS output includes class fields, something like:

var __decorate = // ...autogenerated implementation

let MyClass = class MyClass {
    myField;
};

__decorate([
    myDecorator()
], MyClass.prototype, "myField", null);

When tsc 4.3.5 compiles for the same tsconfig and source files it doesn't include class fields, something like:

var __decorate = // ...autogenerated implementation

let MyClass = class MyClass {
    // No fields here
};

__decorate([
    myDecorator()
], MyClass.prototype, "myField", null);

Adding useDefineForClassFields: false works around the issue and makes the JS output consistent.

Property decorators are ignored (as per #35081).

πŸ™ Actual behaviour

JS output includes class fields (as if useDefineForClassFields was set to true) in VS2019+TS 4.2.4
JS output does not include class fields (as if useDefineForClassFields was set to false) in tsc 3.7 or 4.3

πŸ™‚ Expected behaviour

JS output should not include class fields unless useDefineForClassFields is explicitly set to true.

Whatever default useDefineForClassFields setting is should be consistent between tsc and VS2019+TS

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 26, 2021

The default of useDefineForClassFields depends on your target. For ESNext or ES2020 and above it defaults to true, which is the desired behaviour (to align with the ECMAScript standard).

Same code compiles in VS Code and command lines, it breaks in Visual Studio 2019+Typescript 4.2.4 only

This is very unlikely. You either use different versions and/or different TypeScript settings. The default for target is ES3, for which useDefineForClassFields defaults to false.

@KeithHenry
Copy link
Author

@MartinJohns target is the same as tsconfig.json and all TS source is the same in all environments. The setting is "target": "esnext".

VS Code running 4.3.5 with "target": "esnext" and useDefineForClassFields missing does not emit class fields.

VS2019 running 4.2.4 with the exact same settings and code does emit class fields.

Previous versions of Typescript in both VS Code and VS2019 consistently did not emit class fields, also with the same settings.

Setting "useDefineForClassFields": false makes the output consistent.

Whatever default of useDefineForClassFields is (though it should error with experimentalDecorators if it is intentionally incompatible with them) it should be consistent between TS versions or listed as a breaking change.

@andrewbranch
Copy link
Member

Please show a repro in the playground or with tsc. Talking about compiling with VS / VS Code leaves too much room for confusion. I just double checked with tsc and everything looks expected.

@andrewbranch andrewbranch added the Question An issue which isn't directly actionable in code label Aug 26, 2021
@IllusionMH
Copy link
Contributor

There is issue that (I guess) should track docs for this change - #45076

@andrewbranch
Copy link
Member

@IllusionMH ah, thank you! I bet that’s what @KeithHenry is talking about. The examples I tested all had initializers. It’s a little hard to repro something without a code snippet πŸ˜…

@KeithHenry
Copy link
Author

@andrewbranch it works just fine in a playground or tsc on the command line.

The bug appears to only be in the weird embedded version of TypeScript that Visual Studio runs.

@IllusionMH Part of what made this so hard for my team was that tsc on the command line worked with the same tsconfig file, so whenever we tried to isolate the TS build step from VS the problem just went away. It works just fine without useDefineForClassFields set at all, even when "target": "esnext" and on 4.3.5

Is #42663 working right? Is this actually a bug in 4.3? It was the inconsistency that made this hard to bugfix, if 4.3 should default "useDefineForClassFields": true then it doesn't appear to be.

@IllusionMH
Copy link
Contributor

IllusionMH commented Aug 26, 2021

I don't know anything about VS2019 and especially if it uses built-in TS version or one from node_modules when building.

For VS Code it's recommended to use Workspace Version of TS "typescript.tsdk": "node_modules\\typescript\\lib", to have same experience in tsc and errors in file.

And default build task (auto detected) would use TS from local node_modules.

VS Code running 4.3.5 with "target": "esnext" and useDefineForClassFields missing does not emit class fields.

I wasn't able to reproduce this

with next setup

$ npx tsc -v
Version 4.3.5
class MyClass {
    myField: any;
}
{
  "compilerOptions": {
    "target": "esnext"
  },
  "include": [
    "*.ts"
  ]
}

output will be as expected (implied enabled useDefineForClassFields)

class MyClass {
    myField;
}

If VS Code is not configured to use workspace version and in the same time shows 4.3.5 (which is provided with VS Code 1.59.1) in status bar - I would assume that there is chance that globally installed TS version is used for compilation and it's not 4.3.5.

Having simple repo with proper setup (TS in dependencies/devDependencies, tsconfig.json with "target": "esnext") that demonstrates problem will be helpful.

UPD.

Also not reproduced class fields emit in 4.2.4
$ npx tsc -v
Version 4.2.4

$ cat main.js
cat: main.js: No such file or directory

$ cat main.ts                                                                                                                                       
class MyClass {  
    myField: any;
}

$ npx tsc

$ cat main.js
class MyClass {
}

$ cat tsconfig.json 
{
  "compilerOptions": {
    "target": "esnext"
  },
  "include": [
    "*.ts"
  ]
}

@justinfagnani
Copy link

We're starting to see this break Lit projects. useDefineForClassFields defaulting to true is a big breaking change for decorators and should not have been done lightly. Combined with TypeScript not following semver, npm users can very easily accidentally upgrade their versions to TypeScript and have severly broken applications because of this.

At the very least decorated fields should not emit native class fields because there is no way to metaprogram over them and the vast majority of decorators will break.

@andrewbranch
Copy link
Member

I’m closing this issue because it’s causing confusionβ€”the title is a false assertion and leaving it open may lead people to believe it’s true. If you believe there’s a bug, please read through #45076 first, then open a new issue that clearly demonstrates the problem in a reproduceable way (demonstrated with tsc and/or the playground), following the issue template, including code samples or a link to a repository if necessary. If other tools, be they editors or external build systems, are required to demonstrate a problem with TypeScript’s emitted JS, it’s not a TypeScript bug. Thanks!

@KeithHenry
Copy link
Author

KeithHenry commented Aug 31, 2021

@andrewbranch I'm a little confused by this:

the title is a false assertion and leaving it open may lead people to believe it’s true

My assertion is that useDefineForClassFields defaults to true.

However, @MartinJohns said:

The default of useDefineForClassFields depends on your target. For ESNext or ES2020 and above it defaults to true, which is the desired behaviour (to align with the ECMAScript standard).

Are you closing this because useDefineForClassFields should default to true when target is ESNext ?

Where is this breaking change documented? Should or shouldn't it default to true?

Assuming that this is desired behaviour and this is missing documentation I've raised #45653

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 31, 2021

It doesn't with tsc 4.3.5

Yes, it does. I just verified this behaviour.

~/ ξ‚° ./node_modules/.bin/tsc --version                       
Version 4.3.5

 ~/ ξ‚° cat index.ts
class Foo {
    member!: string;
}

 ~/ ξ‚° ./node_modules/.bin/tsc --target es2019 index.ts && cat index.js
class Foo {
}

 ~/ ξ‚° ./node_modules/.bin/tsc --target esnext index.ts && cat index.js
class Foo {
    member;
}

The same result as Andrew had, and exactly why he asked you for a repro using tsc.

@KeithHenry
Copy link
Author

@MartinJohns apologies, it was a bank holiday over here in the UK and when I came back the issue was closed, there didn't seem much point working on reproducing the problem when the response was that it's by design, just undocumented.

@justinfagnani
Copy link

We keep having users who have broken projects because of this. It's very frustrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

5 participants