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 just4d541d2 with whitespace turned off.

Github isn't showinge466bb9 which I just pushed but it sure seems to be the head ofalways-error-on-property-override-accessor!

See here for the actual commit history:https://github.com/microsoft/TypeScript/commits/always-error-on-property-override-accessor

This commit creates an error with the following pattern (playground):

constmodify=(v:any)=>Object.entries(v)// just something illustrativetypeTYPE_Y_AS_MODIFIED=['Y',1]classBase{getprop(){returnmodify(this.constructor.methodGeneratingValue())}staticgetmethodGeneratingValue(){return{X:1}}}classSubextendsBase{prop:TYPE_Y_AS_MODIFIED// †staticgetmethodGeneratingValue(){return{Y:1}}}consts=newSub()s.prop// properly typed as ['Y', 1]

The adding of an explicit type to a property (effectively "overloading" the type) isa workaround for something that otherwise seems impossible in Typescript: properly typing a class getter/property based on a static class property.

Although this creates an error on theprop, the type forprop nevertheless works correctly. In other words, Typescript errors but obeys.

Ideally there'd be a way to overload the subclass getter return type, but I haven't yet seen how to accomplish that.

A workaround (to the workaround) is makeprop a proper property and assign itprop to the class/subclass in the constructor, though this breaks overloading.

I hope this is helpful feedback. Cheers.

An example from the real world code:

classSomeClassextendsBaseClass{        @special_reactive_decorator()reactiveProperty     :SomeClass// our override for the `parent` property, which is needed to update the `parentEvent` propertyprivate_parent : thisgetparent() : this{returnthis._parent}setparent(value : this){this._parent=valuethis.reactiveProperty=value}}

In this example, we extend the base model, which has a regular propertyparent, and in the subclass, duplicate all writes toparent to another special reactive property, which triggers updates elsewhere.

@sandersn I don't see any errors here. I also foresee you saying - okay, just create a methodsetParent and override that, but the thing is, in the rest of the codebase theparent is assumed to be a property and fixing all those places / changing the contract is not an option. Thankfully JavaScript is expressive enough to allows to easily augment the property access with the getters/setters in the subclass.

@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 inBaseClass. There,this.parent isnot the same asthis.parent inSomeClass. This is confusing in two ways: broadly, JS doesn't normally work this way; narrowly,this.parent references inBaseClass don't update reactive properties inSomeClass.

This is confusing in two ways: broadly, JS doesn't normally work this way;

Not clear what you mean. This pattern is perfectly valid in JavaScript. From consumer perspective, bothBaseClass andSomeClass have the same interface.

this.parent references in BaseClass don't update reactive properties in SomeClass

Not clear again. Why do you expect the parent class to work in the same way as the subclass. The whole purpose of subclassing theBaseClass withSomeClass is to add that reactivity update. Of course, in theBaseClass that update does not happen, becauseBaseClass does not have thereactiveProperty in the first place.

@canonic-epicure I think the inconsistency@sandersn is referring to can be illustrated withthis 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 workwith 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 typescripttypings 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:

Public instance fields are added with Object.defineProperty() either at construction time in the base class (before the constructor body runs), or just after super() returns in a subclass.

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:

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.

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 theuseDefineForClassFields setting and I think, the "old" way of handling class fields in TypeScript is correct and more idiomatic JS.

I believe the compiler error discussed in this thread should only be generated for theuseDefineForClassFields : true case.

@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 theuseDefineForClassFields : true users, at the expense ofuseDefineForClassFields : false users, who lose the property access augmentation pattern.

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 sinceChrome 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
https://github.com/tc39/proposal-decorators/#set

useDefineForClassFields effectively means that in TS (and hence the compiled result in JS) there is a difference between

class A {   x}

compiling withuseDefineForClassFields=false to

class A {}

vs compiling withuseDefineForClassFields=true to

class A {    constructor() {        Object.defineProperty(this, "x", {            enumerable: true,            configurable: true,            writable: true,            value: void 0        });    }}

given the new rule that even stating defined fields likex above are initialised toundefined whenuseDefineForClassFields=true in the new JS standard ... as opposed to just being omitted in the traditional TS compilation withuseDefineForClassFields=false.

This has flow-on issues with conformity of field overrides in subclasses ... and the proposal itself notes the contested nature of thisissue.

TS adapts to these differing intentions by using the keyworddeclare ... which now can be used to alsoredeclare a field in a subclass, but without incurring the effect that field being initialised in the subclass itself.

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 (useDefineForClassFields=true) and not valid for the "classic" TypeScript class fields (useDefineForClassFields=false).

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 theuseDefineForClassFields=true case.

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 makeuseDefineForClassFields=false more inclusive until the next major version bump in TS. My comment was just a defence of the difficult position that TS must take to evolve the language.

The performance ramifications etc are not a TS issue: the conversation regarding these issues was at the JS level andTS 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 thereverse of what you are doing: ie instead of going from TS->JS, go from JS->TS with the following code:

class A {   x}

Which interpretation is correct when copying and pasting this into TS from JS:useDefineForClassFields=false oruseDefineForClassFields=true? By default, this new JS lanuage construct now means that the original coder meantuseDefineForClassFields=true.

I have no insight into the internal politics of TS, but I presume thatuseDefineForClassFields=false will actually become a legacy feature.

I have no objection thatuseDefineForClassFields=true is a new default behavior, matching JS. That's fine.

However there are reasons (performance, loss of useful programming patterns) to use the "legacy" behavior:useDefineForClassFields=false. The majority of the codebases is written with theuseDefineForClassFields=false assumption, this setting is going to be supported for a very long time.

Now again, the people who arenot 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 whowill 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 foruseDefineForClassFields=false, then I support that, but my point has (repeatedly) been that this is not favouritism.

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.

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 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 typedefinition files. Because of course I can describe an existing API that works like described above. And still TS now sees this as anerror, 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 whatuseDefineForClassFields is set to), this implementation will error out. Third party library and typings authors need to adjust their type definition files so that TS will accept it.

And this has certainly not been an edge casefor 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 thedefine approach and thus all that we are left with this implementation is needlessly invalidated type definition files. So at least this was ignoring the perspective of the type definition authors.

@yGuy:#40220

Additional consideration is that, according to theTC39 proposal, it will be possible to switch to theSet semantic for certain properties, using the ES decorators:

As a mitigation, the decorators proposal provides the tools to write a decorator to make a public field declaration use [[Set]] semantics. Even if you disagree with the default, the other option is available. (This would be the case regardless of which default TC39 chose.)

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 tohttps://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:

• Overriding property of the base class with getter/setter pair is a useful "classic" JavaScript pattern, which allows for example augmenting property access in some 3rd party codebase (or even in your own code)
• Right now, in "modern" JavaScript this pattern won't work as expected (as demonstrated by your example). However, there's a promise from TC39 commitee to provide an opt-in escape route to the "classic" behavior with decorator
• In TS "classic/modern" switch is defined with "useDefineForClassFields" compiler option (SET/DEFINE semantic for class fields)
• The "classic" behavior with "SET" semantic will need to be supported for a long time, since a lot of code is written with SET semantic in mind
• It may or may not be reasonable to restrict this pattern for "DEFINE" semantic - personally I don't have an opinion on this. Perhaps the decision on this can be postponed until the decorator proposal reaches stage 3 at least. Perhaps it should be more like a linter rule, than a compiler error.
• It is very reasonable to keep the pattern valid for SET semantic
• Right now, the compiler restricts this pattern for both SET/DEFINE semantics
• Proposal is - to keep the compiler error only for the "DEFINE" semantic

It looks to me like there are two distinct new proposals that have come out of the discussion, which are related to#40220.

• Proposal: allow ambient property declarations to override accessors as well #40220 proposes that ambient properties be allowed to override accessors.
• It sounds like@yGuy also wants accessors to override ambient properties to model JS dependencies.
• It sounds like@canonic-epicure wants to use [[Set]] semantics to build a TS class hierarchy.

@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]].

@sandersn Ok, opened a new issue withproposal. Elaborated on the example and made the SET/DEFINE context more explicit.

@andrewbranch@sandersn Ok, the proposal created. What will be the next step?

A Workaround

Use case: Narrowing the type of an accessor on a subclass

Example:

interfaceOptions{/*...*/}classSuper{  #options:Options;getoptions():Options{returnthis.#options;}setoptions(o:Options){this.#options=o;this.optionsChanged()}optionsChanged?():void}interfaceSpecificOptionsextendsOptions{/*...*/}classSub{declareoptions:SpecificOptions;}

Workaround:

interfaceOptions{/*...*/}classSuper{/**@internal */statico(proto:Super,_:string):void{Object.defineProperty(proto,'options',{get(){returnthis.#options;},set(v){this.#options=v;this.optionsChanged?.(v);},});}  #options:Options;  @Super.ooptions:Options;}interfaceSpecificOptionsextendsOptions{/*...*/}classSub{declareoptions:SpecificOptions;}

Explanation: The original reason for using accessors to defineoptions was to run side effects when setting the property. This change prevents subclasses from narrowing the type ofoptions. The solution is to define the accessors using a decorator defined as a static function on the super class. We could also use a function declaration if we didn't use private storage.