Skip to content

Pre-initialized class fields with a TSConfig target set to ES2022 with useDefineForClassFields set to true will cause runtime issues #3654

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
Coly010 opened this issue Nov 7, 2022 · 22 comments

Comments

@Coly010
Copy link

Coly010 commented Nov 7, 2022

Information

The Problem

Using TS with a TSConfig which has the target set to ES2022.

{
  "compilerOptions": {
    "target": "ES2022"
  }
}

When TS compiles code with this target, it aims to make it TC39 Compliant.

This introduces a runtime issue on modern browsers when we try to write classes that follow this pattern:

export class Effects {
  doSomething$ = createEffect(() => this.actions$.pipe(...));
  
  constructor(private actions$: Actions) {}
}
class MyComponent {
  storeValue$ = this.store.select(mySelector);
  
  constructor(private store: Store) {}
}

Following this pattern of initializing class properties by using a class property that is added via the constructor (this.actions$, this.store) will throw runtime errors that these properties are not defined.

Expected errors (listed in Browser Console):

Cannot read properties of undefined (reading "pipe")

Cannot read properties of undefined (reading "get")

Workarounds

There are currently two workarounds.

  1. Make sure your code is TC39 Compliant
export class Effects {
  doSomething$;
  
  constructor(private actions$: Actions) {
    this.doSomething$ = createEffect(() => this.actions$.pipe(...));
  }
}
class MyComponent {
  storeValue$;
  
  constructor(private store: Store) {
    this.storeValue$  = this.store.select(mySelector);
  }
}
  1. For @Injectable classes, use the inject() function
export class Effects {
  private actions$ = inject(Actions);
  doSomething$ = createEffect(() => this.actions$.pipe(...));
}
class MyComponent {
  private store = inject(Store);
  storeValue$ = this.store.select(mySelector);
}
  1. Set useDefineForClassFields in tsconfig.json

In your tsconfig.json (or tsconfig.app.json in Nx Workspaces), set "useDefineForClassFields": false in the compilerOptions.

Example:

{
  "compilerOptions": {
     ...,
     "useDefineForClassFields": false
  }
}

Notes

Existing applications that are migrated via the Angular CLI or Nx CLI will have "useDefineForClassFields": false set automatically.

@brandonroberts brandonroberts pinned this issue Nov 7, 2022
@nartc
Copy link

nartc commented Nov 7, 2022

I'd like to add another note on "Workaround" is that if you use inject(), you'll likely not run into this issue.

@eneajaho
Copy link
Contributor

eneajaho commented Nov 7, 2022

Reproduction: https://stackblitz.com/edit/angular-ivy-6ayd4a?file=src%2Fapp%2Fapp.component.ts

@Coly010
Copy link
Author

Coly010 commented Nov 7, 2022

I'd like to add another note on "Workaround" is that if you use inject(), you'll likely not run into this issue.

This is true for anything that can be injected. Which is the case for NgRx. It won't erase the issue for non-injectable classes.

@JeanMeche
Copy link

FYI, there is an open ticket about this on microsoft/TypeScript#50971.

When targeting ES2022 the compiler doesn't throw an error, but does it when targeting ESNext

@alex-okrushko
Copy link
Member

TS team says that it works "as expected", so we'll need to add "useDefineForClassFields": false to compiler options.

The current issue that TS is not highlighting it as an issue without the flag, and that will be fixed.

@alan-agius4
Copy link
Contributor

alan-agius4 commented Nov 7, 2022

The useDefineForClassFields is used as part to allow users to migrate to the upcoming standard version of class fields.

New applications using the Angular CLI will also have useDefineForClassFields set to false. But eventually we do want to disable this by default to allow writing TC39 compliant TS.

I think for starters the NGRX docs should be updated to be TC39 spec complaint so at least new users don’t write non complaint code.

@brandonroberts
Copy link
Member

@alan-agius4 that seems like a pretty big ask across the entire Angular ecosystem, not just NgRx. We've discussed moving away from class-based grouping for effects but it wasn't prompted by this recent change. Feels like this should be handled by the compiler or somewhere else upstream and not the developer.

@alex-okrushko
Copy link
Member

@alan-agius4

But eventually we do want to disable this and generate TC39 compliant output.

With useDefineForClassFields flag set either to true or false TS generates TC39 compliant code. IHMO, TS should continue to be more than just "typing" and still support some of the language features. I do understand that continuing with the current behavior might mean that it would be more confusing for future JS devs adopting TS.

@Coly010 Coly010 changed the title Pre-initialized class fields cause runtime errors in new Angular 15 applications Pre-initialized class fields with a TSConfig target set to ES2022 without useDefineForClassFields set to false will cause runtime issues Nov 7, 2022
@alan-agius4
Copy link
Contributor

alan-agius4 commented Nov 7, 2022

Right, got confused both outputs are spec compliant although now in the latest spec it has a different runtime behavior.

@Coly010 Coly010 changed the title Pre-initialized class fields with a TSConfig target set to ES2022 without useDefineForClassFields set to false will cause runtime issues Pre-initialized class fields with a TSConfig target set to ES2022 without useDefineForClassFields set to true will cause runtime issues Nov 7, 2022
@JeanMeche
Copy link

TS team says that it works "as expected", so we'll need to add "useDefineForClassFields": false to compiler options.

It's as expected, but the compiler should error, like it does with the EsNext target.

@e-oz
Copy link
Contributor

e-oz commented Nov 8, 2022

Nice to finally see some discussion, because when I raised a discussion about this issue it went unnoticed: #3405

Also I want to remind you all, that Angular 15 does not allow us to control “target” and “useDefinedForClassFields”: angular/angular-cli#23936

@alan-agius4
Copy link
Contributor

alan-agius4 commented Nov 8, 2022

You can control useDefinedForClassFields and target if the target is set to ES2022 or later.

@e-oz
Copy link
Contributor

e-oz commented Nov 8, 2022

It's nice that we can control “target” if the target is exactly ES2022 (because the only option left is “esnext” and you should not use it in prod).

A customer can have a car painted any color he wants as long as it’s black 😎

@Coly010 Coly010 changed the title Pre-initialized class fields with a TSConfig target set to ES2022 without useDefineForClassFields set to true will cause runtime issues Pre-initialized class fields with a TSConfig target set to ES2022 with useDefineForClassFields set to true will cause runtime issues Nov 8, 2022
@JohannesHoppe
Copy link

The simplest workaround should be the following. We perform a general search and replace for both the demo and the schematics according to the following pattern:

  1. search for constructors
  2. Locate all properties where declaration & initialisation is done together
    --> this.XXX: MyInjectable is changed to inject(MyInjectable)
  3. Keep constructor with constructor injection if the property is also used within methods, remove constructor if not needed anymore

BEFORE:

@Injectable()
export class BookEffects {

  loadBooks$ = createEffect(() => {
    return this.actions$.pipe( 

      ofType(BookActions.loadBooks),
      concatMap(() =>
        /** An EMPTY observable only emits completion. Replace with your own observable API request */
        EMPTY.pipe(
          map(data => BookActions.loadBooksSuccess({ data })),
          catchError(error => of(BookActions.loadBooksFailure({ error }))))
      )
    );
  });


  constructor(private actions$: Actions) {}
}

AFTER:

import { inject } from '@angular/core';

@Injectable()
export class BookEffects {

  loadBooks$ = createEffect(() => {
    inject(Actions).pipe( 

      ofType(BookActions.loadBooks),
      concatMap(() =>
        /** An EMPTY observable only emits completion. Replace with your own observable API request */
        EMPTY.pipe(
          map(data => BookActions.loadBooksSuccess({ data })),
          catchError(error => of(BookActions.loadBooksFailure({ error }))))
      )
    );
  });
}

It has even fewer lines of code! 😎

@mariomurrent-softwaresolutions

The simplest workaround should be the following. We perform a general search and replace for both the demo and the schematics according to the following pattern:

  1. search for constructors
  2. Locate all properties where declaration & initialisation is done together
    --> this.XXX: MyInjectable is changed to inject(MyInjectable)
  3. Keep constructor with constructor injection if the property is also used within methods, remove constructor if not needed anymore

BEFORE:

@Injectable()
export class BookEffects {

  loadBooks$ = createEffect(() => {
    return this.actions$.pipe( 

      ofType(BookActions.loadBooks),
      concatMap(() =>
        /** An EMPTY observable only emits completion. Replace with your own observable API request */
        EMPTY.pipe(
          map(data => BookActions.loadBooksSuccess({ data })),
          catchError(error => of(BookActions.loadBooksFailure({ error }))))
      )
    );
  });


  constructor(private actions$: Actions) {}
}

AFTER:

import { inject } from '@angular/core';

@Injectable()
export class BookEffects {

  loadBooks$ = createEffect(() => {
    inject(Actions).pipe( 

      ofType(BookActions.loadBooks),
      concatMap(() =>
        /** An EMPTY observable only emits completion. Replace with your own observable API request */
        EMPTY.pipe(
          map(data => BookActions.loadBooksSuccess({ data })),
          catchError(error => of(BookActions.loadBooksFailure({ error }))))
      )
    );
  });
}

It has even fewer lines of code! 😎

Did not work for me :(

lukas99 added a commit to lukas99/ysg-manager that referenced this issue Jan 18, 2023
Logic was in constructor before. Parameters injected in constructor are undefined when used in component class field initialization.

Needed for use of latest JavaScript version (ES2022).
See: ngrx/platform#3654
@brandonroberts
Copy link
Member

Closing this as resolved as it's handled by the framework migrations

@brandonroberts brandonroberts unpinned this issue May 8, 2023
@duytran31187
Copy link

I still got this issue. how it resolved?

@duytran31187
Copy link

solution: upgrade typescript: "typescript": "4.9.4"

@JohannesHoppe
Copy link

I would refactor the code to use inject. Here is an article that describes the issue and explains how to rewrite your code base: https://angular.schule/blog/2022-11-use-define-for-class-fields

@mrwilbroad
Copy link

Information

The Problem

Using TS with a TSConfig which has the target set to ES2022.

{
  "compilerOptions": {
    "target": "ES2022"
  }
}

When TS compiles code with this target, it aims to make it TC39 Compliant.

This introduces a runtime issue on modern browsers when we try to write classes that follow this pattern:

export class Effects {
  doSomething$ = createEffect(() => this.actions$.pipe(...));
  
  constructor(private actions$: Actions) {}
}
class MyComponent {
  storeValue$ = this.store.select(mySelector);
  
  constructor(private store: Store) {}
}

Following this pattern of initializing class properties by using a class property that is added via the constructor (this.actions$, this.store) will throw runtime errors that these properties are not defined.

Expected errors (listed in Browser Console):

Cannot read properties of undefined (reading "pipe")

Cannot read properties of undefined (reading "get")

Workarounds

There are currently two workarounds.

  1. Make sure your code is TC39 Compliant
export class Effects {
  doSomething$;
  
  constructor(private actions$: Actions) {
    this.doSomething$ = createEffect(() => this.actions$.pipe(...));
  }
}
class MyComponent {
  storeValue$;
  
  constructor(private store: Store) {
    this.storeValue$  = this.store.select(mySelector);
  }
}
  1. For @Injectable classes, use the inject() function
export class Effects {
  private actions$ = inject(Actions);
  doSomething$ = createEffect(() => this.actions$.pipe(...));
}
class MyComponent {
  private store = inject(Store);
  storeValue$ = this.store.select(mySelector);
}
  1. Set useDefineForClassFields in tsconfig.json

In your tsconfig.json (or tsconfig.app.json in Nx Workspaces), set "useDefineForClassFields": false in the compilerOptions.

Example:

{
  "compilerOptions": {
     ...,
     "useDefineForClassFields": false
  }
}

Notes

Existing applications that are migrated via the Angular CLI or Nx CLI will have "useDefineForClassFields": false set automatically.

this work fine for me

@Himanshu-Tamrakar
Copy link

using @Inject instead of constructor worked for me.

@shanidpk
Copy link

using @Inject instead of constructor worked for me also

jimkeecn pushed a commit to jimkeecn/angular-monorepo that referenced this issue Mar 22, 2025
fixing ngrx action$ not define issue in nx workspace
mmbatha pushed a commit to mmbatha/tr-hacker-angular that referenced this issue May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests