Skip to content
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

Incorrect emit for parameter properties when using standard class fields #55132

Open
sandersn opened this issue Jul 24, 2023 · 7 comments
Open
Assignees
Labels
Bug A bug in TypeScript Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@sandersn
Copy link
Member

Pointed out by @rbuckton on #50971, based on the repro from that bug. When targetting ES2022 or higher, and leaving useDefineWithClassFields: true (the default) for that target:

class Helper {
    create(): boolean {
        return true
    }
}
export class Broken {
    constructor(readonly facade: Helper) {
        console.log(this.bug)
    }
    bug = this.facade.create()
}
new Broken(new Helper)

Produces

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.Broken = void 0;
class Helper {
    create() {
        return true;
    }
}
class Broken {
    facade;
    constructor(facade) {
        this.facade = facade;
        console.log(this.bug);
    }
    bug = this.facade.create();
}
exports.Broken = Broken;
new Broken(new Helper);

Currently the compiler issues an error on bug = this.facade.create() to avoid the bad emit, but it might be better to change the emit when parameter properties are used:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.Broken = void 0;
class Helper {
    create() {
        return true;
    }
}
class Broken {
    facade;
    bug;
    constructor(facade) {
        this.facade = facade;
        this.bug = this.facade.create();
        console.log(this.bug);
    }
}
exports.Broken = Broken;
new Broken(new Helper);

Unfortunately, this isn't standard initialisation order for class fields...so it's correct, but not standard. That's probably OK since parameter properties aren't standard.

@fatcerberus
Copy link

fatcerberus commented Jul 24, 2023

That's probably OK since parameter properties aren't standard.

I’d argue that it’s not OK because you’re potentially changing the initialization behavior of a bunch of unrelated properties just because the class constructor has some parameter props, which feels icky.

If I enable useDefineForClassFields then generally the reason I’ve done so is because I want fields that behave according to the standard. If that behavior doesn’t compose with some older TS feature, I’d rather know about it rather than have the compiler try to be clever and make it work by fudging things.

@rbuckton
Copy link
Member

We've done a similar transformation in the past, such as we do for accessor fields. I don't think this transform would have any major issues, as the only side effects you might observe from this emit would be bad practice to depend on to begin with.

@sandersn sandersn added the Bug A bug in TypeScript label Jul 24, 2023
@sandersn sandersn added this to the Backlog milestone Jul 24, 2023
@robpalme
Copy link

I appreciate this fix will align the newer mode with the original intended behaviour.

What are the chances this will be a breaking change for users of "useDefineForClassFields"?

@sandersn
Copy link
Member Author

@robpalme As far as I can see, the pre-5.2 state is: Typescript doesn't error, but you may crash at runtime, depending on order of class fields vs parameter properties. Post-5.2: Typescript errors. So fixing this bug would avoid crashes or compile errors; that doesn't sound like a breaking change except in the very strictest of meanings where "I expected a crash but the program terminated successfully".

@NicBright
Copy link

NicBright commented Sep 14, 2023

@sandersn wrote:

Currently the compiler issues an error on bug = this.facade.create() to avoid the bad emit, but it might be better to change the emit when parameter properties are used (...)

Totally Agree! This bug (until fixed) forces us to stay with target: "es2021".

@fatcerberus wrote:

If I enable useDefineForClassFields then generally the reason I’ve done so is because I want fields that behave according to the standard.

Exactly. As I understand it, the useDefineForClassFields option was introduced to bring us [[Define]] semantics over [[Set]] semantics. The proposed solution doesn't change this.

In general: regarding whether this bugfix maybe could be a breaking change or not, this is how I see it:

  • either people had used target < es2022 until now, then they never have had proper ES class fields so far. They will only be starting using them when switching to es2022 output. So this cannot be a breaking change for them (in contrast: the bugfix prevents them from suffering a severe breaking change when using parameter properties to initialize class fields)
  • or people are using plain vanilla JS, maybe relying heavily on latest ES standards (i.e. class fields as in the spec) and didn't use TypeScript so far. When switching to TypeScript it will also be not a breaking change for them, because they could not have been using parameter properties before (as it is a TypeScript feature).
  • or people had used TypeScript all the time and switched to target: es2022 "as soon as it was out" (i.e. TypeScript 4.6, released on February 28, 2022). If they sticked with this target, they likely didn't use parameter properties, or at least not used the parameter properties to initialize class fields. Otherwise they would have stumbled into this bug as well. So maybe the proposed solution can be extended by "not pulling things up into constructor", when the available parameter properties aren't used to initialize class fields.

Final Note:
The current behavior of "issuing an error" not always works, e.g. think of this NgRx effect, where the "callback" given to the createEffect() function is called immediately, resulting in cannot read property 'pipe' of 'undefined' (because this.actions$ is undefined at this moment as we're still in the middle of instantiation)

(Example taken from https://ngrx.io/guide/effects)

export class MoviesEffects {
 
  loadMovies$ = createEffect(() => this.actions$.pipe(
    ofType('[Movies Page] Load Movies'),
    exhaustMap(() => this.moviesService.getAll()
      .pipe(
        map(movies => ({ type: '[Movies API] Movies Loaded Success', payload: movies })),
        catchError(() => EMPTY)
      ))
    )
  );
 
  constructor(
    private actions$: Actions,
    private moviesService: MoviesService
  ) {}
}

@NicBright
Copy link

Any news on this? It's 2024 now but because of this issue we're still forced to stick with target: "es2021".

Side note: this issue duplicates #45995

@NicBright
Copy link

CC: @sandersn @rbuckton @alan-agius4

I think this issue really should be resolved rather sooner than later!

I just found out that Angular has had to force-patch their users' tsconfig.json inside @angular-devkit/build-angular (see esbuild tooling + webpack-tooling) because of this bug! Effectively this is leaving us with target: es2022 and useDefineForClassFields=false which is the opposite of what I intended to have (my tsconfig.json states target: es2021 and useDefineForClassFields=true). @alan-agius4 are you aware that you're forcing all Angular users to the old semantics of class fields unless they explicitly set useDefineForClassFields=true (this was your intention by looking at the source code (links see above), but you've implemented it incorrectly by checking it only against the locally created compilerOptions, which never has a useDefineForClassFields set, thus it's always forced to be false (see webpack-tooling). Because of this incorrect implementation, all users using webpack-tooling right now don't even have a choice here.

I can easily imagine this not ending well ... as soon as TypeScript will drop support for target: es2022 (or newer) + useDefineForClassFields=false a lot of projects will be broken for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

No branches or pull requests

6 participants