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 support for client in options #729

Merged
merged 9 commits into from Jul 5, 2017

Conversation

Projects
None yet
6 participants
@flexzuu
Copy link
Contributor

flexzuu commented May 26, 2017

Adds support for client in graphql HoC to support usecases with more than one ApolloClient

Proposal and discussion: #464
Other relevant PR: #481
Docs: WIP

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 tests and linter rules pass
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • 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
@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented May 26, 2017

@flexzuu: 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/

@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented May 26, 2017

Tests seem to not only dont work correctly on my setup but CI aswell:

No tests found
In /home/travis/build/apollographql/react-apollo
  2274 files checked.
  testMatch:  - 2274 matches
  testPathIgnorePatterns: /node_modules/ - 81 matches
  testRegex: <rootDir>/test/.*.test.(ts|tsx|js)$ - 0 matches
Pattern: "" - 0 matches
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files |  Unknown |  Unknown |  Unknown |  Unknown |                |
----------|----------|----------|----------|----------|----------------|

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented May 30, 2017

@flexzuu that is odd, can you try rebasing off of #695? It will be in master soon and works locally and on the CI for me?

@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented May 30, 2017

@jbaxleyiii hey thanks for merging i found some more stuff i needed to edit to finish this feature. will push this now. Will look into the failing tests and add some more tests if needed.
Some feedback about the PR overall is appreciated (It is my first "bigger" contribution to a OSS project)

@flexzuu flexzuu force-pushed the flexzuu:client-option branch 3 times, most recently from 5f6da37 to a79e5ac Jun 2, 2017

@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented Jun 6, 2017

Will work on adding test for the new option next. After that docs and changelog will be added.

@flexzuu flexzuu force-pushed the flexzuu:client-option branch from b0507ae to 922f1f8 Jun 19, 2017

@flexzuu flexzuu force-pushed the flexzuu:client-option branch from 8a3b81d to ea0f8d5 Jun 25, 2017

flexzuu and others added some commits Jun 25, 2017

@jbaxleyiii
Copy link
Member

jbaxleyiii left a comment

This is so great! Let's plan on updating the docs when this is released! YAY @flexzuu

@jbaxleyiii jbaxleyiii merged commit 23d131a into apollographql:master Jul 5, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 92.295%
Details
@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented Jul 6, 2017

@jbaxleyiii Hey thanks for mergeing. I will start working on the docs in my free time.

@adamsoffer

This comment has been minimized.

Copy link

adamsoffer commented Jul 24, 2017

This rocks! Until the documentation gets updated would one of you mind sharing what the syntax looks like for adding more than one client?

@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented Jul 27, 2017

graphql(query, {
  options: props => ({ client: ... }),
})(MyComponent)

Check back with me again if you have anymore questions. I am relatively busy right know will work on the docs next week.

@adamsoffer

This comment has been minimized.

Copy link

adamsoffer commented Jul 27, 2017

Neat. So you can initialize a bunch of clients and then decide which one you want to use on a component by component basis?

@ramrodo

This comment has been minimized.

Copy link

ramrodo commented Sep 21, 2017

Hey @flexzuu thanks for this contribution, I'm trying to implement this but I can't involucrate the two ApolloClient instances, do you have the documentation somewhere?

@vjpr

This comment has been minimized.

Copy link

vjpr commented Dec 11, 2017

@flexzuu Could we get some docs for this please?

@flexzuu

This comment has been minimized.

Copy link
Contributor Author

flexzuu commented Jan 23, 2018

@vjpr i completely reworked the idea and will publish some stuff about how to use it in the future.

The basic idea is that you don't use the option apollo provides but just use 2 providers and the standard graphql hoc then you would add in 2 tunnels that reexpose the context under a namespaced key. You can then use something like recomposes fromContext and withContext to connect to the namespaced context and reexpose it under the original context this way you can use the vanilla provider and vanilla hoc but you can use as many providers as you like. I will try to build a Tunnel lib out of this but if first need to see how well this approach hold up against real world use cases.

@estaub estaub referenced this pull request Jan 28, 2018

Closed

Document client prop #1588

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