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

[parser] Add support for TS declare modifier on fields #10484

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Sep 23, 2019

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

Ref: microsoft/TypeScript#33401

I also updated the parser to generate more useful errors when a class member has a disallowed modifier (e.g. readonly method() {})

cc @babel/typescript

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser area: typescript labels Sep 23, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the TypeScript 3.7 milestone Sep 23, 2019
@babel-bot
Copy link
Collaborator

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

@sandersn
Copy link

Note that the combined PR that I intend to merge is microsoft/TypeScript#33509.

Copy link

@hg-pyun hg-pyun left a comment

Choose a reason for hiding this comment

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

Looks good.

this.raise(startPos, `Duplicate modifier: '${modifier}'`);
}
modifiers[modifier] = true;
}
Copy link
Member

@Jessidhia Jessidhia Sep 27, 2019

Choose a reason for hiding this comment

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

This seems to allow any modifier whitelisted by the parameter in any order; I presume that's okay? or are there some modifiers which have a specific required order (e.g. declare first)

Copy link
Member Author

Choose a reason for hiding this comment

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

Some modifiers must come in a specific order, some others don't. For example, abstract and readonly can be swapped, but public must be before them.

We parse them using a logic similar to this one, which ensures the correct order:

this.tsParseModifiers(["public"]);
this.tsParseModifiers(["abstract", "readonly"]);

Copy link
Member

Choose a reason for hiding this comment

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

Not super important, but it might be nice to add a comment around modifier order/usage.

@@ -124,6 +127,24 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return undefined;
}

tsParseModifiers<T: TsModifier>(allowedModifiers: T[]): { [*]: true } {
const modifiers = {};
Copy link
Member

Choose a reason for hiding this comment

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

I'd feel better with Object.create(null) but it probably isn't really relevant here.

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Oct 3, 2019
@nicolo-ribaudo
Copy link
Member Author

I'll merge this to a separate branch so that I can start working on the transform.

@nicolo-ribaudo nicolo-ribaudo changed the base branch from master to feat-7.7/ts-declare-fields October 13, 2019 09:24
@nicolo-ribaudo nicolo-ribaudo merged commit 9cf1244 into babel:feat-7.7/ts-declare-fields Oct 13, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the ts-declare-field branch October 13, 2019 09:24
nicolo-ribaudo added a commit that referenced this pull request Oct 13, 2019
* [parser] Add support for TS declare modifier on fields

* Use Object.create(null)

* Comment
nicolo-ribaudo added a commit that referenced this pull request Oct 22, 2019
* [parser] Add support for TS declare modifier on fields

* Use Object.create(null)

* Comment
nicolo-ribaudo added a commit that referenced this pull request Nov 5, 2019
* [parser] Add support for TS declare modifier on fields

* Use Object.create(null)

* Comment
nicolo-ribaudo added a commit that referenced this pull request Nov 5, 2019
* [parser] Add support for TS declare modifier on fields

* Use Object.create(null)

* Comment
nicolo-ribaudo added a commit that referenced this pull request Nov 5, 2019
* [parser] Add support for TS declare modifier on fields (#10484)

* [parser] Add support for TS declare modifier on fields

* Use Object.create(null)

* Comment

* Add support for TS declare types to types and generator (#10544)

* Transform TypeScript "declare" fields (#10546)

* Transform TypeScript "declare" fields

* Remove multiple spaces

* declareFields -> allowDeclareFields

* Update after rebase
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 12, 2020