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 is false, but overriding accessor with property still throws error #42563

Closed
trusktr opened this issue Jan 31, 2021 · 5 comments
Assignees
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@trusktr
Copy link
Contributor

trusktr commented Jan 31, 2021

Bug Report

πŸ”Ž Search Terms

useDefineForClassFields override accessor with instance property (no results really, but related: #33509)

πŸ•— Version & Regression Information

I'm in 4.1.3

⏯ Playground Link

playground

πŸ’» Code

class Foo {
  _foo: number = 1
  get foo() {return this._foo}
  set foo(v) {this._foo = v}
}

class Bar extends Foo {
  foo = 123 // ERROR, although useDefineForClassFields is false
}

πŸ™ Actual behavior

There is an error

πŸ™‚ Expected behavior

There should be no error, because the legacy [[Set]] semantics are in play, and therefore it works as intended: the subclass calls the super class accessor.

It should desugar to:

class Foo {
    constructor() {
        this._foo = 1;
    }
    get foo() { return this._foo; }
    set foo(v) { this._foo = v; }
}
class Bar extends Foo {
    constructor() {
        super();
        this.foo = 123; // triggers accessor
    }
}

which is very similar to the output that you see in the playground.


Also it doesn't work with declare.

@whzx5byb
Copy link

Related: #37894, #40220

@trusktr
Copy link
Contributor Author

trusktr commented Jan 31, 2021

I ended up finding that issue, and I left a comment stating my sentiment that the new behavior makes useDefineForClassFields essentially an obsolete option, and that the new behavior is not a good solution because it prevents declaring types for valid JavaScript code rather than expanding our static type abilities.

@RyanCavanaugh
Copy link
Member

@sandersn what was the motivation for the change in 4.0 around this?

@sandersn sandersn self-assigned this Feb 2, 2021
@sandersn sandersn added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 2, 2021
@sandersn
Copy link
Member

sandersn commented Feb 2, 2021

It's described in #37894.

@sandersn sandersn closed this as completed Feb 2, 2021
@canonic-epicure
Copy link

canonic-epicure commented Jun 25, 2021

@RyanCavanaugh The irony is that it seems the motivation for the #37894 was the #34585, filed by the same author as this issue. Also see the updated discussion in the #37894, for example starting from here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants