-
Notifications
You must be signed in to change notification settings - Fork 32
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
Expose Flow Types #7
base: master
Are you sure you want to change the base?
Conversation
@@ -8,6 +8,9 @@ node_modules | |||
build | |||
dist | |||
|
|||
# flow | |||
!dist/index.js.flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem quite right. I don't think anything in the "dist" folder should be committed to Git. Too likely to be stale.
@@ -33,7 +33,7 @@ | |||
], | |||
"main": "dist/index.cjs.js", | |||
"module": "dist/index.esm.js", | |||
"files": ["dist"], | |||
"files": ["src", "dist"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would bloat the size of every NPM install, and not provide any value other than exposing the types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely happy with this either but this is the "supported" solution currently. I think exposing types is a big value for users though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that publishing types might add value for users, but I don't personally think it's worth it to publish the whole source in this way.
@@ -0,0 +1,2 @@ | |||
// @flow | |||
export * from '../src/index.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about instead of this, the build script explicitly gather the flow types and save them in dist/index.js.flow
so they're automatically included with the NPM bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking for such solution for my libraries but couldn't find one. Flow gen-flow-files, the official solution for it, is completely broken (facebook/flow#5871). Maybe there is something to do from within Facebook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunate
This nags me. I've reached out on Twitter to see if anyone has better ideas. |
Resolves #6