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

Expose Flow Types #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+6 −1
Open

Expose Flow Types #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96405c3
Allow a Flow file
iddan Aug 5, 2018
8e9806f
Expose types from src
iddan Aug 5, 2018
a9ec842
Deploy src folder
iddan Aug 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ node_modules
build
dist

# flow
!dist/index.js.flow
Copy link
Owner

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.


# misc
.DS_Store
.env
Expand Down
2 changes: 2 additions & 0 deletions dist/index.js.flow
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// @flow
export * from '../src/index.js'
Copy link
Owner

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?

Copy link
Author

@iddan iddan Aug 5, 2018 •
edited
Loading

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?

Copy link
Owner

Choose a reason for hiding this comment

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

That's unfortunate ☹️ I'm not aware of anything internal.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
],
"main": "dist/index.cjs.js",
"module": "dist/index.esm.js",
"files": ["dist"],
"files": ["src", "dist"],
Copy link
Owner

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.

Copy link
Author

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.

Copy link
Owner

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.

"scripts": {
"precommit": "lint-staged",
"prettier": "prettier --write '**/*.{js,json,css}'",
Expand Down