TypeScript: Handle breaking change in class property runtime behavior

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 declare to mark a field as “type only”

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.

Stage 3: Indicate that further refinement will require feedback from implementations and users

¯\_(ツ)_/¯

@trotyl @mbrowne The corpus I inspected, our internal test repos + the ones at ~/Typescript/tests/cases/user, shows that people rarely override properties with accessors and vice versa. I tested the code with --useDefineForClassFields, which disallows property/accessor and access/property overrides.

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 get, which would crash with [[Set]], so I think those accessors were never used.

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.

We’d have to strip the entire of bar: Bar to not break at runtime

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.

use declare to mark a field as “type only”

I like this quite a bit

as you don’t have to think about whether the superclass has a setter since you’ll always get that property defined.

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 defineProperty. People will mess this up, and important setter side effects or validations are going to get unintentionally clobbered by people who think it’s safe to move a this.p = e assignment in the derived constructor body into the class body because it looks neater.

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.

I wish people on the TS team had argued against it more earlier in the process

The best time to fix a issue was early stage. The second best time is now.

What’s an example of a refinement that wouldn’t break the web? Why does Stage 3 exist at all? Should we just move things to Stage 4 as soon as Chrome puts it out there unflagged?

The TypeScript team can proceed however they like of course, but IMO it would be worth going ahead and beginning work on updating the semantics of TS class properties to match the new ES spec. The PR could be left open since TC39 could still receive new and significant feedback as class fields are used in the real world. But I think the odds are extremely low that the real-world issues with the Object.defineProperty behavior would be so significant and common that they would justify changing the semantics at this point. I’m concerned about compatibility issues between JS and TS here, especially now that many people are compiling TS with Babel.

BTW, as someone who’s not a fan of the Object.defineProperty semantics, I wish people on the TS team had argued against it more earlier in the process, especially those who are on the TC39 committee (although perhaps there were mixed opinions among the TS team). But there were two valid sides to that debate, and at some point we have to accept the spec and move on. Otherwise the differences between TS and JS will probably cause far more issues than would ever have been directly caused by the defineProperty semantics in the first place.

@bigopon Yes, these are the semantics that TC39 chose deliberately in 2016 in collaboration with the TypeScript team.

rather than risking triggering setters on a superclass

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) ?

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

@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 undefined seems like an odd bit of sugar to have when it creates a footgun for derived classes as a trade-off.

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.

I still have concerns about the [[Define]] semantics, especially this case:

class Base {
  x = 1;
}

class Derived extends Base {
  get x() { return 2; }
  set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1

The x field on Base replaces the x accessor from Derived, which breaks inheritance and may be confusing for users.

Given the time remaining for 3.6, I’m moving this to 3.7.

Please, many of us beg, don’t make [[Define]] the default.

@trusktr just another example that i just ran into, related to [[Define]], and maybe someone already mentioned it: href is defined on the instance: It will be a massive mess regardless TS/JS

What it really is expected to be: trigger the setter

The fix is to make it imperative which will invoke the Animal base class accessor.

@robpalme Yes this is the “fix” if follow the [[Define]] semantic. Unfortunately, such fix is not very obvious to most JS/TS programmers, and many see your “correct code” and want to refactor it to the wrong form because they are educated by React. 🙃

There are also many cases, you have no way to “fix”. For example: Base class originally use traditional imperative form, subclass override it to accessors. Then one day the author of base class want to adopt class fields because it’s a new feature and should be wonderful! Remember the author of subclass can not “fix” the decision of base class.

Will this feature be disabled by default, so that [[Set]] semantics are default? It’s going to introduce red squiggly lines into projects if the class fields suddenly are not allowed due to super classes having accessors. 3.6 isn’t a major version bump, you know.

People will accidentally shoot their legs off with [[Define]] gun shells. 😃

For example, I made an oil painting of a gruesome premonition:

class Animal {
    #sound = 'rustling noise in the bushes'

    get sound() { return this.#sound }
    set sound(val) {
      this.#sound = val;
      /* some important code here, perhaps tracking known sounds, etc */
    }

    makeSound() {
        console.log(this.#sound)
    }
}

const a = new Animal
a.makeSound() // 3.14

class Lion extends Animal {
    sound = 'RAWR!'
}

const lion = new Lion
lion.makeSound() // BUG! Expected "RAWR!" but got "rustling noise in the bushes"

It was a bloody mess.

@RyanCavanaugh:

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.

That was my point with https://github.com/microsoft/TypeScript/issues/27644#issuecomment-518543395

I still have concerns about the [[Define]] semantics, especially this case:

class Base {
  x = 1;
}

class Derived extends Base {
  get x() { return 2; }
  set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1

The x field on Base replaces the x accessor from Derived, which breaks inheritance and may be confusing for users.

@rbuckton Using [[Set]] won’t really fix inheritance, but only make the breaking happens to some other people. Say that I need to use throwing getter as abstract property, like:

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 [[Define]], but throwing for [[Set]]. You may argue that TypeScript has abstract modifier to better handle this, but abstract cannot be combined with static, for example:

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 [[Define]] is better than [[Set]], in fact, both of them cause footguns and neither is better than another.

Fair enough.

@ljharb FWIW I don’t think it’s useful to suggest no changes are possible on a stage 3 proposal until the feedback is collected and presented. It may be practically true, but it’s better to be receptive to user/implementer feedback during stage 3 until TC39 has explored the options before them in light of the feedback given.

Mind you I still think fields should be declared with := rather than = to make it clear class Point { x := 10 } is more like o = { x: 10 } rather than o.x = 10, but I think it’s probably too late to change this.

I’ve generally found that depending on subclass setters/getters just tends to make the base class much harder to refactor so // nothing printed doesn’t really bother me.

The other case of console.log(obj.x); // 1 is a bit more concerning, but I don’t really see any way around these sort’ve problems as it’s basically the same as this sort’ve problem (that will happen regardless of [[Set]] vs [[Define]] semantics):

class Base {
  method = () => {
    return 12;   
  }
}

class Derived extends Base {
  method() {
    return 7;
  }
}

new Derived().method(); // 12

Please don’t break people’s code when they update to TypeScript 3.6.

At the very least, having an option like classFieldDefine, even if defaulting to true, would be awesome. I would certainly set it to false.

Something that @littledan brought up last night: the --experimentalDecorators flag currently expects set semantics, not define semantics. We’ll likely have to maintain the old emit with this flag turned on.

P.S.: Keywords for me in the future: set define class fields instance fields properties defineproperty defineproperties

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.