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 definitions in npm package #359

Closed
alexanderchr opened this issue Apr 18, 2016 · 14 comments
Closed

Include flow definitions in npm package #359

alexanderchr opened this issue Apr 18, 2016 · 14 comments

Comments

@alexanderchr
Copy link

I'm currently using a cloned repo to get flow typings but it would be really nice if these were included in the npm package.

The downside is of course that you'd have two copies of the code. Another option would be to provide untranspiled code as a separate package. How do other libs with flow definitions handle this?

@leebyron
Copy link
Contributor

leebyron commented May 7, 2016

Agreed that this would be great.

I'll investigate the current best practice that would avoid us having to maintain a separate type definitions file and export these correctly.

@JohnyDays
Copy link

I have managed to locally make it work, just by copying every JS source file, alongside it's respective output JS file as .js.flow

@iamchenxin
Copy link
Contributor

iamchenxin commented Jun 6, 2016

@alexanderchr @JohnyDays I made a solution (flow-dynamic ) for this . See pr:89 i push to Graphql-relay(detail codes) to resolve flow type casting between Packages.
this pr:89 is a demonstration for Linking graphql's Flow Type with another packages,resolve type conflict will be easy,no more need to worry about how to make them 100% static resolved.
@leebyron This solution will have no extra produce-time load. And when some type conflict will be static resolved by Flow in future,only need to remove the corresponding wrappers(do not break any code inside). maybe it is time to give us a .js.flow output package. A typed graphql will no more break any another package used graphql.Those packages just need wrap a check function out side their type conflict functions(and will get the right Flow Type).
maybe publish a beta version flow-type output graphql for us frist,then we can use an official package,not our own copy.

@leebyron
Copy link
Contributor

leebyron commented Aug 24, 2016

Update: we're going to make this work! Here's the plan:

The Flow team is supportive of the idea of introducing flow types directly in npm packages. Right now the suggestion is to use the same approach @JohnyDays and @nodkz suggested in #412, however as I understand it in the future they hope to include a tool alongside flow which produces higher quality exported flow types.

My primary concern is safely introducing flow types errors for people who are using graphql-js and flow, but have not yet had an opportunity to check for flow errors. I also would like an opportunity to create a window of time for interested people to contribute feedback about the quality of the flow types before introducing them for all.

Here's the upcoming release plan:

v0.7.0: Introduce all latest improvements and changes to graphql-js, but no flow-types.
v0.8.0-beta: Shortly after, a beta release will contain flow-types.
v0.8.0: After some time for people to test and report any issues, a new non-beta release will contain flow types.

(cc @robzhu and @jeffmo who helped arrive at this plan)

@JohnyDays
Copy link

Awesome! Looking forward to ditching my ad-hoc solution. Cheers

@clintwood
Copy link

@leebyron, while slightly off topic, it would be great to include the source in the package and include jsnext:main pointing to that source for tools like RollUp. Thanks.

@leebyron
Copy link
Contributor

leebyron commented Nov 1, 2016

Some exciting news on this front! v0.7.0 was released a couple weeks ago, and after flow v0.34, the graphql-js codebase should be correctly typed again, so I'm looking into releasing a pre-release for people to test.

@nodkz
Copy link
Contributor

nodkz commented Nov 1, 2016

If somebody wants to test NULL in args and flowtype in their app, I built graphql-js package on commit 2b717f1. Just run:

npm install https://www.dropbox.com/s/zrd3b00dmx3jhae/graphql-0.7.2.tgz\?dl\=1

PS. Yarn does not work with this link (((

@wincent
Copy link
Contributor

wincent commented Nov 3, 2016

@leebyron: Do we need to do some additional configuration to include the *.flow files in the released package? I can't see them in the 0.8.0-beta1 release.

I don't know how the Travis-mediated publishing works, but I can see we're doing some manual transforming and copying in resources/prepublish.sh and resource/npm-git.sh. I am not sure where build and therefore build-dot-flow get run in relation to all that.

@leebyron
Copy link
Contributor

leebyron commented Nov 3, 2016

You're right @wincent something went wrong with the npm deploy and the flow files went missing. I'm looking into it :/

@leebyron
Copy link
Contributor

leebyron commented Nov 4, 2016

Hey @wincent I just published 0.8.0-beta3 which made sure the flow files were in there. Let me know what you learn

@leebyron
Copy link
Contributor

leebyron commented Nov 4, 2016

@nodkz at any point you can also depend directly on git! https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge

@leebyron
Copy link
Contributor

leebyron commented Nov 4, 2016

Closing this issue since this is out in beta now - please provide feedback!

@leebyron leebyron closed this as completed Nov 4, 2016
@JohnyDays
Copy link

I'll update my projects and check it out. Thanks @leebyron

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

No branches or pull requests

7 participants