Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

add some typings to graphql HOC #383

Merged
merged 10 commits into from
Jan 31, 2017

Conversation

brettjurgens
Copy link
Contributor

@brettjurgens brettjurgens commented Dec 22, 2016

fixes #379

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
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@apollo-cla
Copy link

@brettjurgens: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -52,6 +54,11 @@ export declare interface QueryOptions {
skip?: boolean;
}

export interface InjectedGraphQLProps<T> {
data?: T;
loading?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also have functions like fetchMore etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, i can add those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realized this should be more like

{
  data?: T & { loading: boolean, /* etc */ }
}

anyway

@brettjurgens
Copy link
Contributor Author

@stubailo added the additional functions & updated the type

@jbaxleyiii
Copy link
Contributor

jbaxleyiii commented Jan 5, 2017

@brettjurgens this is so great!

We updated types to match apollo-client. Would you mind fixing the conflicts?

@ianks
Copy link

ianks commented Jan 5, 2017

Urgh! I totally missed this PR when opening #394 :/

}

export interface InjectedGraphQLProps<T> {
data?: T & GraphQLDataProps;
Copy link

Choose a reason for hiding this comment

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

This could also be mutate, or whatever the user passes in for operationOptions.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fine IMO, because GraphQLDataProps is also exported. that way the user can just make their own interface i.e.

import { GraphQLDataProps } from 'react-apollo';

interface MyCustomInterface<T> {
  whatever?: T & GraphQLDataProps
}

@brettjurgens
Copy link
Contributor Author

@jbaxleyiii just fixed the conflicts

@helfer
Copy link
Contributor

helfer commented Jan 10, 2017

@ianks does this PR cover everything that's in #394? If so, I think we could close that one.

@ianks
Copy link

ianks commented Jan 10, 2017

@helfer it does not. The other PR is different, as it tries to address the issues from the core rather than the typings.

@brettjurgens
Copy link
Contributor Author

@helfer any update here?

@helfer
Copy link
Contributor

helfer commented Jan 27, 2017

@jbaxleyiii let us know if you're good with merging this, and I'll merge and deploy it 🙂

@jbaxleyiii
Copy link
Contributor

@helfer I am!

@helfer helfer merged commit f7db93f into apollographql:master Jan 31, 2017
@brettjurgens brettjurgens deleted the add_typings_to_wrapper branch January 31, 2017 22:15
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.

Component loses typings with HOC
6 participants