Skip to content

[ts] Support override modifiers for parameter properties#13428

Merged
nicolo-ribaudo merged 6 commits into
babel:mainfrom
sosukesuzuki:fix-13427
Jun 10, 2021
Merged

[ts] Support override modifiers for parameter properties#13428
nicolo-ribaudo merged 6 commits into
babel:mainfrom
sosukesuzuki:fix-13427

Conversation

@sosukesuzuki

@sosukesuzuki sosukesuzuki commented Jun 6, 2021

Copy link
Copy Markdown
Contributor
Q                       A
Fixed Issues? Fixes #13427
Patch: Bug Fix? Y
Tests Added + Pass? Yes
License MIT

@sosukesuzuki sosukesuzuki added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: typescript labels Jun 6, 2021
@babel-bot

babel-bot commented Jun 6, 2021

Copy link
Copy Markdown
Collaborator

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

@codesandbox-ci

codesandbox-ci Bot commented Jun 6, 2021

Copy link
Copy Markdown

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 8f7c78c:

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

@fedeci

fedeci commented Jun 6, 2021

Copy link
Copy Markdown
Member

Do we enforce modifier order in constructor args too?
The following seems to be throwing the wrong error:

class Foo extends Bar {
  constructor(override private readonly foo: string) {
  }
}

And can you test this also on a regular class method, because the test should fail in that case as for readonly.

if (allowModifiers !== undefined) {
accessibility = this.parseAccessModifier();
override = !!this.tsParseModifier(["override"]);
readonly = !!this.tsParseModifier(["readonly"]);

This comment was marked as duplicate.

accessibility = this.parseAccessModifier();
override = !!this.tsParseModifier(["override"]);
readonly = !!this.tsParseModifier(["readonly"]);
if (allowModifiers === false && (accessibility || readonly)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we also check override here?

Comment on lines +1936 to +1943
const modified = {};
this.tsParseModifiers(modified, [
"public",
"private",
"protected",
"override",
"readonly",
]);
accessibility = modified.accessibility;
override = modified.override;
readonly = modified.readonly;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used tsParseModifiers on an empty object to reuse the logic to enforce the order. What do you think?

@JLHwung JLHwung left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo changed the title babel-parser: [ts] Support override modifiers for parameter properties [ts] Support override modifiers for parameter properties Jun 10, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit 0eb2853 into babel:main Jun 10, 2021
@sosukesuzuki sosukesuzuki deleted the fix-13427 branch June 11, 2021 00:19
@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 Sep 10, 2021
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 10, 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: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Syntax error is thrown for override modifiers for parameter properties

5 participants