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 recycling for `ObservableQuery`s #462

Merged
merged 6 commits into from Feb 16, 2017

Conversation

Projects
None yet
6 participants
@calebmer
Copy link
Contributor

calebmer commented Feb 14, 2017

This PR is our first attempt to solve the updateQueries regression introduced by apollo-client version 0.5.22 in a bug fix while still trying to keep the bug that got fixed, well, fixed.

The issue is that when updateQueries or a reducer references data for a component which has been unmounted then there is no way to update data in the store at that location. So say you had an app where the user moved to a comment entry screen and your comments list component was unmounted. The user enters a comment and submits a mutation with an updateQueries that references the query for the unmounted comments list component. The updateQueries function would not run in this environment because of our bug fix.

Before 0.5.22 line 502 of src/core/ObservableQuery.ts did not remove the ObservableQuery from QueryManager even when it was added earlier along in the observable query’s lifecycle. This behavior was incorrect and caused apollographql/apollo-client#993. So we fixed it! However, as a side effect of not removing the ObservableQuery from QueryManager any ObservableQuerys on unmounted components were updated. In short, the bug had a desirable side affect!

This PR aims to mitigate the problem which caused apollographql/apollo-client#993 while still providing the desirable updateQueries behavior that many of our users expect. In order to strike this balance this PR implements ObservableQuery recycling. Here’s how it works.

When a component is unmounted, instead of throwing away the ObservableQuery, we put it in a recycler to keep it alive. You can think of it like putting the ObservableQuery in the fridge to be reheated later. When a new component mounts we take our saved ObservableQuery out of the fridge, add some new options, and use the old observable query again.

Now for every container component in your app (you create one whenever you call graphql) there should only be one ObservableQuery that lives forever assuming that you only render our container component once at a time. If you render your component more than once at a time that should still be fine.

When ObservableQuery is in the recycler it will receive any updateQueries and reducer store changes like normal. The only difference is that no component will re-render.

There are probably going to be a few bugs associated with this change. My assumption is that it will be much easier to smash those smaller bugs as they come up instead of letting this updateQueries bug live on. Those bugs will also likely be much smaller in scope.

Here are a few of the issues which can provide context on the issue. I may be missing some:

Basically, there were a lot of bug reports 😊. We didn’t fix this sooner because we saw the bug fix that caused this regression as technically correct, and we’re working on moving towards apollo-client 1.0.

I just want to write one more test to assert that the updateQueries behavior is ok. (because that’s why this PR was opened!)

  • 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

@helfer helfer added the in progress label Feb 14, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 14, 2017

Ironically, now the “♻️” in the repo description means something 😊

@Siyfion

This comment has been minimized.

Copy link

Siyfion commented Feb 15, 2017

Wow, @calebmer this sounds perfect for what is required! Like you said, I assume there will be some minor bugs associated with this change, but hopefully they'll be much easier to squash than the main updateQueries bug.

I for one will be pulling this change asap and will start testing immediately.

calebmer added some commits Feb 15, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 15, 2017

Ok, just added the test which tests the actual behavior we want to fix with this PR (updateQueries not running for unmounted queries). If everyone feels comfortable with the changes made, let’s release 👍 (cc @helfer)

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Feb 16, 2017

@calebmer I think this is a great approach, there's just one thing I have a question about. Since we are keeping an open subscription to each query, we are going to be reading the results out of the store even though we aren't going to be using them. If the queries are very large and there are a lot of them that could create some CPU impact.

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 16, 2017

@stubailo this is an unfortunate side effect. We could disable this by exposing some way in apollo-client to disable updating certain observers maybe observers that don’t have observer.next, or an observer with a private property: observer._keepAliveOnly. It’s fixable, but I think it’s only fixable on the apollo-client side.

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Feb 16, 2017

Personally I think I would solve this by adding a flag in Apollo client watchQuery which makes the observable "hot" and doesn't track subscribing and unsubscribing, then always pass that flag in React-Apollo. It doesn't seem like it would take a lot of work and the benefits could be worth it.

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Feb 16, 2017

We could merge and publish this first then release the other thing as an optimization.

@helfer

helfer approved these changes Feb 16, 2017

Copy link
Member

helfer left a comment

This is great @calebmer, thanks a lot for implementing a fix that is a viable workaround for most people!

I have some comments about the tests, but I think we can ship this as is. I'm fine with keeping an observable subscription around for each component that was previously mounted, but we should make a note for the fix in Apollo Client if performance becomes an issue. In the medium term we should move to hot observables that have to be explicitly stopped and removed.

* An observable query recycler stores some observable queries that are no
* longer in use, but that we may someday use again.
*
* Recycling observable queries avoids a few nasty bugs that may be hit when

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

I think it would be helpful to link to the issue here, which has a fuller description. I also don't think it's right to describe this as a bug, I would rather call it a missing feature (the ability to update parts of the store that have no active query watching them). Also, calling reducers/updateQueries multiple times may actually be necessary to reach all corners of the cache, so that could be considered a feature rather than a bug. Those nuances are what made this issue so tricky in the first place.

It's also important to note that recycling query observables will still not provide the ability to update parts of the cache for variables more than one step back.

This comment has been minimized.

@calebmer

calebmer Feb 16, 2017

Author Contributor

It's also important to note that recycling query observables will still not provide the ability to update parts of the cache for variables more than one step back.

Did not think about that as a use case we should look at supporting. I don’t think the cost of keeping around all observable queries is worth that, however. Hopefully imperative read and write methods will allow users to implement this themselves where necessary.


this.observableQueries.push({
observableQuery,
subscription: observableQuery.subscribe({}),

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

We should modify apollo-client so an active subscription isn't necessary to keep the query in the store.


wrapper.unmount();
done();
}, 10);

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

How are you picking the timeouts? It's 10 here, it was 5 in the other tests. it might be good if there was a constant for these somewhere.

This comment has been minimized.

@calebmer

calebmer Feb 16, 2017

Author Contributor

Arbitrarily picking numbers for no reason 😉

This is a very unimportant number that could go anywhere from 0–1000 and the test would behave the same.

expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1', '2']);
const queryObservable3 = (client as any).queryManager.observableQueries['1'].observableQuery;
const queryObservable4 = (client as any).queryManager.observableQueries['2'].observableQuery;
expect(queryObservable3).toBe(queryObservable1);

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

Apollo Client queryIds are strictly incrementing, so looking for observableQueries[1] twice will always either return the same object or undefined if that observable query has been removed. So I think testing that queryObservable3 is equal to queryObservable1 doesn't actually show anything.

This comment has been minimized.

@calebmer

calebmer Feb 16, 2017

Author Contributor

If queryObservable3 could be undefined if it was removed, then this test does show something, right?

What I really wanted to test was the ObservableQuery instance directly on the components, but that value is really hard to get.

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

Yeah, then maybe compare directly to undefined and write a comment about what we'd really like to check here.

}, 10);
});

it('will not recycle parallel GraphQL container `ObservableQuery`s', done => {

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

I'm not sure what this test is trying to do. Is it supposed to show that if you have the same query rendered twice you actually get two different observable query objects? If so, why is there a remount() in there?

This comment has been minimized.

@calebmer

calebmer Feb 16, 2017

Author Contributor

Yep. It shows that if you render two components with the same query you get two observable queries, but if you unmount and remount one of the components it will recycle that observable query so that in total you only ever have two observable queries.

@@ -394,4 +394,218 @@ describe('mutations', () => {
renderer.create(<ApolloProvider client={client}><Container id={'123'} /></ApolloProvider>);
});

it('will run `updateQueries` for a previously mounted component', () => new Promise((resolve, reject) => {

This comment has been minimized.

@helfer

helfer Feb 16, 2017

Member

This test is really long and keeps track of a lot of stuff, so it would be useful to have a short comment here that summarizes what it does.

@Siyfion

This comment has been minimized.

Copy link

Siyfion commented Feb 16, 2017

I've been testing this since @calebmer made the PR and so far, so good, no new bugs have been found and it definitely resolves the issue.

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Feb 16, 2017

@calebmer I've added you to react-apollo on npm. Feel free to merge this and release when you consider it ready.

@calebmer calebmer merged commit 7ee3397 into master Feb 16, 2017

5 checks passed

./dist/index.min.js +539 bytes (+1.95%) → 27,686 bytes
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 94.896%
Details

@calebmer calebmer deleted the refactor/observable-queries branch Feb 16, 2017

@helfer helfer removed the in progress label Feb 16, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Feb 16, 2017

Released in 0.11.1 🎉

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Mar 6, 2017

Rather than making the recycler global, would it make sense to attach it to the provider (or the client) @calebmer?

I just got really confused in a testing scenario where I was setting up a new provider and new client for each test case, but queries get reused anyway.

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Mar 6, 2017

@tmeasday There is one recycler per every HOC component. So it isn’t technically global, but every HOC component has one and only one recycler, but I do see the problem 😊

A solution would be to, as you suggest, create a WeakMap<ApolloClient, ObservableQueryRecycler> and then we use the recycler for whichever client reference we have at the time. I think since we assume Promises exist in the environment we can also assume WeakMaps exist? Because both objects were part of the ES2015 specification. We should really be using Maps more in apollo-client as well…

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Mar 7, 2017

Could we move the const recycler = new ObservableQueryRecycler(); line inside the class GraphQL? -- that way there'd be one per component instance -- which would also fail in some cases (when the props of the ApolloProvider changed without unmounting for instance), but at least would be easily avoidable via the use of key={xyz}

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Mar 7, 2017

No, the point of the recycler is that observable queries will be recycled across component instances. If we remove that capability then it is basically pointless and we are back where we started as far as the updateQueries issue goes 😣

I think the WeakMap on instances of ApolloClient is safe as long as we are comfortable with requiring a ES2015 collections polyfill (which I think we should for apollo-client). We could also have the recycler ask for an instance of client on recycle and reuse (or queryManager). Then it would only reuse an observable query if the client is the same.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Mar 8, 2017

Makes sense, I opened: #513

@booboothefool

This comment has been minimized.

Copy link

booboothefool commented Mar 26, 2017

I was locked down on apollo-client 0.5.21, react-apollo 0.7.3 for a while due to apollographql/apollo-client#1129, however updateQueries still not does seem to work as expected (doesn't fire) after performing the following upgrade. I'd really to like to start taking advantage of the new imperative store API without having to rewrite my entire app. :)

screen shot 2017-03-26 at 12 15 54 am

Is there a step I'm missing?

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Mar 26, 2017

@booboothefool I noticed you are using React Native. Maybe you need to clear the cache?

@booboothefool

This comment has been minimized.

Copy link

booboothefool commented Mar 26, 2017

@calebmer I'm pretty sure everything is installed correctly / cache cleared, etc.

I even tried the new update: (proxy, mutationResult) and that works fine for a bunch of cases, however my old updateQueries still not firing.

There might be something wrong with my implementation for this particular part I am trying it on because update gives me "Store error: the application attempted to write an object with no provided id but the store already contains an id of ____ for this object". If I simply ignore that, I run into: apollographql/apollo-client#1437

apollo-client 0.5.21, react-apollo 0.7.3: updateQueries fires and updates like I want it to, no issues

apollo-client 1.0.0-rc.6, react-apollo 1.0.0-rc.3: update gives me that error for this particular scenario, but I've gotten update to work in other places

The scenario is very similar to what you mentioned here:

So say you had an app where the user moved to a comment entry screen and your comments list component was unmounted. The user enters a comment and submits a mutation with an updateQueries that references the query for the unmounted comments list component. The updateQueries function would not run in this environment because of our bug fix.

If I downgrade back to 0.5.21, react-apollo 0.7.3, updateQueries just works with no code changes.

UPDATE: So I was able to get updateQueries working for another, similar scenario where the component gets unmounted. I'm trying to figure out what is so special about the case where it isn't working (but still works in apollo-client 0.5.21, react-apollo 0.7.3)...

UPDATE2: Strange, the query in question doesn't even get the new results with refetchQueries, however I can see the new results coming in through dateIdFromObject. What does this mean?

UPDATE3: The difference is fragments (for the query where updateQueries doesn't fire). I believe this might be the same issue: apollographql/apollo-client#1436

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