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

Allow ApolloProvider Prop Updates #479

Merged
merged 8 commits into from Mar 17, 2017

Conversation

Projects
None yet
6 participants
@wmcbain
Copy link
Contributor

wmcbain commented Feb 24, 2017

We have a use case in React Native where we switch the GraphQL endpoint dynamically while the app is running. This PR changes the ApolloProvider API by updating it's client and store when new props are delivered to the provider.

TODO:

  • 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
@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Feb 24, 2017

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

@wmcbain wmcbain force-pushed the ZXVentures:allow-provider-prop-updates branch from 530a883 to 8138b99 Feb 24, 2017

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Feb 24, 2017

Nice! This looks like a great change.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 24, 2017

@wmcbain if you can add a couple tests for this, we can merge it and release it! Thanks!

@calebmer
Copy link
Contributor

calebmer left a comment

A couple of comments, looks good though 👍

Also needs some tests so we can make sure that this behavior never regresses 😊

this._init(props);
}

componentWillReceiveProps(nextProps) {

This comment has been minimized.

@calebmer

calebmer Feb 24, 2017

Contributor

I think this should be componentDidUpdate. We also wouldn’t need to pass in props in that case.

This comment has been minimized.

@calebmer

calebmer Feb 24, 2017

Contributor

Also, we should probably put this in an if that checks if we actually have a new client.

@@ -61,7 +69,6 @@ export default class ApolloProvider extends Component<ProviderProps, any> {
// intialize the built in store if none is passed in
props.client.initStore();
this.store = props.client.store;

This comment has been minimized.

@calebmer

calebmer Feb 24, 2017

Contributor

Shouldn’t the client and store be state instead of class properties? Or better yet, why do we even need them to be class properties? We can just call initStore on them when they change.

@@ -10,7 +10,9 @@ import {
Store,
} from 'redux';

import ApolloClient from 'apollo-client';
/* tslint:disable:no-unused-variable */
import ApolloClient, { ApolloStore } from 'apollo-client';

This comment has been minimized.

@wmcbain

wmcbain Feb 24, 2017

Author Contributor

TS compiler was throwing an error without ApolloStore not being in scope when potentially returning an ApolloStore in getChildContext.

This comment has been minimized.

@stubailo

stubailo Feb 24, 2017

Member

Yeah this is one weird edge case in TS

@@ -3,6 +3,7 @@
Expect active development and potentially significant breaking changes in the `0.x` track. We'll try to be diligent about releasing a `1.0` version in a timely fashion (ideally within 1 or 2 months), so that we can take advantage of SemVer to signify breaking changes from that point on.

### vNext
- ApolloProvider now changes it's client and store when those props change. [PR #479](https://github.com/apollographql/react-apollo/pull/479)

This comment has been minimized.

@stubailo

stubailo Feb 24, 2017

Member

It's -> its

@wmcbain

This comment has been minimized.

Copy link
Contributor Author

wmcbain commented Feb 24, 2017

Ready for review

@wmcbain wmcbain force-pushed the ZXVentures:allow-provider-prop-updates branch 2 times, most recently from b964e63 to 0a36251 Feb 24, 2017

@calebmer
Copy link
Contributor

calebmer left a comment

Looks good to me. I’d just like to see one or two more tests that assert GraphQL container components query the correct client and/or when the client changes re-queries (or not) against the new client.

@wmcbain wmcbain force-pushed the ZXVentures:allow-provider-prop-updates branch from 0a36251 to 70fa2ec Feb 27, 2017

expect(initStoreMock).not.toHaveBeenCalled();
});

it('child component should be able to query new client and store when props change', () => {

This comment has been minimized.

@wmcbain

wmcbain Feb 27, 2017

Author Contributor

@calebmer Test added

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Feb 28, 2017

@calebmer I'll let you finish reviewing this and include it in the next release.

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 1, 2017

The new test is good, but I really want to see something that uses the graphql HOC 😊

When the client changes, does the HOC make a new request to the new client? When the client changes and then refetch is called from the HOC, is the request executed with the new client?

These are behaviors I’d like to see asserted with tests.

@wmcbain

This comment has been minimized.

Copy link
Contributor Author

wmcbain commented Mar 1, 2017

@calebmer I'm not sure if I'm familiar enough with this library to provide the depth of tests that you are looking for. I'm struggling to provide these tests within reasonable time as a result.

Is there an example that you can point to that would help me see what you're looking for?

Wyatt McBain and others added some commits Feb 24, 2017

Wyatt McBain
Allow ApolloProvider Prop Updates
* Change client and store when new props are passed to the ApolloProvider
* Update Changelog
Wyatt McBain
Requested PR changes
* Requested ApolloProvider changes
* Add tests
Wyatt McBain

@wmcbain wmcbain force-pushed the ZXVentures:allow-provider-prop-updates branch from 4a70834 to 7d91e47 Mar 16, 2017

Wyatt McBain

@wmcbain wmcbain force-pushed the ZXVentures:allow-provider-prop-updates branch from 7e8a9b9 to c18528c Mar 16, 2017


// Both cases fail
expect(initialInterface.query).not.toHaveBeenCalled();
expect(nextInterface.query).toHaveBeenCalled();

This comment has been minimized.

@wmcbain

wmcbain Mar 16, 2017

Author Contributor

Hey @calebmer, revisited this problem after learning a bit more and running into a bug that fit the test case you described exactly in this PR. I'm wondering now if you foresaw this being an issue?

Any help/insight is appreciated.

This comment has been minimized.

@calebmer

calebmer Mar 17, 2017

Contributor

I added a better test and fixed the bugs the test revealed. I’ll merge once CI passes 👍

@calebmer calebmer merged commit af53290 into apollographql:master Mar 17, 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.2%) to 92.321%
Details
@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 17, 2017

Released in 1.0.0-rc.3 🎉

@wmcbain

This comment has been minimized.

Copy link
Contributor Author

wmcbain commented Mar 17, 2017

Thanks!

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