urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.63k stars 451 forks source link

[cache-and-network] Bug : __typename absent from stale data #1366

Closed strblr closed 3 years ago

strblr commented 3 years ago

When getting stale data from the graphcache, __typename is absent, but it should actually be there according to this discussion. This caused a bug in one of my apps which relied on that field to sort some data.

urql version & exchanges:

Steps to reproduce

CodeSandbox Link

1) Wait for the list to load a first time, 2) Click on "Unmount and Remount List", 3) The second time the list shows up, stale data is used at first, and it doesn't have __typename.

Expected behavior

That __typename is present.

Actual behavior

It's not when getting stale data. One has to wait for the actual network response to get the __typename back, which isn't always suitable (like in my case, where I initialize states with stale data, etc.).

Thank you for your work, and for taking this issue into consideration.

kitten commented 3 years ago

That's expected behaviour. If you're using the __typename field, especially on the root type, you need to query it in your query document explicitly.

strblr commented 3 years ago

Why would that be expected ? There is a mismatch between the actual old data and the stale data, wouldn't it just be more consistent to just return the same data one had before and therefore expects ? I have lots of files full of GraphQL documents without __typename, it would feel wrong to patch just what I need ; I could add it everywhere of course, but why not just enforce consistency ?

kitten commented 3 years ago

It's partly a limitation and partly expected; basically, it's not limited to stale data but data in general as it comes back from the API. If you look at the query document you write, you haven't actually listed __typename, however, internally Graphcache (and normalised caches in general) rely on the __typename field to do their normalisation and traversal work. So what happens is that before the query is forwarded in the exchange pipeline and picked up by the fetchExchange to be sent to your API it'll call formatDocument which adds __typename fields.

Generally on the side of querying from the cache this does not happen and instead the document remains unchanged. When we receive an API result however the result is written to the cache, we make sure that things like resolvers are applied to the result, and hence read the result back from the cache immediately. However, we wouldn't have access to the original query document by this point for consistency and practical reasons. This is generally because the time of the query operation (the synchronous start) and the result (the asynchronous future of when the API result comes back) lie apart.

But the tl;dr here is that this actually increases consistency: Rather than adding the field whenever the cache sees fit it actually only adds the __typename field when the query document contains it. This may vary from the query document as it's formatted before it's sent to the API but in the grand scheme of how the logic works here it does exactly traverse the fields as expected, which is actually something we added to increases consistency (rather than always adding the __typename field no matter what)

So, if you depend on the __typename field in your UI, i.e. if the field is a data requirement of your component, then it should be added to the query document / selection sets.

strblr commented 3 years ago

@kitten Thank you for the explanation ; wouldn't it therefore be more consistent to not send __typename past the cache exchange down to the hook at all when not explicitly listed in the documents ? You could still consume it at cache-level for normalization purposes, but without confusing the developer into making false assertions about the content of his data. I would also argue that this deserves more explanation in the documentation. Please let me know what you thoughts are.

kitten commented 3 years ago

That's not possible though. Once we transform the document to add the type name fields it'll go to the API, the exchange then generates a result and we can read the document back from it.

At that point we only have access to that document. The alternative is to store the old document instead in a map; but not only is that kind of a hackt solution, we'd also prevent exchanges from being created that add or change the fields on a document before they go to the API.

strblr commented 3 years ago

Got it !