-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Handle breaking change in class property runtime behavior #27644
Comments
I am happy to see TS looking into this important compatibility issue. This plan sounds good to me. Another potential incompatibility is that TC39's fields are defined with Object.defineProperty, not =, so setters defined in a superclass will not run. |
@littledan this is a bit of a compat nightmare for us and it'd be great to understand why initializers are optional in the current proposal. Why would someone write an initializer-free non-private declaration in the first place? Staking out an own property with the implicit value |
class Foo {
val;
constructor({ val }) {
this.val = val;
}
} I expect to see a lot of code like the above. It's really nice to be able to see the shape of the class without having to look in the constructor, but sometimes the value which goes into the field depends on arguments to the constructor. |
Why is that kind of construct even desirable without static (or even dynamic!) enforcement? It's just a comment written in a different syntax - destined to be out of date and out of sync with what actually happens in the constructor |
fwiw static enforcement of that pattern is trivially possible with eslint, with or without an explicit initializer. |
No, it's meaningfully different: because it uses [[DefineOwnProperty]] semantics, it guarantees the property is an own data property, rather than risking triggering setters on a superclass. You can of course put a I recall we discussed the possibility of requiring initializers at some point, though I can't dig that up just now. |
I'm sure you guys have discussed this to death already, but having a body-level declaration be subtly different from a constructor assignment (i.e. it only matters when there's a superclass setter) is only going to increase developer confusion. Now when I look at code with an uninitialized declaration, I have to nervously wonder whether it's there for documentation purposes, or to overwrite a property slot because of the risk of triggering a base class setter (isn't not triggering it the risk? Why is the base class setter there in the first place) ? |
We did actually discuss it to death in TC39... the classic Set vs Define debate (see summary by @bakkot; even though we've already discussed it a bunch, I'm really happy to hear your thoughts on the issue). Compatibility and constructor correspondence were raised, but the committee was ultimately convinced to take the Define and implicit |
It's true that when I looked at this before it wouldn't have been difficult to migrate things at Facebook (IIRC very few places would have had issues in the first place), but it's also worth noting that usage of setters is pretty rare in JS written at FB (as a matter of both tooling support and style). Where they are used I was able to flag them for manual inspection -- but it may also be possible to do something more heuristically smart than just manual inspection for codebases that use them more frequently. |
This has been a problem with babel's typescript support as well, because we need to know how should we treat field type annotations without an initialization. Without interface Bar {}
class Foo {
bar: Bar = {}
}
class Baz extends Foo {
bar: Bar
} If we only strip the |
I don't think there's much we can do about the strict breakage, but I'm concerned about the fact that this makes it impossible to re-declare existing fields on subclasses. One suggestion: use class N {
node: Node
}
class E {
declare node: Element
} This would have the same behavior as today's redeclaration, while the field syntax would have the standards-compliant behavior. |
This was raised in the office and I strongly opposed it - TS has never used a type annotation to change runtime behavior and I would not want to start now.
I like this quite a bit
Every time I hear this it sounds like an extremely solid argument against the behavior as proposed. If your base class has a setter, it almost certainly wrote it as a setter for a reason, and the base class is going to get super confused when it doesn't see its own setter behavior firing. e.g. class Base {
// This is a nice property that normalizes null/undef to 0!
get value { return this._value }
set value(v) { this._value = v ? v : 0; }
func(x) {
this.value = x;
// Can't happen because the setter fixes it
Debug.assert(this.value !== undefined);
console.log(this.value + 10);
}
}
class Derived extends Base {
// FYI guys, value comes from the Base class, no worries
value;
}
const d = new Derived();
d.func(undefined); // oops! You're asking the derived class author to understand whether or not the base class property is a setter in order to understand whether or not it's safe to use a property declaration in the derived class, whereas previously this was a distinction that was rather hard to notice without a direct call to If your derived class is intentionally stomping on its base class setters, I question whether it's even plausible that it's a correctly substitutable instance of its base class. Breaking your base class should be difficult, not easy. |
FYI, we have some work in progress to move class fields transformation from |
@RyanCavanaugh I think the main confusion for me that class fields don't trigger setters is mostly because of the use of the Personally I would've preferred Getting past the use of const base = {
set someValue(value) {
console.log("triggered a setter!")
}
}
// No setter triggered
const derived = {
__proto__: base,
someValue: 12
}
// No setter triggered
derived.someValue = 12
const derived2 = { __proto__: base }
// setter triggered
derived2.someValue = 12 class Derived extends Base {
// analagous to someValue: 12 in an object literal
someValue = 12
} |
Note: nothing from this list has happened yet Proposal
|
This design sounds good to me. What do you mean by, "Declaration emit now emits getter/setter pairs in classes"? A define-style field in a superclass would shadow a getter/setter pair. |
Today if you build this class class A {
get p() { return 0; }
set p(n) { }
} the declaration emit is class A {
p: number;
} which would prevent us from detecting the derived class accidently clobbering the field. |
Thanks for explaining! |
The best time to fix a issue was early stage. The second best time is now. |
After we noticed problems with backward compatibility of .d.ts files in Azure SDK, we decided to change .d.ts emit when class B {
get p() { }
set p(value) { }
get q() { }
set r(value) { }
} Would emit declare class B {
/** @get @set */ p: any
/** @get */ q: any
/** @set */ r: any
} Typescript 3.7 and above will look for these comments on property declarations and treat them as accessor declarations instead. |
@rbuckton Using class Base {
get brand(): string {
throw new Error(`Brand must be defined`)
}
}
class Derived extends Base {
brand = 'derived'
}
const d = new Derived()
console.log(d.brand) The above works well with class Base {
static get brand(): string {
throw new Error(`Brand must be defined`)
}
}
class Derived extends Base {
static brand = 'derived'
}
console.log(Derived.brand) This issue already happens in TypeScript today and break my use case. I'm not going to say |
That was my point with #27644 (comment) |
@trotyl I disagree. When comparing options, we should consider the frequency and severity of the issues, and I think it's a real stretch to say both options are equally good/bad. The use cases I've seen where [[Set]] is problematic are mostly issues of semantic purity or fairly minor issues that would occur pretty rarely. Your example is the first I've seen that I would agree is a pretty significant footgun when using [[Set]]. But the foot-guns with [[Define]] are generally more significant and seem more likely to occur, at least based on what I've seen in online discussions about this issue. So that's my two cents, but I'm not sure why we're still discussing this at all—the ship has sailed. I agree with those who say that stage 3 shouldn't completely close the window to further changes (e.g. changing the syntax to |
I see your points, but have got different conclusions. Almost every "problematic" examples with Even the example provided by @rbuckton, from a reviewer perspective, I'd say it's intended to break the application. Even with the class Base {
// IMPORTANT NOTE!!!
// DO NOT EVER CHANGE THIS TO ACCESSOR, IT WON'T GET CALLED
x = 1;
}
class Derived extends Base {
get x() { return 2; }
set x(value) { console.log(`x was set to ${value}`); }
} I don't believe this is how class inheritance should be, let alone the base class may not be in the same project. Regarding controlling of override ability, OO language can be divided into two categories, explicit-virtual and explicit final, the latter in practice has been considered a design failure, where in any Java code style you should always write the From the lengthy history of OO language, overriding a base class member without knowing it can be overridden is not something feasible, the whole application could break one day or another. Unfortunately JavaScript is even worse than explicit-final languages, which cannot prevent overriding at all (in common declarative class code). If possible, I'd definitely like JavaScript class member to have explicit-virtual semantics, or at least runtime detection like tc39/proposal-class-fields#176 (comment). |
What about decorators? Will the property descriptor is passed to decorator if |
@trotyl @mbrowne The corpus I inspected, our internal test repos + the ones at In fact, failures only happened in older code: there was one example of a property-override-accessor in Angular 2's examples. I believe it was intentional, and would only work with [[Set]]. There were 7 examples of accessor-override-property, all of which were trying to make the derived property readonly by only providing For modern TS, they would instead write class B {
x = 1
}
class D extends B {
readonly x = 2
} Google and Bloomberg also checked their code bases and found no tricky failures. It's safe to conclude that [[Set]] vs [[Define]] is not practically important. |
Since this targets 3.7.1, I'm going to close this bug. When class fields reaches stage 4, I'll do the final two steps. |
From #12212 (comment) @joeywatts:
It appears from the current definition at https://github.com/tc39/proposal-class-fields that this code will print
undefined
.If the proposal advances further with these semantics, we'll need to figure out a mitigation strategy, preferably sooner rather than later.
One option is to make the code as written a type error for ~2 versions and force people to write an explicit
= undefined
initializer, then remove the error.Edit [@sandersn]:
Here is the current plan, together with what has happened so far.
TypeScript 3.6
declare class C { get p(): number; }
(andset
as well)TypeScript 3.7
class C extends B { declare x: number }
declare
where needed.useDefineForClassFields
."useDefineForClassFields": false
:"useDefineForClassFields": true
:"useDefineForClassFields"
defaults tofalse
TypeScript 3.8
(or, the first version after class fields reaches Stage 4)
"useDefineForClassFields"
defaults to true when targetting ESNext."useDefineForClassFields": false
.The text was updated successfully, but these errors were encountered: