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 generic types to apollo client for Query and Mutation #914

Conversation

brettjurgens
Copy link

@brettjurgens brettjurgens commented Nov 15, 2016

Fixes #777

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@helfer
Copy link
Contributor

helfer commented Nov 15, 2016

@kamilkisiela do you have a moment to review this PR? I'm asking since you initially had the idea for #777

@kamilkisiela
Copy link
Contributor

kamilkisiela commented Nov 15, 2016

@helfer I don't get the idea. Why we specify those types while creating an instance of ApolloClient?

ApolloClient<QueryType, MutationType>

and

public watchQuery(...): ObservableQuery<QueryType, MutationType>

With this approach, every watchQuery() will return an object of the same type, even if each query looks differently.

My idea was to specify generic type for each individual watchQuery, query and mutate methods.

For example:

apollo.watchQuery<Entry[]>({query: feedQuery});
// returns { data: Entry[], loading: boolean, ... }

apollo.watchQuery<Entry>({query: entryQuery, variables: { id: 2 }});
// returns { data: Entry, loading: boolean, ... }

@brettjurgens
Copy link
Author

@kamilkisiela is this more what you're looking for?

Copy link
Contributor

@kamilkisiela kamilkisiela 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 be a breaking change if it comes to TypeScript based projects.

Right now we use it like this:

apollo.watchQuery({ query });

It will cause a warning about a missing generic type, so it's up to you guys if you want to release it in v0.5.X.

@brettjurgens
Copy link
Author

unfortunately, generics cannot be made optional (there's an issue here: microsoft/TypeScript#2175), so yes, it will be a breaking change, but quite an easy one to fix (just add <any>)

@kamilkisiela
Copy link
Contributor

@brettjurgens Yes yes, it's not that I see it as a big problem, it's just a breaking change that will screw up few projects if we will push it as 0.5.X. With 0.6.0 we can mark it as a Breaking Change to inform people and even with a release candidate, they can be prepared for it.

@brettjurgens
Copy link
Author

agreed, should not be released in a patch version. 0.6.0 makes sense

@stubailo
Copy link
Contributor

This update sure would go well with working TypeScript code gen: apollographql/apollo-tooling#5

@brettjurgens
Copy link
Author

@helfer
Copy link
Contributor

helfer commented Jan 4, 2017

@brettjurgens Sorry I did't get around to doing anything with this PR until now! Do you think you could you change the base of this PR to zero-decimal-six and resolve all the conflicts in the next few days? If so, then we can merge it and release it with version 0.6. Otherwise I think we should probably close the PR and open a new one down the road, because it will only get harder to merge the PR as it gets more out of date with the rest of the code.

@kamilkisiela
Copy link
Contributor

@brettjurgens yes, it would be super cool. I could make changes also in angular2-apollo in just one release.

@brettjurgens
Copy link
Author

cool, will rebase on 0.6 right now

@brettjurgens
Copy link
Author

@helfer just rebased it and the diff between this branch and the zero-decimal-six one looks clean

@helfer helfer changed the base branch from master to zero-decimal-six January 5, 2017 04:26
@brettjurgens
Copy link
Author

@helfer tests are passing now, seems i missed a conflict or two when the base changed. should be all good now

@helfer
Copy link
Contributor

helfer commented Jan 6, 2017

@brettjurgens wonderful, thanks for your efforts! It always feels great to merge the oldest PR 😌

@helfer helfer merged commit e1180e3 into apollographql:zero-decimal-six Jan 6, 2017
@brettjurgens brettjurgens deleted the add_generics_to_apollo_client branch January 6, 2017 21:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 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