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

Include flow comments in compiled output #17

Merged
merged 1 commit into from Jul 24, 2015
Merged

Conversation

zpao
Copy link
Member

@zpao zpao commented Jul 23, 2015

This should make it such that we can properly run flow in projects consuming fbjs.

(Sorry for the package.json noise, npm3 sorts on writing)

@zpao
Copy link
Member Author

zpao commented Jul 23, 2015

Well this can't be right… that diff definitely has some syntax errors. https://gist.github.com/zpao/29cc0a4bd03791d08cd1

I'll make this so that we run tests with this transform and catch things like that.

@zpao
Copy link
Member Author

zpao commented Jul 24, 2015

And now it's works because @sebmck is awesome.

zpao added a commit that referenced this pull request Jul 24, 2015
Include flow comments in compiled output
@zpao zpao merged commit bfd9b9c into facebook:master Jul 24, 2015
@yungsters
Copy link
Contributor

This change causes dependent projects to suddenly have many false flow errors. A cursory investigation shows that 1) flow does not like the code generated by babel (e.g. _createClass assigns properties to an object without a defined shape), and 2) generic types (e.g. Foo<T>) are not preserved and become invalid identifiers.

@zpao
Copy link
Member Author

zpao commented Jul 24, 2015

What's the best move? Revert?

@sebmck
Copy link
Contributor

sebmck commented Jul 24, 2015

Setting { "loose": "es6.classes" } should be enough to make Flow understand the class structure. Not entirely clear on Foo<T> but if it's a Babel issue let me know and I'll fix it.

@yungsters
Copy link
Contributor

Cool, I'll try { "loose": "es6.classes" }. Thanks!

I might have found another issue:

function everyObject(
  object: ?Object,
  callback: (value: any, name: string, object: Object) => any,
  context?: any,
): boolean {

...is being transpiled into...

function everyObject(object /*: ?Object*/, callback /*: (value: any, name: string, object: Object) => any*/, context? /*:: ?: any*/) /*: boolean*/ {

The problem is context?, which is invalid syntax.


For context, foo?: string is different from foo: ?string. The former is an optional argument that, if supplied, must be a non-null string. The latter is a required argument that is a nullable string. You can also have foo?: ?string, which is an optional argument that, if supplied, is a nullable string.

@sebmck
Copy link
Contributor

sebmck commented Jul 24, 2015

That'd be because the flow transformer has been blacklisted which is responsible for stripping those. Fixed via babel-plugins/babel-plugin-flow-comments@df836e49e846fbec375d7f4de5f8c0a182333487 and released as of babel-plugin-flow-comments@1.0.9.

@yungsters
Copy link
Contributor

Great. I'll give babel-plugin-flow-comments@1.0.9 a spin.

I'll open an issue for the generic types on https://github.com/babel-plugins/babel-plugin-flow-comments.

@yungsters
Copy link
Contributor

Opened babel-plugins/babel-plugin-flow-comments#15.

@zpao zpao mentioned this pull request Jul 30, 2015
@zpao zpao deleted the flow-comments branch August 31, 2015 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants