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 initial flow typings for apollo-client #1688

Merged
merged 6 commits into from Jun 1, 2017
Merged

add initial flow typings for apollo-client #1688

merged 6 commits into from Jun 1, 2017

Conversation

jbaxleyiii
Copy link
Contributor

@jbaxleyiii jbaxleyiii commented May 10, 2017

I've added flow typings for the most commonly used API (I think I got all the critical bits of index.js) as well as updated the build script to package the typings in the npm bundle. This should mean that flow-typed and custom bindings are no longer needed for apollo-client.

TODO:

  • Update CHANGELOG.md with your change

@stubailo
Copy link
Contributor

Is there any way to test that these don't get out of date?

@jbaxleyiii
Copy link
Contributor Author

@stubailo not without writing out tests in js instead of ts. However, I think I can break this file apart and put the files next to their source files under src. This may make it easier to keep up to date. I'll work on that tonight

@stubailo
Copy link
Contributor

Could we maybe compile the tests, then run the compiled js tests with Flow and maybe catch some errors?

@helfer
Copy link
Contributor

helfer commented May 12, 2017

@jbaxleyiii I wouldn't break it apart. But it would be great if there was a way to check if the types are actually correct. For example, how do you know that there are no syntax errors in the flow types right now?

@jbaxleyiii
Copy link
Contributor Author

@helfer flow has a way of writing tests for the types. I can do that instead and configure the CI to verify it 👍

@helfer
Copy link
Contributor

helfer commented May 12, 2017

That would be great!

@helfer
Copy link
Contributor

helfer commented May 26, 2017

Any update on this @jbaxleyiii?

@jbaxleyiii
Copy link
Contributor Author

@helfer I didn't want to finish this until I new I would have some time to support the first couple releases in case there are issues with it.

I'm good to do that now so here are a few smoke tests and CI config!

@jbaxleyiii
Copy link
Contributor Author

After this is released, I can release apollographql/react-apollo#695

Now off to merge PRs / close some issues!

Copy link
Contributor

@helfer helfer left a comment

Choose a reason for hiding this comment

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

This is awesome @jbaxleyiii! I'm going to merge it now, but I still have one question: how are we going to make sure that the flow types stay in sync with the source? Do we have to go in and update the flow types for every PR that we make if it changes a type or a signature? If so, that might be very cumbersome and error-prone, even if we add it as a task to the PR checklist.

declare export class HeuristicFragmentMatcher
extends FragmentMatcherInterface {
constructor(): this,
ensureReady(): Promise<void>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like I forgot to remove this in an earlier PR. Once I do remove it, how will we make sure that the flow typings stay updated? Is the only way to guarantee it to test every little thing?

@helfer helfer merged commit 2f76f2a into master Jun 1, 2017
@helfer helfer deleted the flow branch June 1, 2017 18:52
@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.

None yet

3 participants