Incorrect class #private fields initialization order with target ES2022 and useDefineForClassFields set to false

[Michsior14]

Bug Report

🔎 Search Terms

ES2022 class field initialization order
private fields order
useDefineForClassFields false

🕗 Version & Regression Information

• This changed between versions 4.9.5 and 5.0.4

⏯ Playground Link

5.0.4 with runtime error (useDefineForClassFields: false, target: ES2022)

4.9.5 without runtime error (useDefineForClassFields: false, target: ES2022)

💻 Code

class Buzz {
    wow = 'one'
}

class Foo { 
    #bar = this.buz.wow;
    constructor(private buz: Buzz) {}
}

new Foo(new Buzz())

🙁 Actual behavior

With 5.0.4 and useDefineForClassFields: false, target: ES2022 the above code fails in runtime because #bar = this.buz.wow; stays initialized before constructor, this doesn't happen if "normal private" is used (private bar = this.buz.wow;). Such construct works without issues on 4.9.5.

🙂 Expected behavior

The initialization is done in the same way as on 4.9.5.

[Michsior14]

Still an issue in 5.1.3.

[Josh-Cena]

I don't think this is supposed to work. The fact that private fields were transpiled to an assignment inside the constructor seems like a bug, because useDefineForClassFields is only supposed to affect public properties (which can be shadowed by the base class and therefore suffers from the define vs. set issue). Private fields should always be initialized before the parameter properties, solely by how scoping works.

[Michsior14]

I don't think this is supposed to work. The fact that private fields were transpiled to an assignment inside the constructor seems like a bug, because useDefineForClassFields is only supposed to affect public properties (which can be shadowed by the base class and therefore suffers from the define vs. set issue). Private fields should always be initialized before the parameter properties, solely by how scoping works.

That's fine as long as there is an error produced on compilation time. Having this quietly passing produces a lot of hard to understand issues on run-time.

[Michsior14]

Any updates on compilation errors / order fix?

[Michsior14]

Same on 5.1.6.

[tonivj5]

I don't think this is supposed to work. The fact that private fields were transpiled to an assignment inside the constructor seems like a bug, because useDefineForClassFields is only supposed to affect public properties (which can be shadowed by the base class and therefore suffers from the define vs. set issue). Private fields should always be initialized before the parameter properties, solely by how scoping works.

@Josh-Cena if this is the expected behavior, I think that, at least, should be a compilation error....

class Buzz {
    wow = 'one'
}

class Foo { 
    #bar = this.buz.wow;
                ^^^_ This is property is uninitialized... or something similar

    constructor(private buz: Buzz) {}
}

new Foo(new Buzz())

[rbuckton]

I don't think this is supposed to work. The fact that private fields were transpiled to an assignment inside the constructor seems like a bug, because useDefineForClassFields is only supposed to affect public properties (which can be shadowed by the base class and therefore suffers from the define vs. set issue). Private fields should always be initialized before the parameter properties, solely by how scoping works.

I think this is supposed to work, but we didn't handle this case correctly when private fields were introduced. The ideal solution would be for parameter properties to work with both private fields and --useDefineForClassFields. For such a feature to work correctly, we would have to choose between two different behaviors:

1. Parameter properties are initialized before all class fields, or
2. Parameter properties are initialized after all class fields

Prior to the adoption of private names, the TypeScript emit followed (1), which I believe is still the best approach.

If we want to correctly preserve these semantics and this evaluation order in the presence of private names or --useDefineForClassFields, then the correct emit would be to:

1. declare all of the class fields, including private-named fields and those introduced by parameter properties
2. Initialize the fields introduced by parameter properties
3. Initialize the remaining fields in the order they appear in the class body.

This means moving the field initializers into the constructor body:

// source
class C {
  y = this.x;
  #z = this.y;
  constructor(public x) {}
}

// transpiled
class C {
  x;
  y;
  #z;
  constructor(x) {
    this.x = x;
    this.y = this.x;
    this.#z = this.y;
  }
}

[tonivj5]

If we want to correctly preserve these semantics and this evaluation order in the presence of private names or --useDefineForClassFields, then the correct emit would be to:

1. declare all of the class fields, including private-named fields and those introduced by parameter properties
2. Initialize the fields introduced by parameter properties
3. Initialize the remaining fields in the order they appear in the class body.

This means moving the field initializers into the constructor body:

// source
class C {
  y = this.x;
  #z = this.y;
  constructor(public x) {}
}

// transpiled
class C {
  x;
  y;
  #z;
  constructor(x) {
    this.x = x;
    this.y = this.x;
    this.#z = this.y;
  }
}

That's the behavior I expected. Cool to know it's an unexpected breaking change, and this issue is someting fixable 😄

[Michsior14]

5.3.2 still affected

[Michsior14]

I forgot to mention a small workaround I wrote couple months ago. It's possible to detect wrongly defined properties using eslint and this rule:

'no-restricted-syntax': [
      'error',
      {
        selector: 'PropertyDefinition[key.type="PrivateIdentifier"]:has(ThisExpression)',
        message: 'Use `private` modifier until https://github.com/microsoft/TypeScript/issues/54206 is fixed.',
      },
    ],

Hope this helps someone to prevent runtime issues.

[Michsior14]

No change in 5.4.3

[Michsior14]

5.5.2 also affected