[Proposal] Limit the compiler error about overriding the base class property with getter/setter accessors pair to the `useDefineForClassFields : true` case only

[canonic-epicure]

As suggested by @sandersn, opening a sibling issue to track the proposal from the https://github.com/microsoft/TypeScript/pull/37894

Topic

Currently, when a property in the base class is overridden with a getter/setter pair, compiler always generates an error: Playground link

class BaseClass {
    prop        : string        = 'base_class'
}

class SubClass extends BaseClass {
    _prop       : string        = 'sub_class'

    /*
       ERROR: 'prop' is defined as a property in class 'BaseClass', but is overridden here in 'SubClass' as an accessor.
    */
    get prop () : string {
        return this._prop
    }
    set prop (value : string) {
        this._prop = value
    }
}
// try running this snippet with `useDefineForClassFields : true` and `useDefineForClassFields : false`
console.log((new SubClass).prop)

This error is motivated by the fact, that in modern JavaScript, class fields have DEFINE semantic and the above example won't work as intuitively expected if useDefineForClassFields compiler option is set to true.

The property access augmentation pattern

Need to note, that historically nobody was ever using the DEFINE semantic for class properties. Pretty much all the JavaScript and TypeScript code in the world currently assumes the SET semantic for class fields.

SET semantic makes it possible to use the property access augmentation pattern. Its main use case is to trigger some arbitrary code on property read/write. This is very useful for variety of purposes and is done by simply overriding the property of the base class with getter/setter accessors pair.

For example, lets say we have some 3rd party library with a class with parent property.

class ExternalClass {
    parent      : this      = undefined
    children    : this[]    = []

    appendChild (child : this) {
        child.parent    = this        ​
   ​}
}

In the whole library, the parent is assumed to be a regular property and is accessed with dot notation.

Now lets say we want to plug this 3rd party library in our codebase, which has reactive capabilities. We need to track the access to the parent property and trigger some extra code on property write:

const reactive : any = undefined
type Reactive<V> = V

class OurClass extends ExternalClass {
   ​@reactive()
   ​reactiveParent     : Reactive<this>     = undefined

   ​private _parent : this
   ​get parent () : this {
       ​return this._parent
   ​}
   ​set parent (value : this) {
       ​this._parent = value
       ​this.reactiveParent = value
   ​}
}

With this simple subclass, we can use the 3rd party library code unchanged and mirror the writes to the parent property to the reactiveParent (which is plugged into the rendering process elsewhere).

DEFINE / SET semantic

TypeScript and JavaScript historically were using SET semantic for class fields. Pretty much all the code in the world assumes it. However, TC39 committee decided that class fields should use DEFINE semantic and to provide the way to migrate to new behavior, TypeScript introduced the new compiler config, useDefineForClassFields, disabled by default, because its a breaking change. Obviously, the "classic", historical behavior will need to be supported for a very long time.

With this option enabled, the "property access augmentation" pattern becomes invalid, as demonstrated by the example in the beginning (see however, the "Additional considerations" below). It is probably reasonable to generate a compiler error being discussed in this case.

With this option disabled, the pattern remains valid. In this case there are no reasons to limit the language expressiveness and restrict the user from using the pattern.

Proposal

Proposal is to generate the compiler error being discussed only for the "DEFINE" semantic (useDefineForClassFields : true) case.

Additional considerations

There's a promise from TC39 committee to provide an opt-in escape route to the "classic" SET semantic for class fields, using a decorator. This means, that the "property access augmentation" pattern will probably remain valid in JavaScript, even for native class fields, a user will just need to opt-in for it, using the decorator.

Ideally, compiler will need to be smart, to determine which semantic the field is actually using, to avoid the needless restrictions. Perhaps the nature of this error is that it should be handled better by the linter, rather than compiler.

[sandersn]

Can you give examples of production libraries that use this pattern? One reason I was so confident in making it an error for all targets is that Microsoft, Google and Bloomberg checked their Typescript code with the error enabled and didn't run into any problems.

Another reason, while I agree that [[Set]] is more expressive, is that the future is unfortunately [[Define]]. My thinking is that if nobody's code is broken now, we should keep it that way by preventing new code from being written that will need modification later.

[trusktr]

Obviously, the "classic", historical behavior will need to be supported for a very long time.

I've gotta say, it is very bad TC39 did that.

Anyway, the OP has a good point: when useDefineForClassFields is set to false, there should be no error, as that is how it always worked before in ES5 classes, and even in transpiled ES6 classes for a very long time.

There's a promise from TC39 committee to provide an opt-in escape route to the "classic" SET semantic for class fields, using a decorator. This means, that the "property access augmentation" pattern will probably remain valid in JavaScript, even for native class fields, a user will just need to opt-in for it, using the decorator.

Note that the decorator would need to be applied inside the base class, which means one would still need to modify the external library where they import the base class from unfortunately.

[canonic-epicure]

@sandersn We use this pattern in production at Bryntum. Sorry, can't provide a link to source code, since its proprietary.

The use case is that we have a "regular" codebase, which provides data structures, like tree, and we have a "reactive" codebase. The "regular" codebase is mature and stable, maintains backward compatibility, making changes in it is really not an option. However we need to combine both codebases and make them interoperable. The example I've posted is actually a real-world case, which solves the integration task with the "property access augmentation" pattern.

We also use this pattern extensively in our JavaScript codebase (which assumes ES5 and does not use native class fields).

I also use this pattern a lot in my open-source libraries (this is why I'm advocating for easing the error message), for example here.

[canonic-epicure]

One more example from my open-source project, testing tool called Siesta:

https://github.com/bryntum/siesta/blob/6dac32a6a2d0ce4dbab098b280f5ded87d930cf8/src/siesta/test/TestResult.tsx#L145

There's a base class AssertionBase, which contains a passed flag, indicating whether the assertion is passed or not. Also, there are special, asynchronous assertions, which consists from 2 parts - creation and resolution (which can be added after delay). For such async assertion, the passed flag is determined by the resolution - so it is natural to "wire" the reading of that flag to the resolution instance.

This pattern is very useful, for example, if I'd not use it in this "async assertion" case, I'd have to create a new method getPassed() and then replace all readings of .passed with that method call. As you can imagine this can be problematic, especially if you have some "external" code for which you need to keep the backward compatibility.

[andreialecu]

From a proprietary code base:

@InterfaceType() // nestjs decorators
export class NotificationBase extends TimeStamps {
  /**
   * Whether the notification should be hidden in the app or not
   * This is not an actual persisted field, it's overriden in subclasses instead
   */
  @Field()
  showInNotificationCenter?: boolean;
}

@ObjectType({ implements: NotificationBase })
export class TeamChatNotification extends NotificationBase {
  // @ts-expect-error: https://github.com/microsoft/TypeScript/issues/44927
  get showInNotificationCenter() {
    return this.unreadMessageCount > 0;
  }
}

This is a common pattern we use. Since a decorator can only work on a regular field, there's no other way to define this sort of behavior.

[sandersn]

@andreialecu Shouldn't NotificationBase be abstract?

[andreialecu]

@sandersn that wouldn't work in this case, no. NotificationBase needs to be a concrete type because it is used to define "discriminators" for a mongodb schema, among other things.

Here's an example:

@Module({
  imports: [
    TypegooseModule.forFeature([
      {
        typegooseClass: NotificationBase, // Cannot assign an abstract constructor type to a non-abstract constructor type.ts(2322)
        discriminators: [...]

There are many other scenarios where things would break if it was made abstract:

However, I'm not sure what making it abstract would achieve since it doesn't prevent the TS error.

The main reason for the pattern is to be able to define this in the base:

  @Field()
  showInNotificationCenter?: boolean;

Notice the decorator. It is used for a NestJS "code first" graphql schema definition.

Since you cannot apply decorators on a plain getter, this is the only way to make it work that I can think of. In practice it all works properly, it's just the TS error that needs to be ignored.

[webstrand]

It seems like this'll cause compatability issues in the future? If a project with useDefineForClassFields: false extends a class from a library that was built with useDefineForClassFields: true, then the error message will not be shown but the error will occur at runtime. Silencing this error message in specific conditions will only cause confusion later on, unless TC39 decides to reverse their position.

[canonic-epicure]

@webstrand The scenario you describe is valid, but its a matter of the community fragmentation, which will inevitably happen, based on the preferred semantic for class fields.

Your concern is valid, however, how often do you extend a class from external library and you need to override the property from it with getter/setter pair? Its a matter of proper documentation for that library.

In the same way, one could raise a concern, that what if some library is using native generators functions, but the project that is using it, has set the compilation target to es5. We are talking about the problem of different compilation targets for library/project. Its a common problem and not a reason to suddenly restrict the pattern, that worked for 20 years.

[webstrand]

The difference, as I see it, between this issue and generator/es5 is that useDefineForClassFields is not enabled by default so the additional safety of the error message becomes opt-in. Also, unlike the generator compatibility, this error message isn't tied to compilation targets.

I would be happier if this behavior were opt-out via a dedicated flag, rather than attaching more behavior to the useDefineForClassFields flag. Or even if we got a unique set of targets, like es2015-set that explicitly made the difference in compilation targets clear.

It sounds like, going forwards, programmers will want a way to declare class properties with [[set]] semantics. Once class-fields fully lands, though, I expect that the target (es2021 or whatever is chosen) will use the native syntax for class fields. What if syntax was added to distinguish between the two kinds of properties so that a programmer could opt-out of using define semantics on a per-field basis?

class foo {
    declare foo = 5; // set semantics
    bar = 5; // define semantics
}

or something similar. And then the error would only need to be shown for fields with [[define]] semantics?

[canonic-epicure]

The difference, as I see it, between this issue and generator/es5 is that useDefineForClassFields is not enabled by default so the additional safety of the error message becomes opt-in.

Hm.. Isn't it implicitly enabled when target is esnext, es2021? (I haven't checked, but I guess it should be. And I hope, for these targets, the useDefineForClassFields can be explicitly set to false, to keep the backward compatibility of the codebases).

I would be happier if this behavior were opt-out via a dedicated flag, rather than attaching more behavior to the useDefineForClassFields flag.

I believe useDefineForClassFields is exactly the flag that should control this error message. After all, the point is that concern about this compilation error is only valid for [[define]] semantic, and the useDefineForClassFields is what controls the semantic.

It sounds like, going forwards, programmers will want a way to declare class properties with [[set]] semantics.

Yes, definitely. And it seems the new decorators proposal address it with the new accessor keyword: https://github.com/tc39/proposal-decorators

[fatcerberus]

What do you propose should happen, today (or more accurately, in the near future), when the target is set to e.g. es2021 with useDefineForClassFields=false and someone writes the following code:

class C {
    foo;
    bar = 1;
}

foo can be [[Set]] just by assigning it the constructor (because tsc will elide it), but bar must be downleveled despite the es2021 target because otherwise it gets [[Define]]'d per ES. Might cause some confusion: "Why is it not just using ES2021 syntax in the output JS?"

[canonic-epicure]

I'd suggest the following behavior:

• target es2021/esnext + useDefineForClassFields not specified - all fields get [[Define]] semantic per ES spec, that is, implicit is [[Define]]
• target es2021/esnext + useDefineForClassFields specified (either as true or false) - use the semantic according to the useDefineForClassFields, iow, preserve the user intention

In such setup, people who prefer [[Define]] semantic should either set the compilation target to es2021 or useDefineForClassFields=true. People who prefer [[Set]] semantic will be using useDefineForClassFields=false

[canonic-epicure]

One more example, I've stumbled upon today. I've implemented various primitives on the pure-data Segment class like (there's a lot more logic):

export class Segment {
    start           : number        = undefined
    end             : number        = undefined
    length          : number        = undefined

    contains (point : number) : boolean {
        return this.start <= point && point < this.end
    }
}

Now, I want to implement a "segment that represent a dimension of client rect of the DOM element" and be able to re-use the primitives from Segment. Naturally, start and end are bound to the getBoundClientRect results of the element:

export class ElementDimension extends Segment {
    el              : Element           = undefined
    type            : 'width' | 'height' = 'width'

    get start () : number {
        return this.type === 'width' ? this.el.getBoundingClientRect().left : this.el.getBoundingClientRect().top
    }
    set start (value : number) {
        // readonly
    }

But TS compiler generates an unnecessary error, which is the scope of this proposal.

[canonic-epicure]

@sandersn I believe the community feedback on this proposal is positive. What will be the next step for it?

[sandersn]

We'll take this up at a design meeting when we think that there's enough demand for it. However, right now it's two people's examples against a survey of many code bases across 3 large companies. If you can show quantitatively that demand is high, it would help.

[canonic-epicure]

Its two people examples only in this thread. Plus 12 positive emojis. Plus many more examples in the previous thread, which itself has 12 negative emojis.

https://github.com/microsoft/TypeScript/pull/37894#issuecomment-678646084 +3 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-679190977 +12 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-679279784 +3 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-701947017 +13 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-717789311 +5 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-770318450 +1 emojis https://github.com/microsoft/TypeScript/pull/37894#issuecomment-800857078

This means in total, 2 + 12 + 12 +7 + 3 + 12 + 3 + 13 + 5 + 1 = 70 people are positive for this feature. This amount of developers are representative enough.

Can you share what codebases were examined in the survey? Are they written in the OOP approach? Do the authors of those codebases have the JavaScript background, or they come from other languages?

Because in our internal JavaScript code base, the property access augmentation pattern is used excessively. It is a valid "classic" JavaScript pattern, which assumes [[Set]] semantic for properties, its not clear, why TypeScript restricts it both for [[Define]] and [[Set]].

[canonic-epicure]

Keep in mind, we are talking about adding a single if statement somewhere in the codebase, where the error is generated.

[canonic-epicure]

@sandersn ^^^

[canonic-epicure]

@sandersn If I'll contribute a PR for this proposal - will it be merged?

[sandersn]

We discussed this at the weekly suggestion backlog meeting. The bottom line is that Typescript needs to forbid mixed accessor/property overrides because they're broken by Ecmascript's use of [[Define]] for properties. And Typescript's primary goal is to type Javascript, not to emit slightly-improved Javascript.

We delayed the switch to [[Define]] as long as we could — Typescript normally adopts proposals at stage 3 — but when class fields hit stage 4 and I found that the big code bases that we surveyed only mixed access/property overrides by mistake, we decided that the best option was to prevent future code from relying on [[Set]].

Some of the code bases that I'm aware of that are heavily OO:

• VS Code
• Office (Web)
• Azure (UI)
• Teams
• Chrome devtools
• Firebase (not sure if this qualifies as 'heavy')

My Google and Bloomberg contacts didn't share the projects that they surveyed, but based on Bloomberg's Typescript contributions, I assume they use OO heavily.

[canonic-epicure]

@sandersn Ok, thanks for the update.

[Mahi]

If we're being pedantic, this change singlehandedly prevents everyone from ever using public properties in any of their classes ever again, because it prevents others from ever extending their classes with new behaviour. When you're designing a class, it's impossible to know what someone else might wanna extend it with 10 years later. This change throws us back to the C++ hell of having useless public getters and setters for every attribute we'll ever make, just in case someone else might need to override the behaviour in the unforeseeable future.

Worst change ever made in the history of the TypeScript repository. Other languages are fighting to implement property overriding, while TypeScript decided to proactively prevent it.

@sandersn

[BertrandRenault]

It's for sure a VERY annoying issue, which prevent me to upgrading to typescript 4+, which is VERY annoying in itself for compatibility issue with various others libs. It's quite weird to argue that it's a better design, and that all code pre TS4 that overide prop as accessor is now wrong... It was common and usefull practice. TS2611 is just a pain...