Skip to content

Conversation

@fedeci
Copy link
Member

@fedeci fedeci commented May 2, 2021

Q                       A
Fixed Issues? Closes #13125
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We override ClassScopeHandler in the TS plugin so that it is possible to check for class accessors with different accessibility levels.
In this PR we do not take into account MemberExpression and CallExpression keys because TS does not return any error for those

Comment on lines +2 to +3
public get bar() {}
set bar(v) {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a TS bug 🤔

@fedeci fedeci added area: typescript pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels May 2, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented May 2, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47131/

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 2, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d97eda5:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration


enter() {
this.stack.push(new ClassScope());
this.stack.push(this.createScope());
Copy link
Member Author

@fedeci fedeci May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot call 'this.stack.push' because 'ClassScope' [1] is incompatible with 'IClassScope' [2] in array element.
Why this? Shouldn't it be allowed since IClassScope: ClassScope?

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the behaviour of different accessibility for accessors has changed in TS 4.3:

class Foo {
  public get f() { return 1 }
  private set f(x) {}
}

is now allowed. https://www.typescriptlang.org/play?target=8&ts=4.3.0-dev.20210503#code/MYGwhgzhAEBiD29oG8BQ1oAcCuAjEAlsNAOYCmALtAGYAUAlCtAE6XbMB20AjNAL7oszAgDcwFMtAiUatAB6NkAvkA

Based on that we may want to hold off this PR and close #13125 since it does not throw on TS 4.3

@fedeci
Copy link
Member Author

fedeci commented May 4, 2021

class Foo {
    protected get foo() { return 1 }
    set foo(v: any) { }
}

In 4.3.0 accessors are allowed to have a different accessibility level, but getters must have at least the same level of the setters. https://www.typescriptlang.org/play?ts=4.3.0-beta#code/MYGwhgzhAEBiD29oG8BQ0PQA4Cd4BcBTYIgE2gHND9oAzRACgEoVodqBXHAO2gEZoAX3SYI1OowBuALmhhuATxbIhqQUA
Ref PR: microsoft/TypeScript#42425

@fedeci fedeci force-pushed the accessibilty-accessors branch from 706c907 to d97eda5 Compare June 28, 2021 13:46
@fedeci fedeci marked this pull request as draft June 28, 2021 15:14
@fedeci
Copy link
Member Author

fedeci commented Sep 26, 2021

Closing this, we may want to check the correct behaviour when implementing auto accessors

@fedeci fedeci closed this Sep 26, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Babel parser typescript should throw when accessors do not agree in visibility

3 participants