Using offlineExchange with storage option using metaData methods swallows errors

[labithiotis]

I am experiencing an issue when trying to utilise offline caching for gql client. I have project setup to persist cache to a storage solution and include the metaData methods when I do this, queries that fail, the error value is always undefined I am never seeing a CombinedError object.

If I comment out readMetadata inside the function to makeLocalStorage which is used inside offlineExchange then it works.

I have set up a code sandbox with a working example of the issue. The example is set up to hit an endpoint that doesn't exist to it "fails" always.

https://codesandbox.io/s/urql-bug1-nlzbf?file=/src/graphql/makeLocalStorage.ts

urql version & exchanges:	package	version
urql	1.11.6
@urql/exchange-graphcache	3.4.0
graphql	15.5.0

Steps to reproduce

1. Load code sandbox example
   • Observe the error is undefined (prints null to page)
2. Comment out readMetadata inside /src/graphql/makeLocalStorage.ts
   • Observe the error is now a CombinedError

I added a clear cache button to page as you need to clear storage if you re-enable readMetadata else error is persisted and "appears" to works.

I have also noticed that if readMetadata is enabled the request is always doing a "cache-only" request even though I set in my query for it to be "cache-and-network"

[kitten]

Yep, that's to be expected and part of more opinionated parts of our cache and persistence.

Errors aren't cacheable. They're information on what happened at any given point in the API and explanations for why no data was delivered, especially since a query can partially fail, instead of completely. Hence it doesn't make sense to cache errors or to redeliver them.

For Graphcache and the Document Cache this means that errors are never cached, which also means for Graphcache that none of them are persisted.

[labithiotis]

It makes total sense that errors are not persisted in the cache, I get that.

However, even if I set the query to network-only I still don't get an error?

Also if I do use cache-and-network and start with empty cache I still see a network request which I would expect as the client first queried cache and found nothing and then does network request (which failed). I would still expect the component to get the error?

[kitten]

Oh, I see what you mean now. In the absence of a network connection (and one the triggers for that catches; an invalid URL can trigger the same) we'll execute all queries offline.

The question then is what happens if there's no data at all? I guess previously we'd argue that it's unexpected that Graphcache would then sometimes let the error fall through (specifically when it's offline and there's no data) and sometimes it wouldn't (when partial data is available)

I guess, both can make sense, because data == null && fetching is still a good indicator since it wouldn't make sense to handle the offline / network error throughout the application everywhere where you're fetching data. But that's likely indeed up for discussion.

[ibash]

Related to this — when using an updater resolver is it possible to tell if an error was returned? As far as I can tell a response like this from the server, it's impossible to tell that an error happened in the update resolver:

{
    "data": {
        "myMutation": null
    },
    "errors": [
        {
            "extensions": {
                "code": "INTERNAL_SERVER_ERROR",
                "exception": {
                    "stacktrace": [
                      "..."
                    ]
                }
            },
            "locations": [
                {
                    "column": 3,
                    "line": 2
                }
            ],
            "message": "This is an error!",
            "path": [
                "myMutation"
            ]
        }
    ]
}

[kitten]

@ibash Both parts are very intentional and come down to how server-side data should be stored and are handled.

While we can argue that data that hasn't been returned because the server yielded an unexpected error in the query (which is admittedly rare and rarely intentional, which is why we haven't addressed this), the same doesn't go for updates.

An update on a mutation field directly mostly signals that an update was unsuccessful, which means that a normalised cache should do nothing. So generally what this comes down to is that our normalised cache, Graphcache, has one guiding, opinionated principle: Staying true to server data.

It only ever reflects the data that the server delivers accurately. When for instance a mutation fails we'd take this as a signal that nothing in the server data has changed.

[ibash]

Hmm, I guess the trouble was I wasn't expecting the update resolver to be called in the error case, so I was ending up with bad data in the cache. For now, I'm just checking if it returns null and returning early.

I would much rather have the error available in the update resolver so that I can ensure there aren't error cases that are unexpected.

So, I don't disagree with making the cache reflect the server, but I do think hiding errors from users of the library is going to lead to more brittle and broken software.

[labithiotis]

I'm going to step back from this issue as my situation was caused as you say by using a bad URL - the issue was a little contrived and only encountered as I did it to shortcut issue for development testing. In the "real" world it would work as expected as it wouldn't be offline and would get errors. Happy if you wish to close this issue, or continue the other discussions raised around the empty data or resolvers.

[kitten]

I think there's still a good point in this issue. I believe we can do some investigation work on our end for two parts:

• For writing to the cache, correlate GraphQLErrors to paths in the response and treat them as undefined rather than null, e.g. as uncached rather than a null value.
• Once we correlate the error we can investigate whether there's a good API extension to let mutation updaters see the error, although it may have to be hidden away a little to allow this to be used in exceptional circumstances rather than most updaters.

[kitten]

I'm still doing some testing but code review is complete for #1356, so if you'd like to try these changes out you can find pre-builds of the Graphcache PR on Codesandbox CI (you may copy the package link and place it in a "resolutions" entry or "dependencies" directly in your package.json to install it) https://ci.codesandbox.io/status/FormidableLabs/urql/pr/1356/builds/98282

What you'll find is that it'll try to mark fields that have an associated GraphQLError to them that are null as uncached instead. Furthermore for errored fields that have updaters for them for mutations (under updates), the updater can now access info.error to see the field's specific error.