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

useDefineForClassFields and parameter properties: missing error or bad emit #36410

Closed
ajafff opened this issue Jan 24, 2020 · 4 comments · Fixed by #36425
Closed

useDefineForClassFields and parameter properties: missing error or bad emit #36410

ajafff opened this issue Jan 24, 2020 · 4 comments · Fixed by #36425
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@ajafff
Copy link
Contributor

ajafff commented Jan 24, 2020

TypeScript Version: 3.8.0-dev.20200123

Search Terms: useDefineForClassFields parameter

Code

// @useDefineForClassFields: true
// @target: esnext
class C {
  bar = this.foo.bar;
  constructor(private foo: {bar: string}) {}
}

Expected behavior:

Transpiled code works (bar is defined in the constructor as it's done for the downlevel emit) OR used-before-assignment error on this.foo in the initializer of bar.

Actual behavior:

Since the initializer of bar is run before the assignment of foo, there's a runtime error.
This code used to work for ages (of course without native class fields). A lot of code that uses dependency injection looks like this.

Playground Link: https://www.typescriptlang.org/play/?useDefineForClassFields=true&target=99&ts=3.8.0-dev.20200123&ssl=4&ssc=2&pln=1&pc=1#code/MYGwhgzhAEDC0G8BQ1oCMwCdoF5oBcALASwgDoAzAeyrI0wG4VpgqA7CfTAV2HyswAKAA6ZiANzD4AptGpUAXInpLOYtgHMAvgEpEWpFqA

Related Issues:
I thought this was already discussed somewhere with @RyanCavanaugh and @sandersn, but I couldn't find an issue for that.
Found while looking at #36405. That PR would need an exception if parameter properties are involved IF you want to continue supporting the example code.

@sandersn
Copy link
Member

Support for [[Define]] so far has given an error whenever code that used to work for [[Set]] is no longer valid. I think that an error with useDefineForClassFields: true is the correct thing here. This error should move to useDefineForClassFields: false at the same time as the other errors from #33401 and #33423.

@sandersn sandersn self-assigned this Jan 24, 2020
@sandersn sandersn added the Bug A bug in TypeScript label Jan 24, 2020
@sandersn
Copy link
Member

sandersn commented Jan 24, 2020

The emitted code is also wrong, but in a different way, for targets lower than esnext:

// @useDefineForClassFields: true
// @target: es2019
class C {
    constructor(foo) {
        this.foo = foo;
        Object.defineProperty(this, "foo", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: void 0
        });
        Object.defineProperty(this, "bar", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: this.foo.bar
        });
    }
}

"foo"'s defineProperty should come before this.foo = foo (or should replace it entirely).

Edit: This should not be an error since we're not trying to emit class fields.

@sandersn
Copy link
Member

Never mind, the bug I saw must be fixed because it only repros in the playground.

@ajafff
Copy link
Contributor Author

ajafff commented Jan 24, 2020

@sandersn yeah, you fixed that a while back in #34987
... and in that PR we already talked about the order of parameter properties vs. class field initializers.

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.

2 participants