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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client scoped query recyclers (#513) #876

Merged
merged 2 commits into from Aug 13, 2017

Conversation

Projects
None yet
3 participants
@rcoedo
Copy link
Contributor

rcoedo commented Jul 21, 2017

This PR addresses the issue #513. I used a WeakMap as suggested in this conversation #462 (comment), but I'm not sure if we should polyfill WeakMap.

I tried the code linking the package to my playground and it works, but I'm a bit lost with the test structure of the project and I don't really know how to write a test for this feature.

Also I'm also quite new to Apollo and I've never touched Typescript before so I really appreciate all the feedback you can give 馃槃

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Jul 21, 2017

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

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Jul 25, 2017

@rcoedo thank you for this PR! I'll take a look at it today and provide some feedback!

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Jul 25, 2017

@rcoedo the code for this looks great! I'll see if I can write up a test scenario for you with some guidance of how to test! 馃憤

@jbaxleyiii jbaxleyiii self-assigned this Jul 25, 2017

@rcoedo

This comment has been minimized.

Copy link
Contributor Author

rcoedo commented Jul 26, 2017

Thanks for reviewing it @jbaxleyiii

I've been thinking about this isssue and having these WeakMaps all around as global application state seems kind of hackish. It works but I think there has to be a better solution.

Maybe a better approach would be to have a Map<Component, Recycler> in the ApolloProvider component and pass the recycler via context, so the thing would really be scoped by provider and not just by client (two providers using the same client wont share the recyclers). In fact, the whole thing could be a new RecyclerProvider that wraps the current ApolloProvider and handles that context.

any thoughts?

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Jul 31, 2017

@rcoedo I do think adding it to the context could be good, but I'm nervous about updating causing rerenders based on context updates. By keeping it out of the tree, we don't risk updates.

I'd rather not introduce new providers (at least to the external API). What would the updating look like? Could we write a test to ensure it wouldn't rerender?

I do think having it on the context would be great btw!

@rcoedo

This comment has been minimized.

Copy link
Contributor Author

rcoedo commented Aug 1, 2017

I think it wont cause rerenders since the context will only hold a reference to a Map and that reference will never change.

The provider I propose is internal only, since we don't want to leak the recycler implementation details. For safety, in that provider we could implement shouldComponentUpdate as a function that always return false.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Aug 1, 2017

@rcoedo that sounds great! Would you update this PR to move to that version?

@rcoedo rcoedo force-pushed the rcoedo:master branch 3 times, most recently from 80c53c2 to 8fea657 Aug 8, 2017

@rcoedo rcoedo force-pushed the rcoedo:master branch from 8fea657 to c0a6b22 Aug 8, 2017

@rcoedo

This comment has been minimized.

Copy link
Contributor Author

rcoedo commented Aug 8, 2017

@jbaxleyiii I updated the PR. Current tests are passing and I manually tested it out in my playground and it seems to be working.

If you feel like we need more testing I'd really appreciate some guidance, there are a lot of use cases that I probably miss since I'm not experienced with apollo or react-apollo.

Thanks! 馃槃

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Aug 10, 2017

@rcoedo YAY! This is so great! I'm going to do a release to address some flow type issue then take a look this week! Thank you!!

James Baxley
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Aug 13, 2017

@rcoedo I think this is a great start. We can come back and increase testing later but I'm going to merge and release this 馃憤

@jbaxleyiii jbaxleyiii merged commit 93ba6ee into apollographql:master Aug 13, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
Danger All green. Yay.
Details
bundlesize ./dist/index.min.js: 11.81kB < maxSize 12kB gzip (155B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcoedo

This comment has been minimized.

Copy link
Contributor Author

rcoedo commented Aug 14, 2017

@jbaxleyiii Thanks for the kind words 馃槃

I've been thinking and with this implementation the recyclers will be reset when the client changes in the ApolloProvider. I'm not sure how are users using this. Is it common to bounce from client to client in the same application? If this is the case, we may need to keep those recyclers alive as well, and use a Map<ApolloClient, Map<Component, QueryRecycler>> instead to store the state.

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