Honour class property values set by parents

[brianmhunt]

It is a common pattern to have a parent class that sets the properties of children, but the current TC39 spec introduces a problem with this pattern: children properties are set to undefined.

Just one illustrative example (in Typescript, since the types aid the illustration):

abstract class Parent {
  constructor (params) {
     for (const prop of this.observables) {
        // for some observable e.g. ko.observable, mobx, etc., or other "decorator"
        this[prop] = observable()
     }
  }
  abstract get observables () : string[]
}

class Child {
  a: Observable<string>
  b: Observable<number>

  get observables () { return ['a', 'b'] }
}

c = new Child()
c.a === undefined // 🚨

The current spec 2.7 DefineField(receiver, fieldRecord) at 2.7.6 mandates that properties a and b be assigned at the construction of Child the value undefined.

Based on my own experience, this behaviour obscures bugs that can be subtle and difficult to debug. If introduced into e.g. Typescript this would have the potential to break a lot of code.

This problem would be avoided if a class does not assign undefined if the parent has already assigned a value (i.e. the property exists). I feel quite strongly that this is the behaviour most developers expect.

If one wishes to obscure properties from parents, it could be explicitly done with a new keyword such as protected or own i.e.

class Child {
   a: Observable<string>
   own b: string
   get observables() { return ['a', 'b'] }
}

c = new Child()
typeof c.a // Observable
typeof c.b // undefined

I noted that this is the behaviour honoured by @babel/plugin-proposal-class-properties, and that it's already been identified there as an issue https://github.com/babel/babel/issues/8280 (closed on the basis that it's in compliance with this spec).

[ljharb]

This seems like it might be related to https://github.com/tc39/proposal-class-fields#public-fields-created-with-objectdefineproperty?

[jridgewell]

This is typescript not matching spec semantics.

[brianmhunt]

@jridgewell Typescript is just used here for illustration; the actual culprit appears to be @babel/plugin-proposal-class-properties ; if they've mis-interpreted the spec, maybe we should file an issue referencing babel/babel#8280

Here's an example in Babel:

class Parent {
  constructor () {
    this.observables.map(o => this[o] = 1)
  }
}

class Child {
  x
  get observables () { return ['x'] }
}

gets turned into

..
  function Child() {
    _classCallCheck(this, Child);

    _defineProperty(this, "x", void 0);
  }
...

Your expectation that Typescript doesn't match the spec semantics supports the belief that most developers won't expect this.

[nicolo-ribaudo]

What's the reason for explicitly declaring x in the child class?

[ljharb]

I’m also not clear on how common this pattern is - I’d find it very strange to see any parent class that depends on knowledge of its children.

[brianmhunt]

@nicolo-ribaudo it's mandatory in Typescript for type-definitions, but also needed for e.g. property decorators.

[nicolo-ribaudo]

That is a typescript problem (for which I have proposed a solution a few months ago: https://github.com/microsoft/TypeScript/issues/12437#issuecomment-444010614).

Also, a decorator can easily remove the property if it shouldn't actually be initialized by the class constructor.

[brianmhunt]

@ljharb it's an abstract factory; it's routinely used in database/datastore models, view models for observables. I use it in Knockout.js/tko all the time (I help maintain Knockout), but I know it's also quite common with mobx and other libraries that have hundreds of thousands of users.

[shannon]

One use case I would use to explicitly declare x in this case is readability and documentation:

class Child {
  /**
   * The x field description
   */
  x;
  get observables () { return ['x'] }
}

I’m also not clear on how common this pattern is - I’d find it very strange to see any parent class that depends on knowledge of its children.

A similar pattern is used in lit-element: https://lit-element.polymer-project.org/guide/properties#declare

[brianmhunt]

@nicolo-ribaudo It seems [Typescript itself doesn't exhibit this problem](https://www.typescriptlang.org/play/index.html#src=abstract%20class%20Parent%20%7B%0D%0A%20%20constructor%20(params)%20%7B%0D%0A%20%20%20%20%20for%20(const%20prop%20of%20this.observables)%20%7B%0D%0A%20%20%20%20%20%20%20%20%2F%2F%20for%20some%20observable%20e.g.%20ko.observable%2C%20mobx%2C%20etc.%2C%20or%20other%20%22decorator%22%0D%0A%20%20%20%20%20%20%20%20this%5Bprop%5D%20%3D%20observable()%0D%0A%20%20%20%20%20%7D%0D%0A%20%20%7D%0D%0A%20%20abstract%20get%20observables%20()%20%3A%20string%5B%5D%0D%0A%7D%0D%0A%0D%0Aclass%20Child%20%7B%0D%0A%20%20a%3A%20Observable%3Cstring%3E%0D%0A%20%20b%3A%20Observable%3Cnumber%3E%0D%0A%20%20%0D%0A%20%20get%20observables%20()%20%7B%20return%20%5B'a'%2C%20'b'%5D%20%7D%0D%0A%7D%0D%0A%0D%0Ac%20%3D%20new%20Child()%0D%0Ac.a%20%3D%3D%3D%20undefined%20%2F%2F%20%F0%9F%9A%A8); it's only when coupled with @babel/plugin-proposal-class-properties

[jridgewell]

One use case I would use to explicitly declare x in this case is readability and documentation:

That's a good argument for doing it.

It seems Typescript itself doesn't exhibit this problem; it's only when coupled with @babel/plugin-proposal-class-properties

Typescript uses non-standard behavior. Babel is following the spec behavior, so you're hitting it there.

[trotyl]

It seems Typescript itself doesn't exhibit this problem; it's only when coupled with @babel/plugin-proposal-class-properties

@brianmhunt TypeScript already claimed to fix this (making it undefined own property), see https://github.com/microsoft/TypeScript/issues/27644#issuecomment-457652059 for more information.

[brianmhunt]

I hadn't realize this was a topic discussed ad nauseam already, but I think @mbrowne correctly assesses it:

As best I can tell, the main argument in favor of Define is avoiding side effects, meaning that declaring a public field should always do one and only one thing: define an own property of the instance. It seems that the distinction between declarative and imperative syntax is important here; field declarations are intended as declarative syntax, so side effects such as a getter or setter being called (which would be acceptable in an imperative setting) are undesirable. What I don't think has been demonstrated is that there are significant practical downsides to Set, or at least none that come close to the significance of the practical downsides of Define, i.e. potentially confusing issues for developers and resulting bugs.

This is mirrored by the comments @RyanCavanaugh wrote on the linked issue:

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

I'm here because implementations have trickled down & out into the developer tools and this issue is causing significant real-world pain. I feel the reservations expressed by @mbrowne and @RyanCavanaugh are on-point, in which case I am a canary in the coal mine, and lots more pain is coming. 🦜

To speculate on the opposite scenario, if Set had been chosen instead of Define, I suspect this wouldn't even be a point of discussion (much less debate).

I'm not sure what the standard is for modifying a proposal at this stage, but I think this is one worth giving a good, hard look if it's not too late.

[RyanCavanaugh]

I'm not sure what the standard is for modifying a proposal at this stage, but I think this is one worth giving a good, hard look if it's not too late.

Nothing's changing, because you can't evidence someone out of a position they didn't evidence themselves into. The committee looked at 6+ years of TypeScript developers using [[Set]] semantics with ~zero complaints and picked [[Define]] instead. Edit: Same for whether bare declarations should do anything or not.

[bakkot]

If I understand correctly this has nothing to do with [[Set]] vs [[Define]]. The parent class does not have accessors or a configurable + nonwritable property for the relevant fields, so there's no difference (ignoring quibbles about Object.prototype).

[jridgewell]

@bakkot is correct here. Both set and define semantics would have overridden the super class’s prop with the subclass’s.

The issue is only with declaring a field and expecting it to do nothing.

[mbrowne]

I think maybe it makes sense for TypeScript to act differently here since in this case you're declaring a property just for the purpose of giving it a type annotation. But certainly in plain JS, I think it's usually an anti-pattern to re-declare a property already present in a parent class when all you're doing is inheriting it. IMO properties should generally only be re-declared in subclasses when you're overriding the default value.

[rdking]

The problem is a compile-time vs run-time issue. Where Typescript can skip the issue (barring the presence of the proposal plugin) because it has compile-time knowledge of the structure of the derived classes, native ES (and babel's proposal plugin) only apply the subclass' fields to the instance after the base class constructor has run its course.

There's only 2 semi-reasonable ways out that I can think of:

1. Move the fields to the prototype (Don't bother arguing. I know why most think this is not acceptable.)
2. Apply all fields from all levels of the inheritance chain before running the base class's constructor logic.

Given the example in the OP, this would mean that Child.a and Child.b would be applied to this before the for loop in Parent.constructor gets run. As a result, the code would operate as expected, and the proposal would only receive a minor timing tweak.

[ljharb]

It’s an important invariant imo that the parent constructor doesn’t get to run any code after super() returns to a child constructor, which includes installation of instance fields.

[shannon]

It's not clear to me, would this code work?

class Parent {
  constructor () {
    this.properties.map(o => this[o] = 1);
  }
}

class Child extends Parent {
  x = this.x;
  get properties () { return ['x'] }
}

const c = new Child();
assert(c.x === 1);

[jridgewell]

Yup, that’ll work.

[mbrowne]

One thing that might help here would be an ESLint rule enforcing what I said above; i.e. this would be an error:

class Base {
    x
}
class Sub extends Base {
    x  // cannot re-declare `x` without giving it a different default value
}

but this would not:

class Base {
    x
}
class Sub extends Base {
    x = 1
}

I think the following would also be OK, since it's explicit, although I'm not sure why you'd need to do this (maybe if you were using Flow to give it a type annotation?):

class Sub extends Base {
    x = undefined
}

In any case, it's important that field declarations without default values get set to undefined:

class AClass {
    x
    constructor() {
        // this should output ["x"]
        console.log(Object.keys(this))
    }
}

[rdking]

@ljharb The parent constructor wouldn't be running any such code after super returns. That would still be very much impossible. Here's an example. Suppose you had the following classes:

class PointBase {
  constructor() {
    //does something useful
  }
  //Some generic point-based methods
}

class Point2D extends PointBase {
  x = 0;
  y = 0;
  constructor() {
    super();
    //does something else useful
  }
  //Some 2d point-specific methods
}

class Point3D() extends Point2D {
  z = 0;
  constructor() {
    super();
    //more useful stuff
  }
  //Some 3d point-specific methods
}

What I've suggested is that new Point3D() should generally behave like this:

1. Construct Point3D
   1. call super()
      1. Construct Point2D
         1. call super()
         2. Construct PointBase
            1. create object instance
            2. apply PointBase fields (public and private)
            3. apply Point2D fields (public and private)
            4. apply Point3D fields (public and private)
            5. return control to PointBase constructor
            6. return constructor retval or this
         3. return from super
         4. return control to Point2D constructor
         5. return constructor retval or this
      2. return from super
   2. return control to Point3D constructor
   3. return constructor retval or this
2. return control from constructor.

The difference here is that instead of fields from Point2D and Point3D being applied immediately after the super call in their own constructors, those fields would be applied immediately following the creation of the instance object by the base class before the base class constructor's code is executed.

[ljharb]

Applying the child fields that way is running child code before the parent constructor is done.

[ljharb]

Also note, each constructor could choose to return a new object in which the child’s fields should instead be applied.

[rdking]

@mbrowne The ESLint rule, while helpful in its own right, wouldn't solve the problem presented by the OP. In the example @brianmhunt gave, a and b are fields of Child, not Parent. As such, the ESLint rule would have no effect on that scenario.

[rdking]

@ljharb Is it because the field initializers are themselves functions that depend on the current state of the active instance? In that case, I agree with your assessment that the idea shouldn't be allowed, but with caveat. It falls back to what IMO was a bad design decision regarding initializers. I still believe that initializers should be run at declaration time instead of at construction time. That would've mitigated the argument you're making.... but I get that one of the goals was elimination of explicit constructors. Not fond of that goal either.

In either case, you're left with a situation that is otherwise unsolvable for the OP. No constructor of any ancestor class will ever be able to indirectly manipulate all fields of the instance. That's just a design flaw(trade-off).

[ljharb]

That is indeed the design decision, which matches how pre-es6 inheritance works.

[rdking]

To an extent, yes. However, pre-ES6 inheritance also commonly accommodated data properties on prototype objects. The mere existence of the knowledge that putting an instance-specific object on a prototype property is a foot-gun is proof of this.

I said that to point this out: the design decisions that were made were neither the best, nor even among the best of the available options, not for class, and now not for private state. The pain point raised by the OP will be just one of many that will stand as proof of this fact. While it will probably fall on deaf ears, I plead with the board to reconsider this design. There are more options out there that will prove more technically suitable and language amenable. They just need to be found and considered outside the light and pressure of the current proposal.

[rdking]

Off topic clarification of the previous post

Just a note: I'm not trying to incite an off-topic discussion, it's just that the phrase: > which matches how pre-es6 inheritance works. seems to imply that the design decisions were based on the **only way** pre-ES6 inheritance works, when the reality is that ES6 inheritance has multiple modes, most of which depend on the prototype object, which has never been required to only contain methods. Nor has it been exclusively used in that way by the community. While the doors may be closed over the questionable but somewhat reasonable choices regarding `class`, that in no way should be seen as a reason to make more questionable, and somewhat less reasonable decisions about how to implement `private`. Note that the definition of reasonable is subjective, and mine greatly differs from TC39 on some of the issues of this proposal. This is not at all to call fault against the board, only to state that I cannot agree with many of their decisions purely on logical and technical grounds, and that I have only seen a few discussions that managed to weaken my view, but none to turn it.

[hax]

this has nothing to do with [[Set]] vs [[Define]]

Common point: They both ask for modification of semantics which TS users used and relied several years. And they both cause breaking change which is subtle to caught but may bring significant bugs. Currently it only make the TS maintainters' life harder, when the new version with breaking change landed, it will affect all TS users and millions of lines of code.

[hax]

Yup, that’ll work.

Yup, we just invent a new weird JS "pattern" (x = this.x) which make infamous this.x = this.x.bind(this) looks not so weird. 😜

[hax]

might help here would be an ESLint rule

ESLint can't save us for cross-modules case. TS may help. But even TS can not save us in define/set case, this is why TS is forced to change d.ts to emit getter/setter instead of prop. Modifying d.ts will be a very very very very big breaking change for the whole TS ecosystem.

[littledan]

We in TC39 are aware of this mismatch, and we discussed it extensively since 2016. There were various proposals made to smooth over this issue, but ultimately, the decision made (supported by the TypeScript representative at the time) was that it's more important for field declarations to have simple, reliable, consistent semantics than that they match TS semantics for this case.

[rdking]

@littledan If TS semantics were the only consideration of note in favor of [[Set]] semantics, even I would be inclined to dismiss that argument. However, what of the design goal contradictions? One of the biggest value-adds provided by fields is the ability to eliminate constructors that only set initial field values. Correct me if I'm wrong, but that was also a design goal, yes? Since most developers who use class constructors for that purpose don't use Object.defineProperty(this,...) unless they're defining an accessor property based on constructor parameters or other input data, the use of [[Define]] in setting the value of the property will lead to surprising and undesirable results.

In the end, either this should be reconsidered, better explained, or TC39 needs to stop touting constructor elimination as a feature of this proposal. The simple fact is that this proposal leads to different results than using the constructor approach. That's regardless of TS semantics, and yet is in favor of it all the same.

---

This is another case where we're constantly told what:

There were various proposals made to smooth over this issue, but ultimately, the decision made... was that it's more important for field declarations to have simple, reliable, consistent semantics...

when the best way to help everyone accept is to explain "Why?". Let me help a little with a direct question:

Why is it more important for field declarations to have "simple, reliable, consistent semantics" despite the numerous issues this brings to well known, widely used programming paradigms in ES?

[mbrowne]

I just wanted to note (a little late) that I think we're getting off-topic here. It was established earlier in this thread that the issue in the OP would still occur using either Set or Define.

[rdking]

Off topic for the initial post? Yes. However, the original poster changed the direction of the conversation in this thread himself: https://github.com/tc39/proposal-class-fields/issues/242#issuecomment-497386551. Unless I missed something, everything beyond that point has been inline with that new conversational direction.

[brianmhunt]

@rdking This is true, with the caveat that the direction change was on the basis that using Set solved the problem in my original post, in part on the understanding that this is how Typescript behaves and Typescript does not exhibit this issue. If the problem continues to exhibit with Set in Javascript, then that would be an important clarification & consideration.

[bakkot]

@brianmhunt Yes, as I say above, this has nothing to do with Define vs Set. The relevant difference between TS and JS here is that TS treats the a as being essentially a comment, with no effect on runtime semantics, whereas JS treats it as a declaration of a field initialized to undefined.

[RangerRick]

Just popping in here to point out that the babel + typescript + subclass issue just referenced in here is from my code, doing what I imagine is a not-uncommon thing: an abstract class calling a method during construction that's overridden in the subclass to create local state. Then that local state gets blown away when the subclass's own construction happens. I have nothing constructive to add here other than that is my use case and seems like a valid one to include in the discussion.

The funny part is, it happens to work with the actual tsc generated code, but breaks in Babel's version of it, which is apparently in this case hewing closer to the spec?

[brianmhunt]

As a matter of interest for future readers, it seems one can work around this with Babel & Typescript by changing the order of the plugins to put Typescript ahead of the class-properties i.e. in the Babel config:

  plugins: [
    "@babel/plugin-transform-typescript",
    "@babel/proposal-class-properties",
  ]

The @babel/typescript-preset cannot be used since Babel plugins run before presets.

[mbrowne]

I think it would also work to just use the loose flag for @babel/proposal-class-properties, which tells it to use the legacy behavior that matches TypeScript:

    plugins: [
        ["@babel/plugin-proposal-class-properties", { loose: true }]
    ]

[mbrowne]

As an aside, there's an open issue about adding the loose flag to the official starter template for TypeScript using Babel: https://github.com/microsoft/TypeScript-Babel-Starter/issues/39

...although maybe the better solution would be to make it part of @babel/preset-typescript itself. Ideally it would be be good to make people more aware of the native behavior, but the more compelling concern here is that compiling with Babel ideally should not give different results than compiling with tsc (beyond the edge cases already listed in the docs)...

[mbrowne]

It looks like @ljharb left a comment but then deleted it, so I won't reply to that, but I did want to share my findings from trying out some things with the latest production release of tsc...

First of all, the example in the OP doesn't compile with strict mode enabled. If you disable strictPropertyInitialization then you can compile it; here's a very minimal example showing that TS property declarations without initializers do not result in actually creating a property and setting it to undefined as would happen natively. Rather, the declaration simply has no effect:

class A {
    x: string
}

transpiles to:

"use strict";
var A = /** @class */ (function () {
    function A() {
    }
    return A;
}());

TypeScript also still transpiles using Set, not Define:

class Parent {
    set x(value: number) {
        console.log(value)
    }
}

class Sub extends Parent {
    x = 1
}

Here's how it transpiles the Sub class:

var Sub = /** @class */ (function (_super) {
    __extends(Sub, _super);
    function Sub() {
        var _this = _super !== null && _super.apply(this, arguments) || this;
        _this.x = 1;
        return _this;
    }
    return Sub;
}(Parent));

So the semantics are definitely different than the semantics of this proposal.

It's a bit off-topic, but I also wanted to note that the latest version of create-react-app sets loose: true for @babel/plugin-proposal-class-properties—not only for TS projects, but for JS projects as well. I think that if anything, loose mode ought to be enabled only for TS projects (unless TS changes their default semantics), not for new JS projects.

[ljharb]

I deleted my comment, because TS doesn’t yet provide a way to generate compliant output. Once they do, that’s what should be the default.

[mbrowne]

Sorry for the multiple posts in a row, but I just realized that these differences could cause some significant confusion for those who were previously targeting ES5 but now would like to compile to ES6+, which will become increasingly common as people drop IE support (which is already happening of course). So it's good to hear that TS will be changing their (hopefully default) output to match the spec.

[sepehr]

This behavior has caused us a lot of headaches, too.

As a developer with most of my OOP background in Python and PHP, I personally found this behavior unintuitive and misleading. I've seen numerous support questions in the Babel issue tracker regarding this behavior or its side effects, probably because of the likewise mindsets out there.

I also second that this issue is not related to [[Set]] vs. [[Define]]. It happens with either loose: true or false. To me, it's also not primarily about the undefined initial value per se; it's mainly about the "execution order of child class property initializations and the parent constructor".

// @babel/plugin-proposal-class-properties with either loose: true or false.
//
// Order of execution:
//
// 1. Parent property initializer
// 2. Parent constructor
// 3. Child property initializer

class Super {
  prop = 'Parent property initializer'

  constructor () {
    this.prop = 'Parent constructor'
  }
}

class Derived extends Super {
  prop = 'Child property initializer'
}

instance = new Derived();

console.log(`Final value => ${instance.prop}`)
// Outputs: "=> Child property initializer"

While in Python and PHP the output would have been "Parent class constructor" instead. Checkout these repls:

• Babel
• PHP
• Python

As far as I can see, the undefined case @brianmhunt has raised originally, is only a side-effect of this behavior.

To put it into perspective for myself, I did a bit of experiment with other languages as well; I guess I gotta broaden my horizon!

• Java
• C++11
• Swift
• Typescript

[ljharb]

@sepehr what you're describing is true prior to class fields; it seems like an inherent difference in how inheritance works in JS and some other languages. Parent construction code is finished before child constructor code gets any access to the instance.

[hax]

The behavior of Java and C++ is not because the construction/initialization order, but the semantic: the field of Super and Derived are two fields (storage) at all (even they use same name)

[mbrowne]

It's only one property in JS also (except for private fields, but we're discussing public fields...private fields aren't properties at all).