Design Meeting Notes, 8/13/2024

[DanielRosenwasser]

Ordering Parameter Properties and Class Fields

#45995

• When we adjusted our class field emit (with useDefineForClassFields), we also adjusted the relative ordering of class fields and parameter properties.

• Under JS, class fields run before the body of the constructor.

• Under TS, previously, class fields ran after parameter properties, but before the body of the constructor.

• Under useDefineForClassFields we error on any access to parameter properties, but otherwise it's fine.

• Should we adjust the ordering to be consistent?

• useDefineForClassFields was introduced as a legacy behavior to avoid breaks, why break people?

  • Could deprecate setting it to false.

• We could run a PR where it always errors, see what breaks.

  • Is there an argument for making things more spec-compliant over time?

• Is there something clever we could do with emit?

  • Witnessable side-effects.

• The tough part is that most codebases that would be affected by running on GitHub won't be uncovered. Typically these are more mature codebases.

• Could always do a codemod. We typically rely on quick fixes here.

  // Before
  class A {
      x = 1;
      constructor(public y: number) {
          console.log(this.y);
      }
  }

  // After
  class A {
      declare x;
      constructor(public y: number) {
          this.x = 1;
          console.log(this.y);
      }
  }

• Some of us feel like it's a good idea to get consistent emit. Picking a date in the future to deprecate and eventually remove is the proposed plan. But need to experiment first.

• Maybe 6.0 deprecation is the plan.

• This has only been available since ES2022 though!

• 6.0 deprecation = 6.5 removal, which is just over 2 years from now.

• Aside: we should investigate parameter properties in ECMAScript.

[robpalme]

Thanks for discussing this!

The original issue highlights inconsistency for target emit across ES2021 and beyond.

The notes here suggest a solution of deprecating the ability to set useDefineForClassFields: false. That sounds great.

Taken literally, that would provide consistency for target emit >= ES2022 but would leave everything earlier defaulting to the legacy behavior.

Is the suggested deprecation intended to go further and imply that useDefineForClassFields: true would become universally true? Because that would resolve the inconsistency mentioned in the original issue. (This is a clarifying question not intended to advocate for a particular answer.)

[justinfagnani]

If parameter properties might be deprecated, is there a canonical reference for that idea? And is it related at all to other non-standard runtime features like namespaces and enums (outside of their overlap with https://github.com/rbuckton/proposal-enum)?

I ask because it came up in a discussion about Node's recent --experimental-transform-types PR where the author was worried that not supporting these runtime features would create a new flavor of TypeScript. If a future version of TypeScript would also not include them, then TypeScript - runtime features isn't so much o a new flavor, but a future flavor. It might be helpful for new tools without backwards compatibility constraints to be able to target such a flavor.

[rbuckton]

If parameter properties might be deprecated, is there a canonical reference for that idea?

Parameter properties are not deprecated. What we are considering deprecating is useDefineForClassFields: false, which would also happen to address the initialization order problem.

We have some interest in investigating parameter properties as a TC39 proposal in the future.

[justinfagnani]

@rbuckton ah, thanks for the clarification

[rbuckton]

Is the suggested deprecation intended to go further and imply that useDefineForClassFields: true would become universally true?

Yes, that would be the intention. It should be noted that even under useDefineForClassFields: true you can approximate useDefineForClassFields: false by using a declare field and moving the initializer to the constructor body, so there would still be a workaround for specific cases.

[NicBright]

I must say I'm shocked! This all reads as if you're willing to break a lot of codebases! Please see my feedback below, which is from the point of view of a TS power user who's been working on a very large (and private) Angular powered code base (owned by a Swiss insurance company) starting with TS 2.x
In my feedback, I will make the argument, why your recently conducted experiment should NOT be used as guidance as the numbers are just utterly wrong.

Under useDefineForClassFields we error on any access to parameter properties, but otherwise it's fine.

I suppose you're referring to #55028?
No, it's not fine, because immediately executed functions are not caught be the compiler, see my "Final Note" here: #55132 (comment)
For example, this affects any Angular project using NgRx with Effects as per the docs by listening to the actions$ observable (which is injected via parameter property). See https://ngrx.io/guide/effects#writing-effects
These errors will NOT be caught by the TS compiler and only emerge at runtime. That's reason No. 1 why your experiment is inaccurate.

When we adjusted our class field emit (with useDefineForClassFields), we also adjusted the relative ordering of class fields and parameter properties.

This is a HUGE breaking change for a lot of projects that are using Constructor Dependency Injection in combination with parameter properties in case they want to access the injected services during instantiation (which is a common thing to do with RxJS and/or NgRx and I'm sure lots of other libraries as well) - the most common case being to avoid using methods that return distinct object identities every time they're called. Instead it's often desirable to have a single fixed object (e.g. observable) assigned during instantiation that will not mess around with change detection.

By changing the order (and planning to deprecate any way to switch back to the old order, except for quirky workarounds, see below) you're effectively deprecating Constructor Dependency Injection, as Angular (e.g.) used to have it since the very beginnings.

This brings me to reason No. 2 why your experiment is inaccurate:
In your experiment (if I'm not mistaken), you're just looking at Github projects. These are very far away from a realistic and accurate statistic sample because of these reasons:

1. most enterprise codebases are usually private - you'll not find them on Github
2. I assume that the vast majority of impacted projects will be single page applications that use Constructor Dependency Injection with parameter properties. Those will be private (enterprise) projects NOT hosted on Github.
3. In contrast, on Github, most projects (especially the "top 1000" you wanted to look at in your experiment) are most likely libraries of some kind that do NOT use Constructor Dependency Injection in combination with "access-while-instantiating".
4. And even if those projects had been affected in the past (the bug has been around for two and a half year now), they will most likely already have adopted their code to work around that breaking change (because top 1000 open source projects can be assumed to be well maintained). So looking at these (top 1000) well-maintained projects now will not tell you anything about how many projects will be affected, that are not so well maintained.

One might be tempted to ask now: if there are so many private enterprise projects that could run into problems: why haven't we got more people participating in reporting this? I can think of two reasons why:

1. I think that most private enterprise codebases are not that well maintained when it comes to their tsconfig.json switching to target: es2022 or newer. Either because of sloppiness, a lack of resources, lack of importance ("ES2021 and below will continue to run, so why change (now)?") or because the older target is still needed to support older environments.
2. Those projects that did make the switch, might have "worked around" the ordering issue by just using useDefineForClassFields=false.

useDefineForClassFields was introduced as a legacy behavior to avoid breaks, why break people?
. Could deprecate setting it to false.

No. I see it differently: It has been introduced (with TS 3.7, Nov 2019) as a preparatory means to allow people to adhere to the upcoming standard. It was meant for users to actively set it to true (false being the default). No user at this point in time would have thought that the order of execution would be changed in a breaking way. Starting with target: es2022 (introduced Nov 2021), it was assigned additional semantics: suddenly, setting it explicitly to false brought back the old behavior. Users have silently adopted to this: because of the resulting bugs, e.g., the Angular team used to add useDefineForClassFields=false to the tsconfig.json that their @angular/cli's ng new generates (up until version 17, only since 18 (which was released only 3 months ago) they're no longer adding it at all (which is a poor decision in my eyes, more about this in my final remark below). Inside their @angular/cli tooling, which is also responsible for building an Angular application (or library), they even patch their user's tsconfig.json to be able to use target: es2022 internally (they only patch it when the target is missing or below es2022). To do so, they're also silently adding useDefineForClassFields=false to their user's tsconfig.json, to work around the emit order breakage (see webpack tooling, esbuild tooling <-- in both files they're even mentioning the root issue #45995).

Besides that: Exactly! Why break people? Why breaking my "legacy" code base that's relying on the order of execution being "parameter properties" first, class fields second, constructor body third.

Could always do a codemod. We typically rely on quick fixes here.

// Before
class A {
    x = 1;
    constructor(public y: number) {
        console.log(this.y);
    }
}
// After
class A {
    declare x;
    constructor(public y: number) {
        this.x = 1;
        console.log(this.y);
    }
}

I'm not sure. Is this your proposed solution for old codebases to work around that breaking change of removing useDefineForClassFields=false? If so: do you really think that this is a good developer experience (DX)? This solution might be a good fit for the RARE cases related to bugs caused by the semantic change going from [[Set]] to [[Define]] semantics. However, it is not well-suited if you want to address the ordering issue that will occur much more frequently by one or two orders of magnitude.

Some of us feel like it's a good idea to get consistent emit. Picking a date in the future to deprecate and eventually remove is the proposed plan. But need to experiment first.

Here's an alternative plan that's far more DX friendly:

• Please rethink about whether TS has led a LOT of projects onto a path where they're expecting parameter properties to come FIRST and class fields SECOND. Right now you all seem NOT to think so. I do, however, and there's a simple reason for that: Constructor Dependency Injection as promoted by Angular
• If you really plan to continue to support parameter properties, please accept, that they MUST be executed FIRST, because otherwise Constructor Dependency Injection is losing a lot of its DX: basic injection will still work, but "access-while-instantiating" will be no longer possible except with the quirky workaround of doing it in the constructor (combined with separate declare line) rather than on a single-line field declaration. This workaround by the way defeats the very thing you are trying to achieve: allow users to use ES class fields (which I support, but I want to use them and be able to initialize them with stuff that's injected into the constructor via Dependency Injection).
• A good way to go forward could be:
  • Everything's good when parameter properties are not used. No need to change anything then.
  • As soon as parameter properties are used, because the user explicitly wants to use this feature, it is far more important to provide the "old" order rather than the new. Parameter properties in fact, is such a great feature (at least until recently) that it should be considered a feature that's worth to try to incorporate into ES standard. If you look at it this way, it is perfectly fine to make an exception to the "new" ES class fields emit and continue to have them execute FIRST and make ES class fields only come SECOND.

Final remark:

Angular has been somewhat promoting the new inject() method since v14.2 which is an alternative to Constructor Dependency Injection. They're not promoting it to the extreme, but they're using it now in their examples over the Constructor Dependency Injection way, presumably because the examples are more concise and thus easier to understand. To me it seems, that they're willing to drop support for Constructor Dependency Injection (although nobody officially said that -- that's why I'm also thinking that they might not even be aware of the problems themselves, it's so weird!). In my understanding, there are three reasons for inject() being promoted like this:

1. they wanted to get rid of class based route guards (and the like), starting with [email protected] (and allow users to use simpler functional route guards)
2. (this is just an assumption of mine) they might have been in fear that the breaking change in the emit order might not be resolved in a good way (i.e. by bringing back the old behavior) - they've been definitely aware of the breaking change, which the code from above proves (see webpack-tooling + esbuild-tooling links above).
3. they might be fearing, that the proposal ECMAScript Decorators for Class Method and Constructor Parameters might not make it and thus that they might be losing support for their constructor dependency injection decorators (@Inject, @self, @host, @SkipSelf, @optional) as soon as TS drops support for experimentalDecorators. @rbuckton maybe you could talk to the Angular team to clarify this?

If nothing changes (i.e. if you stick with the described plan of deprecating useDefineForClassFields=false), I can predict with 90% certainty: Constructor Dependency Injection as we know it today will vanish. This would be really sad, as it has pros and cons over inject() based dependency injection (and to me, the pros overweigh the cons by far). If you're interested, I can write a little bit more about the pros and cons but I think right now it's not that important for the discussion.

[justinfagnani]

In order to execute parameter properties before class field initializers, all class field initialization would have to be transformed to be done in the constructor after the fields induced by parameter properties are set. That could work with decorators as long as decorators are downleveled, but removing the initializer expressions would be visible with native decorators, so using parameter properties would mean forever downleveling decorators. Those are big emit changes to have happen because you use parameter properties.

And if parameter properties were standardized, there's no guarantee that they would have the same ordering wrt class fields as TypeScript.