[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 #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?"