Breaking change to default useDefineForClassFields to true should be clearly documented

[KeithHenry]

Bug Report

In some contexts (I'm not sure exactly which, but "target": "esnext" does it) useDefineForClassFields now defaults to true.

This is a breaking change for any code with "experimentalDecorators": true, but I can't find documentation for it anywhere.

🔎 Search Terms

useDefineForClassFields experimentalDecorators

🕗 Version & Regression Information

The useDefineForClassFields flag was introduced in 3.7 but defaulted to false: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier

This is a known breaking change when using decorators: #35081

In #42663 this was changed so that in some contexts useDefineForClassFields now defaults to true.

There is no mention of useDefineForClassFields on https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-4.html (or 4.3, or 4.2, or 4.1)

There is a mention on 4.0, but in as different context and the default change isn't mentioned https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-0.html#properties-overriding-accessors-and-vice-versa-is-an-error

This is a breaking change for all projects that use decorators, it should be clearly identified as a breaking change.

⏯ Playground Link

This is missing documentation

💻 Code

This is missing documentation

🙁 Actual behavior

Release notes don't mention the breaking change. I'm not even sure which release they should be in.

🙂 Expected behavior

Release notes should clearly document the breaking change.

[MartinJohns]

While I understand the sentiment for a mention of this in the release notes, I'm seriously confused about this. You're stating that you want to target ESNext, and this is what you get. The behaviour with define-semantics is in accordance with the ESNext version.

ESNext is by definition breaking. If you don't want this, why do you target ESNext? This confuses me to no end.

[KeithHenry]

I think "target": "es2020" does it too, but it's hard to be sure as I can't find any documentation for when useDefineForClassFields defaults change.

I'm fine with breaking changes on our esnext build - as they come up I'll find out what caused them and make changes appropriately, that's why we have it.

My problem was that it took significant digging through the issues and PRs on this repository to even figure out what the breaking change was. I'm not even clear whether it's entirely by design as #45584 was closed for being misleading. I don't know what combination of target or any other tsconfig settings cause it to change.

Finally, tsc should probably just throw an exception if passed both useDefineForClassFields and experimentalDecorators as true, rather than outputting broken JS, if the intent is for them to be mutually exclusive.

[orta]

Makes sense, the tsconfig reference was updated with the new default info already: https://www.typescriptlang.org/tsconfig#useDefineForClassFields

But I've also sent a PR adding this to the 4.3 release notes on the website: microsoft/TypeScript-Website#2008

[MartinJohns]

Finally, tsc should probably just throw an exception if passed both useDefineForClassFields and experimentalDecorators as true, rather than outputting broken JS, if the intent is for them to be mutually exclusive.

It's really not intended to be mutually exclusive. It's just that the decorator support in TypeScript is based on an old (outdated) decorator proposal, and the team is waiting for proper decorator support until the new decorator proposal is finalized. The old decorator proposal is just incompatible with the define-semantics of class fields. This is just what happens with experimental features, and also why the team doesn't add such experimental features anymore. Besides this, class-, method and parameter decorators should continue to work just fine.

[KeithHenry]

@MartinJohns I get that, but decorators (despite being 'experimental') have been in TS for years and are used by a lot of projects. Meanwhile JS decorators have been in limbo/committee-hell for almost as long and are still only in stage 2.

TC39 are aware of this and their own recommendation is the opposite of yours:

Note that these decorators depend on "[[Set]] semantics" for field declarations (in Babel, loose mode). We recommend that these tools maintain support for [[Set]] semantics alongside legacy decorators, until it's possible to transition to the decorators of this proposal.

I get that initialising fields is now best practice (even though not doing so is not breaking) and TS should emit whatever the compliant JS is for any given ES target, but you're still breaking one feature with another and not documenting it.

Besides this, class-, method and parameter decorators should continue to work just fine.

They do, but that makes it worse... it's part of the reason why it cost my team (and probably many others) so much time to track down this problem. Property decorators just silently do nothing, but other decorators work (and unit tests that isolate decorators work), it took us a while to identify that it was actually a bug in the JS emitted by TS, and then even longer to track down that it wasn't actually a bug, it was broken on purpose but not documented anywhere.

The whole point of using Typescript is to turn run-time errors into compile-time ones, silently applying a secret config change into JS code that hides the actual error is kind of a big fail, as least for how we actually practically use it.

Yes, you are technically correct (the best kind of correct) but it still feels like the wrong direction for Typescript.

tsc should throw an exception if you're using a property decorator with config options that will cause that emitted code to do nothing.

[fatcerberus]

It's tricky in this case, I think, because it's not so much a "secret config change" as just "emitting spec-compliant JS whenever it's possible to do so according to your target", which is what TS has always done. Unfortunately, there's no way, today, to get the [[Set]] behavior while also having native class fields in the output, so useDefineForClassFields: true ends up being implied simply due to being imposed by the JS runtime.

[andrewbranch]

Thanks @orta for updating the docs. Talked to @rbuckton about this, and he points out that some property decorators will work, if you’re just doing some kind of metadata tracking or dependency injection; i.e. if you aren’t trying to replace the property with a getter/setter, so there’s no way we can flat-out disallow property decorators under this combination of settings. We did discuss the possibility of issuing suggestion diagnostics, either in the tsconfig itself (for the suspicious combination of target/useDefineForClassFields/experimentalDecorators), or on property decorator usage (maybe the first usage per class or per file). Not sure exactly what the suggestion would say; we really don’t know whether the decorator usage will be valid or not, and that doesn’t make for a great suggestion.

[justinfagnani]

Finally, tsc should probably just throw an exception if passed both useDefineForClassFields and experimentalDecorators as true, rather than outputting broken JS, if the intent is for them to be mutually exclusive.

It's really not intended to be mutually exclusive.

useDefineForClassFields simply breaks most decorators. They should be mutually exclusive and users should be told about it as early as possible. I know, as @rbuckton says, that some decorators could work, but according to the surveys we did in the decorators working group, most decorators do replace fields with accessors. Either way, there's a huge chance this breaks users. A 100% chance for my customers.

Why can't the presence of a decorator change only the decorated field to use assign semantics? That's a non-breaking change and seems a lot better than silently breaking users.

[rbuckton]

Why can't the presence of a decorator change only the decorated field to use assign semantics?

This seems like it would introduce even more inconsistent and surprising behavior. One of the reasons we introduced declare x; field declarations was to give you the ability to declare and decorate a field without defining it (though you have to move the initializer to the constructor). I'm not necessarily opposed to the idea, but this further complicates the semantics of TS decorators compared to what may eventually be native decorators.

Another option we could consider would be to have the default behavior for useDefineForClassFields depend not only on target but on the presence of experiementalDecorators, such that enabling decorators puts all of your classes in Set-semantics mode unless explicitly overridden.

[justinfagnani]

@rbuckton

This seems like it would introduce even more inconsistent and surprising behavior.

Not for the people who have existing working code that breaks from simply running npm update. Our users don't have declare x; now because they didn't need it. Yes, we're in a bad spot at the moment with decorators, but the current behavior only makes it worse.

I don't think how the semantics of current decorators relate to upcoming native decorators matters much, and don't think making decorated fields use set semantics would actually complicate things compared to trying to explain why they suddenly don't work anymore. As someone who's having to explain why things are breaking I think the current situation is complicated and just got a lot more complicated with these new unannounced changes.

I would preserve decorators as-is when the experiementalDecorators flag is on and don't break existing code. If authors want to use native decorators when they arrive, then make them turn off experiementalDecorators to get them. They're already too different for them to coexist.

Another option we could consider would be to have the default behavior for useDefineForClassFields depend not only on target but on the presence of experiementalDecorators, such that enabling decorators puts all of your classes in Set-semantics mode unless explicitly overridden.

This would be fine too. Our users have experiementalDecorators enabled so their code would continue to work.

[andrewbranch]

This issue was about docs, and the release notes have been updated, so I’m going to close. The result from the design meeting (#45718) was that we're open to, but not enthusiastic about, adding a suggestion diagnostic in tsconfig.json. Feel free to continue discussion here, but if someone is passionate about taking further action, I would open a new issue with a proposal.