Apollo-client: [Question] Objects within list change after mutation of the list

[danielgriese]

Hi there,

I did not find a related discussion, so I've opened a new issue. Not sure though if it is an issue or intended behaviour (just not mine).
Thanks in advance for shutting some light on this and any kind of explanation!

I've been trying Apollo on a simple list to test paging and simple mutations.
For simplicity assume the following easy query-result structure...

[
 { id: "item-1", value: "a" },
 { id: "item-2", value: "b" },
 { id: "item-3", value: "c" },
...
]

This result is passed to a List component and each entry is then passed to ListItem components. So far so good.

I then delete "item-2" via a mutation.

const id = 'item-2';
mutate({
        variables: { id },
        resultBehaviors: [
          {
            type: 'DELETE',
            dataId: id,
          },
        ],
      }),

That's pretty neat, as the item is automatically removed from the list and of course the list re-renders due to the changed list. But all my remaining list items are re-rendered as well!

Although all other objects within the list have not been touched by the mutation, they get re-created somehow (new reference, same values) and a simple === check in shouldComponentUpdate of my list item does not help anymore...

The same goes for paging. When incorporating fetchMore results into the list, all the previous items of the list seem to be re-created as well. And thus re-render.
I am implementing paging to reduce the required network-traffic of course, but also to reduce the amount of items I need to render in one go (10, then 10 more, etc), but now it renders 10, then 20, 30, ... which is not what I want to achieve.

When implementing such things in previous projects, I used immutable.js for my stores, especially due to performance concerns.
I went with the assumption that the Apollo Client would do these things under the hood (out of the box). Or at least the remaining items should be left untouched.

I am pretty confused about this behaviour. Maybe I am missing a simple point and did a mistake.
Any explanation on that matter would be highly appreciated!

Thanks and kind regards,
Daniel

[stubailo]

Hi, that's how it works because Apollo is re-generating the query result from the store every time. This could be fixed by making the query execution take into account the previous result, and make sure that any unchanged objects are equal to before.

Is this rendering causing a performance bottleneck for your app?

[danielgriese]

Hi,

Thanks for the clarification!
Wasn't sure if that's how Apollo works internally or if I used it wrong ;)

At this stage it is not a performance bottleneck (yet), as these are simple lists and simple tests.
Within in the real project it will most probably be an issue as the ListItem components can become kind of complex (DOM-wise) and even regenerating the virtual dom elements (+diffing) is intensive (takes seconds for 100+ items). Just imagine scrolling through a Facebook-Feed and the old stories at the top of the list re-render while loading new items.

So I'd really, really like to avoid re-rendering 100 items, when actually only 10 new items appear on the list. It very much seems avoidable...
Maybe new React versions with off-screen lists will help too.

Earlier I used a) immutable objects and pure renderers to avoid re-renders and b) also react-virtualized to avoid needing render 100+ items initially.

I don't like the b) work-around from a UX perspective, it's always kind of quirky to integrate.
With paging I get around the need for b), but a) would still be the case.

So it is at the moment not a show-stopper, but I guess the more complex applications using Apollo will get, the more this might become a requirement.

Thanks and kind regards,
Daniel

[stubailo]

Yeah, I think there are definitely pretty reasonable solutions for this, but it's a more "advanced" optimization in my mind.

Perhaps it's something we can do after a refactor, we're planning on making the execution much better soon.

[stubailo]

Some implementation notes:

1. QueryManager is the thing that reads from the store and decides whether or not to fire the query observable, here: https://github.com/apollostack/apollo-client/blob/ee24f3d3dd5867b70286a9256fdc95fefac128e1/src/core/QueryManager.ts#L391
2. It uses readQueryFromStore to read from the store here: https://github.com/apollostack/apollo-client/blob/ee24f3d3dd5867b70286a9256fdc95fefac128e1/src/core/QueryManager.ts#L378
3. readQueryFromStore eventually comes down to calling graphql-anywhere here: https://github.com/apollostack/apollo-client/blob/1eed528ea3781d060e896d71c17acac223740281/src/data/readFromStore.ts#L174
4. this is the package, which we also own: https://github.com/apollostack/graphql-anywhere, it's a fairly generic and simple GraphQL query executor
5. Possible solutions include:
   
   
   
   1. graphql-anywhere could acquire an extra previousResult option, which would maintain references in cases where the result is the same
   
   
   2. apollo-client could try to use the resultMapper option to compare the previous result to the new one
   
   
6. Ideally, this would let us replace the isDifferentResult function to avoid doing a deep comparison of the results.

[kachkaev]

We're also facing this issue (see #945) and it causes pretty large chunks of virtual DOM to be re-rendered. Let's say you have three lists with 1000 items in each and you only mutate one attribute in one item. Even if you carefully implement updateQueries and patch only one little node out of the whole data tree, all related components fail to shallow compare their props.

We'd be really glad to see this fixed in Apollo client!

[armstrjare]

+1 on this. This would make a big performance impact in Angular2 with OnPush change detection.

[armstrjare]

@stubailo regarding your suggested approaches, I suspect that it would be simplest to allow maintaining object references via the graphql-anywhere call. Presumably other users of the lib will have similar problems.

Wondering if we could modify graphql-anywhere to allow passing a Builder object that creates the return value by being walked across for each graphql field. A simple builder would just create a new object. But, passing a custom instance of Builder would allow the builder to maintain a previous object and re-use object references where possible.

You could also implement optimising builders, for cases where from the resolver value can be determined to be unchanged (i.e. by a versionId or object reference in the case of immutable values, etc.), the builder can let the graphql iterater avoid walking down that object path by instead using the prevoius value (assuming it's the exact same query).

Further, perhaps that builder could merge in the Mapper functionality of the existing graphql-anwhere implementation.

So in essense the core graphql-anywhere simply iterates over the graphql query fields, calling the resolver, and passing the mapped field along with the resolved value into the builder to "do it's thing". Then, graphql() simply returns builder.getValue()

This would also allow alternate implementations graphql-anywhere results, like using Immutable.js etc.

Thoughts?

[helfer]

@benjamn Did you have any time to look into this for graphql-anywhere yet? I think it would be nice if we could fix this for 1.0

[calebmer]

Referential equality for data returned by the Apollo Client was implemented in https://github.com/apollostack/apollo-client/pull/1136 and released in 0.7.x so now to see if data has changed you can simply use === 👍

Let me know if this isn’t enough.