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

Fix some flow issues in @babel/parser flow plugin #8002

Merged
merged 2 commits into from
May 22, 2018
Merged

Conversation

existentialism
Copy link
Member

@existentialism existentialism commented May 22, 2018

No description provided.

@existentialism existentialism added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: parser labels May 22, 2018
@babel-bot
Copy link
Collaborator

babel-bot commented May 22, 2018

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

@@ -2455,6 +2456,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
node.callee = base;
node.typeArguments = this.flowParseTypeParameterInstantiation();
this.expect(tt.parenL);
// $FlowFixMe
Copy link
Member Author

Choose a reason for hiding this comment

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

Not great, but related to the TODO around converting arguments into $ReadOnlyArray, which requires quite a bit more changes.

@@ -541,6 +540,8 @@ export type ConditionalExpression = NodeBase & {
export type CallOrNewBase = NodeBase & {
callee: Expression | Super | Import,
arguments: Array<Expression | SpreadElement>, // TODO: $ReadOnlyArray
optional?: boolean, // For OptionalCallExpression only
Copy link
Member

Choose a reason for hiding this comment

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

Is moving this something we can avoid if we refactored the code to verify the type before setting the flag? Even if it's a pain, it seems like ideally we'd keep the types as specific as possible.

Copy link
Member Author

@existentialism existentialism May 22, 2018

Choose a reason for hiding this comment

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

Agreed on keeping types specific.

I went down a bit of a rabbit hole with trying to refactor for this last night... but actually after a second look, it seems like simply removing the CallExpression typecast makes flow happy.

@samwgoldman
Copy link
Contributor

Thanks so much @existentialism! I went on vacation right after pushing up those PRs, so I really appreciate your cleaning up my mess. I will be sure to keep Flow clean in the future.

storyn26383 pushed a commit to UniSharp/babel that referenced this pull request Aug 6, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants