This repository has been archived by the owner. It is now read-only.

exact object type annotations for Flow plugin #104

Merged
merged 4 commits into from Sep 13, 2016

Conversation

Projects
None yet
6 participants
@bhosmer
Copy link
Contributor

bhosmer commented Sep 1, 2016

Adds support for exact object type annotations in the Flow plugin. Exact object types are delimited by {| and |}, but are otherwise identical in syntax to ordinary object types.

Replicates the functionality added to Flow here.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 1, 2016

Current coverage is 94.13% (diff: 100%)

Merging #104 into master will decrease coverage by 2.64%

@@             master       #104   diff @@
==========================================
  Files            19         19          
  Lines          3130       3068    -62   
  Methods         320        320          
  Messages          0          0          
  Branches        800        802     +2   
==========================================
- Hits           3029       2888   -141   
+ Misses          101        100     -1   
- Partials          0         80    +80   

Powered by Codecov. Last update dc56c0b...e9c9e00

}

case 125:
++this.state.pos; return this.finishToken(tt.braceR);

This comment has been minimized.

@danez

danez Sep 2, 2016

Member

Needs 2 space indentation.

@@ -510,7 +523,8 @@ pp.flowParsePrimaryType = function () {
return this.flowIdentToTypeAnnotation(startPos, startLoc, node, this.parseIdentifier());

case tt.braceL:
return this.flowParseObjectType();
case tt.braceBarL:
return this.flowParseObjectType(false, true);

This comment has been minimized.

@danez

danez Sep 2, 2016

Member

If the token === tt.braceLshouldn't the call be this.flowParseObjectType(false, false);. It probably doesn't really change anything but it would be more clear maybe (and saves us one comparison in the flowParseObjectType, although it probably doesn't have an impact.)

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 2, 2016

Thank you for bringing the new feature immediately to babylon. We recently are a little bit behind the flow specs with other features, and I'm really happy to see more help in bringing the new features to babylon.

I have two small nitpicks (see comments) but otherwise looks really good. To make the complete chain of babel work with this change there also needs to be a PR to babel that makes babel-generator aware of the exact field so the syntax can be printed again. Could you make these changes there?

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 3, 2016

@danez Sure, will make the requested changes here and in babel-generator. Thanks for the quick response Daniel!

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 6, 2016

@danez Hi Daniel - incorporated your suggestions. Not sure what to do about the coverage numbers, if anything - I'd have thought the new tests would cover the new code paths, but LMK if there's something else to be added.

Re babel-generator, I've got the new code in my fork, but since the new tests choke the old parser, it seems like I should hold off on a PR until this change here makes its way over there, right?

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 6, 2016

@danez PR to babel-generator is here.

But I think we may need to bump Babylon's version to reflect the new support, and then specify the new version in babel-generator's dependencies, otherwise the babel-generator tests I added which use {| |} will fail. Not sure how big a version bump is; alternatively, I could just remove the new tests from the PR. LMK which you'd prefer!

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 6, 2016

Coverage looks good and you can create the PR already for babel. The tests will fail there of course but that is okay for features that span over both repos.

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 6, 2016

Ah you were faster with your reply ;) yes this is now perfectly fine. We are going to merge the change in babel after babylon gets released.
Thank you

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 6, 2016

@danez ah, ok cool - thank you Daniel!

case 125: ++this.state.pos; return this.finishToken(tt.braceR);

case 123:
if (this.hasPlugin("flow") && this.input.charCodeAt(this.state.pos + 1) == 124) {

This comment has been minimized.

@jamiebuilds

jamiebuilds Sep 12, 2016

Contributor

=== 124

This comment has been minimized.

@bhosmer

bhosmer Sep 12, 2016

Author Contributor

(y)

@jamiebuilds

This comment has been minimized.

Copy link
Contributor

jamiebuilds commented Sep 12, 2016

This is looking good. @hzoo @danez could you push this through?

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 12, 2016

Looks like the coverage would be for a failing test on unexpected token?

https://github.com/babel/babylon/pull/104/files#diff-8ce9de97e7fb2bd7f30e9eee69714584R406

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 13, 2016

@hzoo added test and coverage looks taken care of, I think. Not sure why the build failure though, or how to kick.

@hzoo hzoo merged commit ddbda7d into babel:master Sep 13, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/patch 100% of diff hit (target 96.77%)
Details
codecov/project Absolute coverage decreased by -2.64% but relative coverage increased by +3.22% compared to dc56c0b
Details
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 13, 2016

I restarted got the same failure - should be ok

@bhosmer

This comment has been minimized.

Copy link
Contributor Author

bhosmer commented Sep 13, 2016

@hzoo cool, thanks!

@gabelevi

This comment has been minimized.

Copy link
Contributor

gabelevi commented Sep 15, 2016

@hzoo - thanks for merging! Would you mind publishing this to npm? Thanks!

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 15, 2016

Sorry been busy not doing open source 😄, can do it real quick

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 19, 2016

@gabelevi sorry for delay, released now!

ghost pushed a commit to facebookarchive/nuclide that referenced this pull request Sep 19, 2016

Update to eslint@3.5.0 and eslint-plugin-react@6.2.2
Summary: New rules and fixes. Most noteworhy though is that babylon@6.10.0 is now pulled in. This version has support for "exact object type notation" (see babel/babylon#104), that is `{| |}`.

Reviewed By: nmote

Differential Revision: D3888815

fbshipit-source-id: 963aa5fcbe920d4380e43d5e93bfc1d0bb7f3be6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.