Inconsistent parameter properties initialization order depending on the target syntax with useDefineForClassFields

[nicolo-ribaudo]

Bug Report

🔎 Search Terms

properties initialization esnext order initialization fields useDefineForClassFields

🕗 Version & Regression Information

• This is the behavior in every version I tried, and I reviewed the FAQ for entries about classes

Ref: babel/babel#13779

⏯ Playground Link

Playground link with relevant code (ES2021 target)

Playground link with relevant code (ESNext target)

💻 Code

new class Dashboard {
    name = console.log(this.amount);
    constructor(public amount: number) {
    }
}(1);

🙁 Actual behavior

When targeting ES2021 this.amount is initialized first; when targeting ESNext this.name is initialized first.

🙂 Expected behavior

They should be consistent.

[dnalborczyk]

@nicolo-ribaudo esnext (and eventually es2022) ignore useDefineForClassFields as there is no down leveling.

for target: es2021 and useDefineForClassFields: false I would expect the compiler to generate non-ecmascript standard with the wrong property initialization order and using this, and to throw an Error with useDefineForClassFields: true.

I think the name useDefineForClassFields is unfortunately a bit misleading, as it not only uses defineProperty but also fixes the initialization order.

I think useDefineForClassFields should be the default for new Typescript projects (tsc --init) and also be recommended in the docs. People will get a big awakening once es2022 becomes the default and their code all the sudden behaves differently.

[dnalborczyk]

for target: es2021 and useDefineForClassFields: false I would expect the compiler to generate non-ecmascript standard with the wrong property initialization order and using this, and to throw an Error with useDefineForClassFields: true.

that said, if this can be fixed, even if it's essentially "breaking", even better.

[nicolo-ribaudo]

@rbuckton I see that this issue has been added to the 5.1.0 milestone. Is there any update? Asking because we are aligning our behavior with TS in babel/babel#15583.

[nicolo-ribaudo]

We got more issues by users confused about useDefineForClassFields affecting parameter properties, it would be great to move towards a decision for this issue :)

[rbuckton]

If we were building the feature from scratch based on the current version of ES, then I would expect that field initializers would run before parameter property initializers given that field initializers don't have access to the constructor body:

// @target: esnext
// @useDefineForClassFields: true

​class C {
  x = 1;
  constructor(public y: number) {}
}

// becomes
class C {
  x = 1;
  y;
  constructor(y) {
    this.y = y;
  }
}

playground

This order also has the best semantics when combined with native class element decorators as well as the parameter decorators proposal.

Unfortunately, prior to useDefineForClassFields, TypeScript has historically set parameter property initializers first:

// @target: es2015
// @useDefineForClassFields: false

​class C {
  x = 1;
  constructor(public y: number) {}
}

// becomes
class C {
  constructor(y) {
    this.y = y;
    this.x = 1;
  }
}

playground

Since field initializers are allowed to access this, it is possible to write code that would break were we to align the emit order for useDefineForClassFields:

// @target: es2015
// @useDefineForClassFields: false
class C {
  x = this.y.x;
  constructor(public y: { x: number }) {}
}

// becomes
class C {
    constructor(y) {
        this.y = y;
        this.x = this.y.x;
    }
}

playground

Any change we make to emit will need to be carefully considered for the potential breaks it would introduce. So far, we've opted to leave the emit order distinct between the two cases under the premise that useDefineForClassFields: false is essentially a legacy option, especially since we will implicitly set it to true when your target is high enough.

[rbuckton]

Per the design meeting, the current plan would be to add a deprecation for useDefineForClassFields: false in TS 6.0 and remove the flag entirely in 6.5. This would mean that eventually our behavior for pre-ES2022 would be to align with ES2022 semantics for fields, which would also address the ordering, but that will likely be at least two years out.

An initial investigation into the impact of this change was performed in #59623, which only looked for the "use-before-initialization" error illustrated by my third example, above. Only one instance of this error was found in the top 1000 projects, which is a hopeful sign. However, that investigation did not look at other possible issues resulting from side effects.

[JoostK]

As an additional data point, I have applied the change in #59623 to a private repo (~500kLOC) which heavily depends on the "old" initialization order (i.e. useDefineForClassFields: false) and no new diagnostics are reported, so I doubt it's a good proxy to measure the impact of changing the initialization order.

[NicBright]

@JoostK @rbuckton this is how it looks like in our project (around 79k statements according to our code coverage report, approx. 340k LOC (excluding tests) + 497k LOC (tests only - approx. 14k test cases)):

Starting with a 0-error-compiling project, when setting target: 'es2022' + useDefineForClassFields: true we suddenly get 1336 errors in 538 files:

Possible migration paths

Let's explore, what migration would look like for us. I could think of four different solutions. However, any of those is taking a lot of flexibility away and they all have significant drawbacks. It just feels like a big step back regarding JS ergonomics:

1) The Super Class Hack

This is maybe the best option for singleton services that are used in a lot of different places because the consumers don't need to be updated:

2) Conversion Into Functions / Methods

This could be suitable for components and services dedicated to a component. In this case, there will be only few and obvious places that need to be updated:

3) Moving Things Into The Constructor

This will easily lead to constructor bloat, especially if you would like to continue to have TS to infer the type of private (maybe even public) properties (in this case, you can't delegate it to separate private methods, because then, type inference will not work any more). In the screenshot, admittedly, we don't use type inference, but some people clearly prefer it over explicitly adding types to private properties.

4) Switching To Inject() - Angular specific

This is the easiest migration path for projects that use (the really slow) TestBed for everything, even for testing simple services.

It would mean to completeley abandon constructor dependency injection!

This is not an alternative for us because of the testing pains and because it would mean to tightly couple our unit tests to Angular's TestBed (and because we would need 60 test shards then instead of 30 because I'd expect overall test runtime to double).