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.

Sign up for GitHub

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

Jump to bottom

Fix private property parsing in Flow #8340

Merged
merged 9 commits into from
Aug 2, 2018
Merged

Fix private property parsing in Flow #8340

merged 9 commits into from
Aug 2, 2018

Conversation

kalenikalexander
Copy link
Contributor

Q                       A
Fixed Issues? Parse private class properties with Flow types
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link No
Any Dependency Changes? No
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 18, 2018 •
edited
Loading

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

existentialism
existentialism reviewed Jul 18, 2018
View reviewed changes
packages/babel-parser/test/fixtures/flow/type-annotations/140/input.js Outdated
@@ -0,0 +1,4 @@
class A {
Copy link
Member

Choose a reason for hiding this comment

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

I know we do a really bad job of it, but can we give this folder a name? Maybe class-private-property or something?

@existentialism
Copy link
Member

existentialism commented Jul 18, 2018 •
edited
Loading

We might also want to make a note to bump https://github.com/babel/babel/blob/master/Makefile#L2 to make sure we're syncing with flow's parsing of private props.

@kalenikalexander
Copy link
Contributor Author

kalenikalexander commented Jul 19, 2018 •
edited
Loading

I added private_class_properties/multiple.js and private_class_properties/super.js to whitelist because parser doesn't do semantic analysis of private properties.

@radex
Copy link

radex commented Jul 28, 2018

Hey, what's the status here? This seems to be the last bit necessary to make private properties work nicely when using Flow.

nicolo-ribaudo
nicolo-ribaudo reviewed Jul 28, 2018
View reviewed changes
packages/babel-parser/src/types.js Outdated
@@ -716,6 +716,7 @@ export type ClassPrivateProperty = NodeBase & {
value: ?Expression, // TODO: Not in spec that this is nullable.
static: boolean,
computed: false,
typeAnnotation?: ?TypeAnnotationBase, // TODO: Not in spec
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this type be TypeAnnotation?

@existentialism
Copy link
Member

existentialism commented Aug 1, 2018 •
edited
Loading

Parsing looks good, should we also add tests to generator and flow-strip-types to make sure all is well?

Can also land this now and follow up.

@kalenikalexander
Copy link
Contributor Author

I should put flow-strip-types tests in fixtures/regression or fixtures/strip-types directory?

@existentialism
Copy link
Member

@kalenikalexander let’s do strip-types

@nicolo-ribaudo nicolo-ribaudo merged commit 5c728ea into babel:master Aug 2, 2018
@nicolo-ribaudo
Copy link
Member

Thank you!

storyn26383 pushed a commit to UniSharp/babel that referenced this pull request Aug 6, 2018
@kalenikalexander @nicolo-ribaudo
Fix private property parsing in Flow (babel#8340)
a8db834 
* Fix private property parsing in Flow

* Flow tests updated

* Fix type error

* Appropriate name was given to test folder

* Fix

* Empty

* Correct type annotation

* Add required changes in generator package

* Add required changes in flow-strip-types
@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.
Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kalenikalexander @babel-bot @existentialism @radex @nicolo-ribaudo