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

Imported npm package throws errors within flow #25

Closed
rricard opened this issue Jul 5, 2015 · 5 comments
Closed

Imported npm package throws errors within flow #25

rricard opened this issue Jul 5, 2015 · 5 comments

Comments

@rricard
Copy link

rricard commented Jul 5, 2015

Since the transformations applied before exporting the package to npm strips the flow annotations but not the comments, a lot of flow errors arise after a npm install graphql.

Here's some of them:

/Users/rricard/src/github.com/rricard/td-foundation/lib/td-mirror/node_modules/graphql/lib/error/index.js:37:25,31: parameter message
Missing annotation

/Users/rricard/src/github.com/rricard/td-foundation/lib/td-mirror/node_modules/graphql/lib/error/index.js:39:3,7: parameter nodes
Missing annotation

/Users/rricard/src/github.com/rricard/td-foundation/lib/td-mirror/node_modules/graphql/lib/error/index.js:39:10,14: parameter stack
Missing annotation

/Users/rricard/src/github.com/rricard/td-foundation/lib/td-mirror/node_modules/graphql/lib/error/index.js:73:23,27: parameter error
Missing annotation

/Users/rricard/src/github.com/rricard/td-foundation/lib/td-mirror/node_modules/graphql/lib/error/index.js:73:30,34: parameter nodes
Missing annotation

...


... 121 more errors (only 50 out of 171 errors displayed)
To see all errors, re-run Flow with --show-all-errors

One solution would be to strip the /* @flow */ comments from the npm package. But we would lose all of the useful information contained inside those annotations. If we had a transform able to put the annotations in a comment, that would be much better...

Anyway, I'm open to discuss the best way to fix this so I can start working on a pull-request and/or a new transform for flow annotations!

@rricard
Copy link
Author

rricard commented Jul 5, 2015

Here is a quick fix if you want to continue to work before that issue gets fixed:

Add this to your .flowconfig:

[ignore]
.*/node_modules/graphql/lib/.*

EDIT Nope, it doesn't work. The imported module won't exist anymore for flow...

EDIT2 Add this to your Makefile or in a package.json hook or anything you want to use:

cd node_modules/graphql && find . -type f -print0 | xargs -0 sed -i '' 's/@flow//g'

This will strip the @flow from the GraphQL-js's files. Take this as a temporary solution.

@leebyron
Copy link
Contributor

leebyron commented Jul 6, 2015

Sorry about this. I will bring this up with the Flow team to get their best advice

@rricard
Copy link
Author

rricard commented Jul 6, 2015

Oh don't worry! I deactivated the error with my small sed regex. It will be
fixed when it needs to be. Thanks for bringing this up to the flow team on
this though! Again, I can write the patch if it's not top priority for you
at fb.

On Mon, Jul 6, 2015, 03:28 Lee Byron notifications@github.com wrote:

Sorry about this. I will bring this up with the Flow team to get their
best advice


Reply to this email directly or view it on GitHub
#25 (comment).

@leebyron
Copy link
Contributor

With the recent change to babel, the /*@flow*/ header is now removed, so hopefully this solves your issues.

I'll continue to look into nicer ways to improve this going forward.

@angelf
Copy link

angelf commented Apr 7, 2016

It would be briliant to have flow annotations preserved. For instance in facebook/draft-js they leave the original code in /lib/ and place the generated plain javascript in /build/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants