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

Add flow-types to library level #1820

Merged
merged 10 commits into from
Jul 7, 2017
Merged

Add flow-types to library level #1820

merged 10 commits into from
Jul 7, 2017

Conversation

michalkvasnicak
Copy link
Contributor

@michalkvasnicak michalkvasnicak commented Jun 23, 2017

This PR adds support for flow type without need to configure .flowconfig in a project.

It's basically the same as in apollographql/graphql-tag#97.

This is good for library authors too, for example I wanted to typecheck in my library react-apollo-graphql but I didn't want to force users to configure their .flowconfig just to typecheck apollo-client's stuff. So I made my own typings.

With this PR I can get rid of my own types and use yours :)

Closes #1819

@hunaczech
Copy link

+1

@michalkvasnicak
Copy link
Contributor Author

I tested this in our fairly big project. Fixed a few errors.

For example Request definitions was causing an error because you can't have optional fields and then [extra: string]: any. See try flow.

Also I changed BatchingNetworkInterfaceOptions and NetworkIntefaceOptions definitions to be compatible. So you can assign BatchingNetworkInterfaceOptions to NetworkIntefaceOptions. And changed definition for NetworkInterfaceOptions.uri to string instead of ?string. I thinks this is good, because NetworkInterface is throwing and error if you don't define uri so we can typecheck this so user will get an error sooner than in runtime.

@michalkvasnicak
Copy link
Contributor Author

michalkvasnicak commented Jun 23, 2017

Btw index.js.flow could be splitted to multiple files e.g. /transport/networkInterface.js.flow, etc. So keeping it in sync with typescript definitions could be easier and more readable in the future.

But I'm not sure if something like this won't introduce a breaking change because package.main will point to index.js instead of apollo.umd.js.

@stubailo
Copy link
Contributor

stubailo commented Jul 3, 2017

I'll leave this to @jbaxleyiii to look at soon!

@jbaxleyiii
Copy link
Contributor

@michalkvasnicak I'll take a look at this today! I agree that breaking it apart would be much better! We talked about doing it the first time around but opted to ship it as an enhancement / make it optional in order to get feedback from users like yourself! Thank you so much for the work on this!

I'll review it and report back today!

@jbaxleyiii
Copy link
Contributor

@michalkvasnicak are you using react-apollo as well? It would be great to do the same setup there if you don't mind!

@jbaxleyiii
Copy link
Contributor

@michalkvasnicak great work! Thank you so much for this!

@jbaxleyiii
Copy link
Contributor

@michalkvasnicak this is great! If you could update the changelog, I'll merge it in and it can be part of the next release!

@michalkvasnicak
Copy link
Contributor Author

@jbaxleyiii I don't use react-apollo directly, but we use it in another project, I can take a look at flow stuff there as well.

@jbaxleyiii jbaxleyiii merged commit ce3ef05 into apollographql:master Jul 7, 2017
@jbaxleyiii
Copy link
Contributor

@michalkvasnicak thank you again for this! It will be released as 1.8!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants