[Bug] Fresh data gets overwritten with stale data from __queryCache

[zenflow]

(almost) minimal reproduction repo

https://github.com/zenflow/mst-gql-next/tree/demo-bug-stale-data-reused

I removed almost all of the code unrelated to demonstrating this bug. Pardon the next.js integration bit; it has zero impact here.

The relevant parts are in server/schema.js and pages/index.js

steps to reproduce

1. Open up the app
   
2. Click the "Show" button under the "Done Todos" heading. This fetches todos from a separate query in the graphql schema, doneTodos.
   
3. Click the "Hide button. The screen should look the same as in step 1.
4. Toggle one or more of the todos.
   
5. Click the "Show" button under "Done Todos" again.
   

You can see that the todo items I toggled have been reverted to their original state. 💩

This example is using the default fetchPolicy "cache-and-network", so immediately after clicking "Show", the stale cached query (from when we opened the "Done Todos" the first time) is used for an instant before the freshly fetched query is received, and in using it, (here's the problem), it's model properties/data are written to the "living" model instances in store.todos.

The bug is easier to observe with the "cache-first" fetchPolicy, but I wanted to demonstrate that this is fairly severe bug in the typical/basic usage (i.e. not doing anything special).

I believe mst-gql should simply (?) never copy data from the __queryCache onto root type model instances, since that data will never be more fresh than the data the model instance already has (from other queries, and even optimistic updates). Really it seems that __queryCache shouldn't need to keep properties/data of root types at all, and in the case of query results that are an array of root types, the entry value (in __queryCache) should be just an array of IDs. That would also make things much more efficient when serializing/deserializing state for SSR, localStorage, etc.

Is this likely to be fixed?

BTW, it would be awesome if mst-gql had a model instance for the root graphql type Query {} that worked generally the same as the other graphql models, and thus would:

• Be able to replace the __queryCache (since the Query model would have the same necessary data from __queryCache)
• Be part of the normalized data model. No duplicate (stale) data. References are used in place of actual data for other "root types".
• Allow for optimistic updates to properties of the root graphql Query type! That would mean we can optimistically update all parts of our query results. Not just properties of [other] root type instances, but also which instances are included in query results, and non-root type data in query results. Taking the reproduction repo app for example, in the optimistic update function for toggling a todo, we could (in addition to toggling the done property) add/remove it to/from the store.rootQuery.doneTodos array (of course only if store.rootQuery.hasLoaded('doneTodos')) and then our list of "Done Todos" would stay up to date as we make changes!

Thanks for your work on a super awesome and powerful package. ❤️ 💪

[zenflow]

I was thinking about taking a try at fixing this sometime in the next few days, although I have no typescript experience (yet).. Somebody may need to help with or correct that aspect of my changes to get it merged, but it would at least get the ball rolling.

The changes I would like to make:

• Don't ever copy data from the __queryCache onto root type model instances. Only copy data from query results [onto root type model instances] when those results are from network (not cache).
• Filter/remove [now-] unused data from __queryCache, so root type objects in query results [in __queryCache] have no properties except __typename and id (the two that are needed to find the object among our cached model instances).

@mweststrate @chrisdrackett @dpnolte @mattiasewers Please let me know if you disagree with that change in principle, or if for any other reason I shouldn't bother working on this. Thanks for your time :)

[chrisdrackett]

@zenflow I haven't had a chance to look at this issue yet. I might have some time later this week to download the code and dig through.

[chrisdrackett]

@zenflow I think this issue is worth looking at for sure. This is one of the first things I ran into when using mst-gql as well. In our projects we "get around" it by never using data from useQuery in any of our components and instead only ever referencing the store directly.

First off IMO the following suggestions:

• Don't ever copy data from the __queryCache onto root type model instances. Only copy data from query results [onto root type model instances] when those results are from network (not cache).
• Filter/remove [now-] unused data from __queryCache, so root type objects in query results [in __queryCache] have no properties except __typename and id (the two that are needed to find the object among our cached model instances).

are solid and I think we should do them.

It seems to me that if you are using GraphQL is a more read-only way the current behavior of the cache is good and does not have many problems. However, if you are often mutating data this pattern breaks down as the cache is very often going to be out of date in one way or another. The above starts to address this.

If you want to get a PR going I'm more than happy to look at it!

[zenflow]

Looks like my second suggested change ("Filter/remove [now-] unused data from __queryCache...") did not take into account the raw: true option for queries. These queries currently depend on having fully populated objects in __queryCache . I bring up this problem and a proposed solution in PR #97.