-
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
Always error on property override accessor #37894
Conversation
Previously this was only an error when useDefineForClassFields: true, but now it's an error for all code. This is a rare mistake to make, and usually only occurs in code written before `readonly` was available. Codefix to come in subsequent commits.
1. Add add-all test 2. Add codefix that delegates to get-set accessor refactor. Needs massive amounts of cleanup and deduplication.
1. Fix lint. 2. Make code easier to read. 3. Turns some asserts into bails instead.
Assigning this to two people since the reviewers list is broken. @jessetrinity this is only technically touches refactors, but I thought it might be interesting to you anyway. |
Easiest way to view the changes to the original refactor code is to view just 4d541d2 with whitespace turned off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a codefix test asserting what happens when the base class has the property declaration and is in another file. From glancing at the code I think it might be broken, but it would be good to have a test either way.
startPosition = baseProp.valueDeclaration.pos; | ||
endPosition = baseProp.valueDeclaration.end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, how do you know this valueDeclaration
is in file
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't! I need to update file
to be the file with the superclass, not the file with the error.
Additionally, getSupers
doesn't follow aliases, so doesn't work for imported classes either.
1. Follow aliases with getSupers. 2. Make fix in file with superclass, not with the error.
Github isn't showing e466bb9 which I just pushed but it sure seems to be the head of |
See here for the actual commit history: https://github.com/microsoft/TypeScript/commits/always-error-on-property-override-accessor |
An example from the real world code: class SomeClass extends BaseClass {
@special_reactive_decorator()
reactiveProperty : SomeClass
// our override for the `parent` property, which is needed to update the `parentEvent` property
private _parent : this
get parent () : this {
return this._parent
}
set parent (value : this) {
this._parent = value
this.reactiveProperty = value
}
} In this example, we extend the base model, which has a regular property @sandersn I don't see any errors here. I also foresee you saying - okay, just create a method @everyone in this thread, please post your examples where you use the property augmentation with getters/setters and describe the use case it solves for you. Need to demonstrate that this pattern is valid. |
The problem is in |
Not clear what you mean. This pattern is perfectly valid in JavaScript. From consumer perspective, both
Not clear again. Why do you expect the parent class to work in the same way as the subclass. The whole purpose of subclassing the |
@canonic-epicure I think the inconsistency @sandersn is referring to can be illustrated with this example. You can see the logged results by clicking the "Run" button in the toolbar. |
I think the confusion stems from the fact that this does not work with class fields. It does however work in almost any older code that defined fields in a different way, e.g. by declaring the field with a default on the prototype of the base class, or simply setting the value in the constructor, which is not what happens with class fields. We had the same problem in our code: our code was written with without ES classes and ES class fields. And when we wrote our typescript typings for the JS library, we declared properties and field as a field and it was possible to have subclasses "override those fields" with getters and setters, but we are not able to model that anymore with the change. I too, find it unintuitive that the code you posted does not work, which is due to the way class fields work in JS:
Due to this mechanism, your get and set accessors simply disappear (get hidden) and your code does not work, but only if you are using native class fields. |
@andrewbranch Yes, I'm totally for treating this as an error for the ES class fields, your example illustrates it very well and @yGuy describes what happens in it:
However, personally, I think ES class fields are non-idiomatic JavaScript, because they always "hide" the value in the prototype (even if the fields does not have the initializer). This breaks the other idiomatic JavaScript pattern, where you "thread" some property object through the class hierarchy, at each level using the value of the previous level as a prototype. Plus it breaks the "property access augmentation" pattern. Also, anybody tried to benchmark the class instantiation, the "old" class properties vs "new" ES class fields? If you try, you'll be very surprised. This is why, personally, I'm not going to use the I believe the compiler error discussed in this thread should only be generated for the |
@sandersn @andrewbranch Any comments regarding the note, that the compiler error under discussion is only relevant for the ES class fields case? |
The way I see it, the very useful pattern has been needlessly restricted in TypeScript. Language expressiveness has been artificially limited. This compiler error is protecting the Why favoring one part of the community? |
There seems to be a confusion of intention here among commentors. TS is actually tracking the JS standard on this feature, which has effectively been implemented in JS since Chrome 72 shipped with these semantics enabled. This is necessary to prepare for a shift within JS itself on this issue: https://github.com/tc39/proposal-class-fields
compiling with
vs compiling with
given the new rule that even stating defined fields like This has flow-on issues with conformity of field overrides in subclasses ... and the proposal itself notes the contested nature of this issue. TS adapts to these differing intentions by using the keyword This is not a case of favouritism. TS must adapt to the new JS standard itself ... but as with most of these progressive adaptations, legitimate JS may require hacks within TS to accommodate edge cases. |
I don't think there's a confusion here, I believe all commentors understand the topic very well. I'm just pointing that, the compiler error being discussed is only valid for the new JS standard ( Personally, I think that "define" semantic for class fields is a very unfortunate ES spec quirk. Also, if you benchmark the instantiation of class with native JS fields, you'll notice its 10x slower. That is why, again personally, I'm not going to use native class fields, nor the "define" semantic for them. People who prefer the "define" semantic, will of course benefit from this compiler error, which will protect them from some un-intuitive errors in the code, as illustrated by @andrewbranch. But what about people who are not going to use the "define" semantic? Such people are just needlessly restricted from using the very useful and practical patterns, arising from the property augmentation with getter/setter. This is why I believe the error should be only enabled in the |
You are demonstrating the confusion that you say does not exist, and I feel for your situation because I have had to do extensive recoving to accommodate new behaviours. You seem to be confusiing a decision that TS has made to move towards an emerging JS syntax standard vs some favouritism. That's the confision that I am highlighting. This is not a criticism of your argument - I actually agree that this is a sharp incline for adoption by some coders and I wish that there were a keyword that could accommodate the legacy coding intentions better or just make The performance ramifications etc are not a TS issue: the conversation regarding these issues was at the JS level and TS was appartently rather neutral. The problem that TS faces is that as a represtation of "JS + types", it needs to maintain good conformity with the JS representation and syntax of the language itself. Imagine the situation of doing the reverse of what you are doing: ie instead of going from TS->JS, go from JS->TS with the following code:
Which interpretation is correct when copying and pasting this into TS from JS: I have no insight into the internal politics of TS, but I presume that |
I have no objection that However there are reasons (performance, loss of useful programming patterns) to use the "legacy" behavior: Now again, the people who are not going to use the "define" semantic - they lose the patterns I've mentioned because of this compiler error. Why do we have this error? To protect the people who will be using the "define" semantic. The 2nd group of people is favored at the expense of the 1st. What is the confusion here? |
The confusion is that you are mistaking a TS design goal for favouritism. "6. Align with current and future ECMAScript proposals." If you want better legacy support for |
Yes, my only point is that "legacy" behavior has been needlessly restricted by this compiler error. Also I'm pointing that this "legacy" behavior will have to be supported for a very long time (performance problem with native class fields), so its worth actually fixing this restriction. |
@sandersn @andrewbranch Thanks for the feedback. @poseidonCore You are right, its not a favoritism. It is ignoring. |
I can see that this can make sense for newly compiled TypeScript code. I don't understand why the same rules need to be put into place for type definition files. Because of course I can describe an existing API that works like described above. And still TS now sees this as an error, when it clearly is not one. That is inconvenient to everyone, because the way it has been implemented in the third party library ( no matter what And this has certainly not been an edge case for JavaScript for years, so many files are affected, I guess. In fact I would argue that the majority of hand-written JS code did never use the |
Additional consideration is that, according to the TC39 proposal, it will be possible to switch to the
This means, that the "property access augmentation" pattern will still remain valid in JavaScript, even for native class fields, a user will just need to opt-in for it, using the decorator. Not in the TypeScript though. But if you don't listen, how can you hear? |
This is what I was referring to when I said "I wish that there were a keyword that could accommodate the legacy coding intentions better", and is why I linked to https://github.com/tc39/proposal-decorators/#set. There is a relevant discussion here: https://github.com/tc39/proposal-decorators/#how-should-i-use-decorators-in-transpilers-today As for "But if you don't listen, how can you hear?" ... bear in mind that these are still proposals, and the TS team is probably hearing more than what you yourself are listening to. |
Part of this conversation has been hovering right on the edge of what’s appropriate for this particular forum, so while I appreciate that you all have kept it from getting out of hand so far, I want to preemptively refocus it and lower the temperature. The TypeScript team is very open to criticism on technical decisions we’ve made based on technical reasons and anecdotes about your own experience using the language. I would ask that the discussion on the issue tracker (1) assume good intentions of everyone involved, and (2) remain focused on the language feature in question. Thanks! |
@andrewbranch I'm totally for constructive discussion. It is just sometimes you need to shout to be heard (and noticed). Summarizing the recent comments:
|
It looks to me like there are two distinct new proposals that have come out of the discussion, which are related to #40220.
@canonic-epicure @yGuy can you open sibling issues to #40220? It'll make it easier to prioritise long-term. And @canonic-epicure I think you've collected enough arguments to make a solid proposal. However, I still don't understand your example code. Can you rework that for the proposal? I think you may just need to make clear that it only works with [[Set]]. |
@andrewbranch @sandersn Ok, the proposal created. What will be the next step? |
A WorkaroundUse case: Narrowing the type of an accessor on a subclass Example: interface Options { /*...*/ }
class Super {
#options: Options;
get options(): Options { return this.#options; }
set options(o: Options) { this.#options = o; this.optionsChanged() }
optionsChanged?(): void
}
interface SpecificOptions extends Options { /*...*/ }
class Sub {
declare options: SpecificOptions;
} Workaround: interface Options { /*...*/ }
class Super {
/** @internal */
static o(proto: Super, _: string): void {
Object.defineProperty(proto, 'options', {
get() { return this.#options; },
set(v) { this.#options = v; this.optionsChanged?.(v); },
});
}
#options: Options;
@Super.o options: Options;
}
interface SpecificOptions extends Options { /*...*/ }
class Sub {
declare options: SpecificOptions;
} Explanation: The original reason for using accessors to define |
Support svg image (using sharp), not support online svg now Add externals: { sharp: 'commonjs sharp'} in webpack.config.js lovell/sharp#2350 (comment) Zhihu username sometimes shows undefined, it is because profile is gziped Add gzip: true in fetchProfile Update ts-loader webpack typescript webpack-cli, with somebugs this undefined https://github.com/Microsoft/TypeScript/wiki/FAQ#why-does-this-get-orphaned-in-my-instance-methods Explictly call member function, e.g. const sendRequest = (options) => httpService.sendRequest(options) Always error on property override accessor microsoft/TypeScript#37894 My solution is set tooltip and description in the constructor Remove dep of uglifyjs-webpack-plugin add keyword of 知乎专栏, 知乎, Markdown
With useDefineForClassFields: true, it was already an error to have a property override an accessor or vice versa, because neither works with that flag on. With useDefineForClassFields: false, it is possible to have a property usefully override a getter/setter pair, but there will be a runtime error if there is no setter. My analysis of our user tests, and our partner's analyses of their internal code bases, didn't show any uses of this pattern, however.
However, this is a new error, so I'm going to hold this change until 4.0, even though it shouldn't affect many projects.
The code change to add the error is tiny. Most of the review adds a codefix, fixPropertyOverrideAccessor. This codefix just delegates to generate get/set accessor, and moves the code into a central place. I added parameters to the function so that it could be called from a refactor as well as a codefix. I think it's possible to reconcile the types used in the two, but I didn't want to do it here.
Fixes #34585
Fixes #34788