Skip to content

ES2022 Class Decorators not working when Class has self-static member #52004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vladimir-siv opened this issue Dec 23, 2022 · 18 comments · Fixed by #54046
Closed

ES2022 Class Decorators not working when Class has self-static member #52004

vladimir-siv opened this issue Dec 23, 2022 · 18 comments · Fixed by #54046
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@vladimir-siv
Copy link

vladimir-siv commented Dec 23, 2022

Bug Report

Class decorators are not working properly when the class has a static field of the same type and is instantiating it statically. Seems like this only happens when targeting ES2022.

🔎 Search Terms

Various combinations of the below search strings. Haven't been able to find anything useful.
Been searching for several days now, these are just the most recent ones I could remember:

  • Typescript class decorator with static property not working
  • Typescript decorators: TypeError: undefined is not a constructor
  • Typescript decorators not working (with es2022)
  • Typescript decorator es2022
  • Angular custom decorators not working
  • Angular custom decorators with aot

🕗 Version & Regression Information

Regressed when tsconfig.compilerOptions.target is ES2022. Other ES versions that I tried seem to be working as expected.
Tried Nightly version in the playground, could still repro it (after I changed target to ES2022 in TSConfig).

⏯ Playground Link

Bug repro in Playground
You can try setting e.g. ES2021, and the code works, but not on ES2022.

💻 Code

src\index.ts:

function Message(ctor: Function)
{
    console.log(`Registring: "${ctor.name}"`);
}

@Message
class PingMessage
{
    public static Default: PingMessage = new PingMessage(); // [A]
    public constructor(public readonly Value: string = "Ping") { }
}

// PingMessage.Default = new PingMessage(); // [B]
console.log(PingMessage.Default.Value);

// Swapping instantiation from [A] to [B] renders this code working.
// Seems like constructor is not defined well while instantiating a static member within its own class.

package.json:

{
  "name": "decorator-test",
  "version": "1.0.0",
  "description": "Test App",
  "scripts": {
    "start": "(if exist dist rd /S /Q dist) >nul 2>nul && tsc && node ./dist/index.js"
  },
  "dependencies": {
    "tslib": "^2.4.1"
  },
  "devDependencies": {
    "typescript": "^4.9.4"
  }
}

tsconfig.json:

{
  "compileOnSave": false,
  "compilerOptions": {
    "baseUrl": "./",
    "outDir": "./dist",
    "target": "ES2022",  // <----- !!!
    "module": "ES2022",
    "strict": true,
    "experimentalDecorators": true,
    "useDefineForClassFields": false,
    "lib": [
      "ES2022",
      "dom"
    ]
  },
  "files": [
    "src/index.ts"
  ]
}

🙁 Actual behavior

~\dist\index.js:13
static { this.Default = new PingMessage_1(); } // [A]
          ^

TypeError: undefined is not a constructor
 at Function.<static_initializer> (~\dist\index.js:13:29)
 at Object. (~\dist\index.js:12:19)
 at Module._compile (node:internal/modules/cjs/loader:1155:14)
 at Object.Module._extensions..js (node:internal/modules/cjs/loader:1209:10)
 at Module.load (node:internal/modules/cjs/loader:1033:32)
 at Function.Module._load (node:internal/modules/cjs/loader:868:12)
 at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
 at node:internal/main/run_main_module:22:47

🙂 Expected behavior

Console.logs:

Registring: "PingMessage"
Ping
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jan 4, 2023
@rbuckton
Copy link
Member

rbuckton commented Jan 5, 2023

The binding for PingMessage_1 is initialized too late. I believe I've fixed this for Stage 3 decorators, but not for legacy decorators.

@rbuckton rbuckton added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Jan 5, 2023
@rbuckton
Copy link
Member

rbuckton commented Jan 5, 2023

When targeting ES2022+, the emit for the example above is currently:

let PingMessage = PingMessage_1 = class PingMessage {
    static { this.Default = new PingMessage_1(); }
    constructor(Value = "Ping") {
        this.Value = Value;
    }
};
PingMessage = PingMessage_1 = __decorate([
    Message,
    __metadata("design:paramtypes", [String])
], PingMessage);

However, PingMessage_1 isn't yet initialized. The emit should probably be this instead:

let PingMessage = PingMessage_1 = class PingMessage {
    static { PingMessage_1 = this; }
    static { this.Default = new PingMessage_1(); }
    constructor(Value = "Ping") {
        this.Value = Value;
    }
};
PingMessage = PingMessage_1 = __decorate([
    Message,
    __metadata("design:paramtypes", [String])
], PingMessage);

Please note that class decorators and static initializers that reference this or the class constructor don't play well together under --experimentalDecorators because static initializers run before decorators are applied. That ordering can be problematic when a class decorator performs constructor replacement as the initializer will not use the replacement constructor. In the Stage 3 Decorators proposal, decorators are applied before static initializers instead.

As a workaround, you could lazily initialize the value so that the constructor is run later:

@Message
class PingMessage
{
        private static _Default: PingMessage | undefined;
	public static get Default() { return PingMessage._Default ??= new PingMessage(); }
	public constructor(public readonly Value: string = "Ping") { }
}

@vladimir-siv
Copy link
Author

No worries, feel free to close the issue, there are several ways to work around this, I made a similar workaround, just wanted to report this issue.

@markwhitfeld
Copy link

markwhitfeld commented Jan 7, 2023

Came across this issue recently. I believe it is in fact a TS bug that needs to be resolved.
The issue is caused by the aliased class name, which is not initialised at the time of the static initialisers being called, where the class itself is, in fact, available from within the static initialisers in ES2022.

Given the following TS Code:

function createFromClass(caller: string, classRef: unknown) {
    console.log(`${caller}:`, classRef);
    return 'foo';
}

function SomeDecorator(): ClassDecorator {
    return () => { };
}

class UndecoratedClass {
    static test = createFromClass('UndecoratedClass', UndecoratedClass);
}

@SomeDecorator()
class DecoratedClass {
    static test = createFromClass('DecoratedClass', DecoratedClass);
}

If this is run targeting ES2021 or earlier then the class is available to the static initialisers for both UndecoratedClass and DecoratedClass. But, if we target ES2022 or later then the DecoratedClass class receives undefined as its reference to itself from within its static initialisers.
This is a result of the intermediary ..._1 class reference in the generated code not being populated yet:

let DecoratedClass = DecoratedClass_1 = class DecoratedClass {
    static test = createFromClass('DecoratedClass', DecoratedClass_1);
};
DecoratedClass = DecoratedClass_1 = __decorate([ SomeDecorator() ], DecoratedClass);

When there is no decorator, then the intermediary reference is not created and it all works as expected.

Here is a playground of the above example:
https://www.typescriptlang.org/play?target=9#code/GYVwdgxgLglg9mABBATgUwIZTQMRXAWwGEAbDAZ3IAoIMSS0UAuRcqFGMAcwBpkzKAJTTAW4ANZg4AdzABKRAG8AUIjXIE5OAwB0JOFyoADACSLa9RgF8mRvhAHlhwOQG5V69FBAokAcmA4OD93K2VlUEhYBEQAZUI0ABE0CDgULDSqORZSCnJk1PSoNKUPNS8fJCzEAF4APiVEK1DwhzzEAFUwABMUtKw0btzKUvVWKCwYCERsNlrkdAG8QmHqPy7ewoGhxz8+Db6iwdW3ZTDlAAF4giTDjJQs5TaRgv7sHfaVMbZJ6dmoeaoTDYZbERxUPyvI4fSh7RBQ7YnUJAA

Proposed solution

Not sure all the reasons for the intermediary class reference, but potentially an alternative would be for TS to generate the following JS code when a decorator is used:

let DecoratedClass = (() => {
    class DecoratedClass {
        static {             
            this.test = createFromClass('DecoratedClass', DecoratedClass); 
        }
    };
    return __decorate([ SomeDecorator() ], DecoratedClass);
})()

The code above effectively uses a shadowed variable name within the IIFE as the intermediary class reference to the same effect, but without the reference initialisation timing issue.
This even allows for less of a need for modification of the decorated class code and the final code has fewer characters (ignoring whitespace).
@rbuckton What do you think of this proposed approach for legacy decorators?

(EDIT: It could even be trimmed down to the following, but it really depends on the reason for the intermediary reference as to how far we simplify this.)

let DecoratedClass = (() => 
  __decorate([ SomeDecorator() ],
    class DecoratedClass {
      static {
        this.test = createFromClass('DecoratedClass', DecoratedClass);
      }
    }
  )
)();

@markwhitfeld
Copy link

I am seeing more and more Angular projects running into this issue.
@rbuckton is there any plan to address this issue soon?
Hopefully my proposed solution above could provide some ideas.

@rbuckton
Copy link
Member

rbuckton commented Mar 2, 2023

Not sure all the reasons for the intermediary class reference, [...]

ECMAScript classes are "doubly bound", which means that the class name (i.e., DecoratedClass) is bound both outside the class and inside the class body, separately. This differs both from function declarations, which only bind the function name outside of the function, and function expressions, which only bind the function name inside of the function. The result of this behavior is that any reference to DecoratedClass inside of the class body will always be the original class, even if a class decorator replaces the constructor:

class A {
  static foo() { console.log(A.name); }
}

// replace A with B
A = class B extends A { }

// print the current name of A
console.log(A.name); // B

// indirectly print the name of A that the 'foo' method sees:
A.foo(); // A

[...] but potentially an alternative would be for TS to generate the following JS code when a decorator is used:

let DecoratedClass = (() => {
    class DecoratedClass {
        static {             
            this.test = createFromClass('DecoratedClass', DecoratedClass); 
        }
    };
    return __decorate([ SomeDecorator() ], DecoratedClass);
})()

The code above effectively uses a shadowed variable name within the IIFE as the intermediary class reference to the same effect, but without the reference initialisation timing issue. This even allows for less of a need for modification of the decorated class code and the final code has fewer characters (ignoring whitespace). @rbuckton What do you think of this proposed approach for legacy decorators?

I'm not sure what you mean by "shadowed variable" in this context, but unfortunately this doesn't work per the reason I stated above.

(EDIT: It could even be trimmed down to the following, but it really depends on the reason for the intermediary reference as to how far we simplify this.)

let DecoratedClass = (() => 
  __decorate([ SomeDecorator() ],
    class DecoratedClass {
      static {
        this.test = createFromClass('DecoratedClass', DecoratedClass);
      }
    }
  )
)();

This also wouldn't work since DecoratedClass wouldn't be the replaced constructor.

The only change I could see us making would be to more closely emulate native decorators with regards to the timing of static initializer evaluation, such that where we previously ran static initializers before decorators, we would run them after decorators. However, I'm not certain whether there are customers that might be affected by such a major semantics change. I'm more likely to err on the side of caution and make it an error to reference the class name immediately within a static initializer (much like we do for a TDZ check for let/const).

I would still suggest using lazy initialization for these fields, as I mentioned above.

@markwhitfeld
Copy link

markwhitfeld commented Mar 8, 2023

@rbuckton I have spent some time trying to understand the problem, and I found your commit here which resolved this issue as well as the double binding problem.

EDIT. I have found a better solution in my comment below: #52004 (comment)
Leaving the body of this comment here as a collapsible section if anyone is interested in the thought progression.

Expand to see comment body

I think I have found a solution that would cover these cases and would additionally allow code to function in the same way when targeting ES2022 as when we target ES2021 or earlier.

Given the following TS code:

@SomeDecorator()
class DecoratedClass {
    static test1 = doSomething('DecoratedClass', DecoratedClass);
    static test2 = DecoratedClass;
    static test3() { return DecoratedClass; }
}

and assuming that the SomeDecorator decorator did something dramatic to modify the constructor, like this:

function SomeDecorator() {
    return (target) => {
        return class ModifiedClass extends target {};
    };
}

Today, if we compile with ES2021, we get:

let DecoratedClass = DecoratedClass_1 = class DecoratedClass {
    static test3() { return DecoratedClass_1; }
};
DecoratedClass.test1 = doSomething('DecoratedClass', DecoratedClass_1);
DecoratedClass.test2 = DecoratedClass_1;
DecoratedClass = DecoratedClass_1 = __decorate([
    SomeDecorator()
], DecoratedClass);

This results in the following observations:

  1. DecoratedClass.test1 has its initialiser called with DecoratedClass before the decorator ran
  2. DecoratedClass.test2 references the DecoratedClass from before the decorator
  3. DecoratedClass.test3, when invoked would return ModifiedClass (as returned by the decorator)

Today, if we compile with ES2022, we get:

let DecoratedClass = DecoratedClass_1 = class DecoratedClass {
    static { this.test1 = doSomething('DecoratedClass', DecoratedClass_1); }
    static { this.test2 = DecoratedClass_1; }
    static test3() { return DecoratedClass_1; }
};
DecoratedClass = DecoratedClass_1 = __decorate([
    SomeDecorator()
], DecoratedClass);

This results in the following observations:

  1. DecoratedClass.test1 has its initialiser called with undefined
  2. DecoratedClass.test2 is undefined
  3. DecoratedClass.test3, when invoked would return ModifiedClass (as returned by the decorator)

1 and 2 are breaking changes!


This would be fixed if the code generated when targeting ES2022 was changed to:

let DecoratedClass = DecoratedClass_1 = class DecoratedClass {
    static { this.test = doSomething('DecoratedClass', this); }
    static { this.test1 = this; }
    static test2() { return DecoratedClass_1; }
};
DecoratedClass = DecoratedClass_1 = __decorate([
    SomeDecorator()
], DecoratedClass);

As you can see, references to DecoratedClass_1 have been replaced with this for the test1 and test2 initialisers.
So, if I understand things correctly, references to the containing class from static property initialiser expressions could be replaced with this, but static function declarations would still use the aliased name (DecoratedClass_1) in order to bind to the decorated class.

Do you think that this is a viable solution?

PS. I know that native decorators would do things differently here, but the "Experimental Decorators" have a different historical behavior in this regard, so I think that the historical behavior should be maintained for the experimental decorators.

@markwhitfeld
Copy link

markwhitfeld commented Mar 8, 2023

EDIT. I have found a better solution in my comment below: #52004 (comment)
Leaving the body of this comment here as a collapsible section if anyone is interested in the thought progression.

Expand to see comment body

Just an additional note that a static initialiser like this:

static test4 = accessLater(function foo() { return DecoratedClass });

would also still need to return the aliased class DecoratedClass_1 and not this because the foo function has its own this that we would not want to be referencing.

But the potential issue here is that, depending on when this function is executed, the aliased class may not yet be available.
I'll play a bit more...

@markwhitfeld
Copy link

markwhitfeld commented Mar 9, 2023

Ok, I think I have got a solution that will work for all scenarios.
I reworked the playground to make the expectations clearer too: See here

Trimmed TS example code:

console.log('=== Declaring: Original');

@SomeDecorator()
class Original {
    static test1 = init('Original', Original);
    static test2 = Original;
    static test3() { return Original; }
    static test4 = doNothing(function () { return Original });
    static test5 = Original?.test4();
}

console.log('--- After Declaration, should be Wrapped:', Original.name);
console.log(`'test1' should be Original:`, Original.test1?.name);
console.log(`'test2' should be Original:`, Original.test2?.name);
console.log(`'test3()' should return Wrapped:`, Original.test3()?.name);
console.log(`'test4()' should return Wrapped:`, Original.test4()?.name);
console.log(`'test5' should be Original:`, Original.test5?.name);

When run using ES2021, the output is:

[LOG]: "=== Declaring: Original" 
[LOG]: "In initialiser, should be 'Original':",  "Original" 
[LOG]: "INIT Decorator" 
[LOG]: "Decorating class:",  "Original" 
[LOG]: "--- After Declaration, should be Wrapped:",  "Wrapped" 
[LOG]: "'test1' should be Original:",  "Original" 
[LOG]: "'test2' should be Original:",  "Original" 
[LOG]: "'test3()' should return Wrapped:",  "Wrapped" 
[LOG]: "'test4()' should return Wrapped:",  "Wrapped" 
[LOG]: "'test5' should be Original:",  "Original"

All expectations align.

But, when run using ES2022, the output is:

[LOG]: "=== Declaring: Original" 
[LOG]: "In initialiser, should be 'Original':",  undefined 
[LOG]: "INIT Decorator" 
[LOG]: "Decorating class:",  "Original" 
[LOG]: "--- After Declaration, should be Wrapped:",  "Wrapped" 
[LOG]: "'test1' should be Original:",  undefined 
[LOG]: "'test2' should be Original:",  undefined 
[LOG]: "'test3()' should return Wrapped:",  "Wrapped" 
[LOG]: "'test4()' should return Wrapped:",  "Wrapped" 
[LOG]: "'test5' should be Original:",  undefined 

Many cases are broken for ES2022.

Here is the current code emitted for the ES2022 target:

let Original = Original_1 = class Original {
    static { this.test1 = init('Original', Original_1); }
    static { this.test2 = Original_1; }
    static test3() { return Original_1; }
    static { this.test4 = doNothing(function () { return Original_1; }); }
    static { this.test5 = Original_1?.test4(); }
};
Original = Original_1 = __decorate([
    SomeDecorator()
], Original);

I think the best solution that covers all bases here is to add a static initialiser block at the top of the emitted class:

    static { Original_1 = this; }

Now the alias will be available to all the static initialiser blocks. Everything works!

Here is an example of the tweaked JS output code (playground) with just this one line added:

let Original = Original_1 = class Original {
    static { Original_1 = this; } // <-- PROPOSAL TO ADD THIS LINE
    static { this.test1 = init('Original', Original_1); }
    static { this.test2 = Original_1; }
    static test3() { return Original_1; }
    static { this.test4 = doNothing(function () { return Original_1; }); }
    static { this.test5 = Original_1?.test4(); }
};
Original = Original_1 = __decorate([
    SomeDecorator()
], Original);

@rbuckton what do you think of this solution?

PS. potentially the line: let Original = Original_1 = class Original {
could now be simplified: let Original = class Original {
because the alias is assigned internally. Although this is just a bundle size optimization.

@rbuckton
Copy link
Member

let Original = Original_1 = class Original {
    static { Original_1 = this; } // <-- PROPOSAL TO ADD THIS LINE
    static { this.test1 = init('Original', Original_1); }
    static { this.test2 = Original_1; }
    static test3() { return Original_1; }
    static { this.test4 = doNothing(function () { return Original_1; }); }
    static { this.test5 = Original_1?.test4(); }
};
Original = Original_1 = __decorate([
    SomeDecorator()
], Original);

@rbuckton what do you think of this solution?

Yes, this will work fine. I should have a PR up for this shortly.

PS. potentially the line: let Original = Original_1 = class Original { could now be simplified: let Original = class Original { because the alias is assigned internally. Although this is just a bundle size optimization.

No, this will not work. Inside of a class body, the bound name of the class is immutable (constant) and cannot be changed.

@rbuckton
Copy link
Member

PS. potentially the line: let Original = Original_1 = class Original { could now be simplified: let Original = class Original { because the alias is assigned internally. Although this is just a bundle size optimization.

No, this will not work. Inside of a class body, the bound name of the class is immutable (constant) and cannot be changed.

Sorry, I misread your suggestion. To clarify, you're stating that the Original_1 assignment in the let is unnecessary because it will be assigned in the first static block. Yes, this will work.

@KurtGokhan
Copy link

Will this be backported to 4.9? We are running into this issue in Angular 15 which uses TS 4.9 and targets ES2022 by default.

@markwhitfeld
Copy link

I agree that a backport would be really great. This bug is coming up many times in the Angular community.

@lalo-mx
Copy link

lalo-mx commented Jun 2, 2023

How likely is to be fixed in 5.0 branch? Angular 16 currently depends on 5.0 branch and looks like is expected to be fixed on TypeScript side angular/angular#49415

@RyanCavanaugh
Copy link
Member

In general we only backport critical crashes or regression specific to a particular release, or issues without any workarounds. This doesn't appear to qualify for any of those criteria.

@russcarver
Copy link

This is still an issue with Angular 16.1.4 and Typescript 5.0.4 and a tsconfig with lib: es2022, module: es2022 and target of es2022. It's fine with a target of es2021.

According to the Angular upgrade matrix (https://gist.github.com/LayZeeDK/c822cc812f75bb07b7c55d07ba2719b3), for Angular 16.x.x, you must use Typescript >=4.9.5 <5.1.0

Has anyone tried Typescript >=5.1.0 with Angular ^16.1.4?

If no one knows the compatibility results, I'll give it a try and see what happens.

@ericrovtar
Copy link

This is still an issue with Angular 16.1.4 and Typescript 5.0.4 and a tsconfig with lib: es2022, module: es2022 and target of es2022. It's fine with a target of es2021.

According to the Angular upgrade matrix (https://gist.github.com/LayZeeDK/c822cc812f75bb07b7c55d07ba2719b3), for Angular 16.x.x, you must use Typescript >=4.9.5 <5.1.0

Has anyone tried Typescript >=5.1.0 with Angular ^16.1.4?

If no one knows the compatibility results, I'll give it a try and see what happens.

To answer this question: Yes, using TypeScript 5.1.3 with Angular 16.2.12 did the trick.

@iqaan
Copy link

iqaan commented Mar 26, 2025

Still an issue with Angular 17 (tried, 17.3.12) and TypeScript 5.4 (tried, 5.4.3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants