Urql: retryExchange is causing data: undefined

[frederikhors]

After https://github.com/FormidableLabs/urql/pull/985 I updated https://codesandbox.io/s/urql-svelte-crud-offline-fetching-stale-data-rj22w adding retryExchange like this:

const retryOptions = {
  initialDelayMs: 1000,
  maxDelayMs: 10000,
  randomDelay: false,
  maxNumberAttempts: 5,
  retryIf: (err) => err && err.networkError
};

const exchanges = [
  dedupExchange,
  offlineExchange({ storage }),
  retryExchange(retryOptions),
  fetchExchange
];

Now you have a REPL to understand why retryExchange is causing variable $todos to be: { fetching: true, stale: false, data: undefined } when offline after one go and back from list to detail navigation.

Also I think this: trying to see the problem from me, as urql user (not developer!) I think I need to have stale: true when I'm offline or server is down.

I think is correct.

What is NOT correct is data: undefined. And maybe also fetching: true because retryExchange is trying to get it from server.

What do you think about?

[frederikhors]

You can use Incognito mode to try and make sure the server is started (the API calls doesn't restart it).

[kitten]

I'm thinking there isn't enough details here to fully jump on this.
@wgolledge worked on this but I don't think he'll be able to get an idea of when and how this occurs.

In the spirit of the issue template for bugs, can you please list what conditions are a factor, e.g. different options, removing retry, etc.

Also we'll need expected behaviour and actual behaviour in more detail.

The stale note is irrelevant to this report and we likely won't set it to true for offline results because it'll only be true when a newer result can be expected, which isn't the case for offline.
If you need to detect whether you have an offline result specifically (offline mode for an app) then you need to write logic for that state.

We'd also need some idea of what you tried so far, e.g. where you see results flowing through the exchanges and how far they'd get.

We can't exclude the possibility that this isn't an issue with @urql/svelte and if that's confirmed I'll have to close this issue for now, since that's still in alpha with several issues being unresolved at the moment, since its implementation is very temporary.

But tl;dr I can't send someone hunting for a bug without preliminary details when it may also be an issue with Svelte 😅

[frederikhors]

urql version & exchanges:

"@urql/exchange-graphcache": "3.1.1",
"@urql/exchange-retry": "0.1.8",
"@urql/svelte": "0.4.0",
"graphql": "15.3.0",
"graphql-tag": "2.11.0",
"svelte": "3.25.1",

Steps to reproduce

1. Open browser in Incognito mode
2. Go to the Reproduction server
3. See if it's started by trying a query with GraphQL playground
4. Go to the Reproduction Client (here the source code)
5. Open browser's Dev Tools
6. Click on the first one todo (the todo is there with { fetching: false, stale: false, data: [object Object] })
7. Put your browser offline
8. Click again on "Todos list" (the list is there with { fetching: false, stale: true, data: [object Object] })
9. Click on the first one todo (the todo is there with { fetching: false, stale: true, data: [object Object] })
10. Click on "Todos list" again (the list is NOT there with { fetching: true, stale: false, data: undefined } )

Expected behavior

If the browsers is offline or the server is down I need to see my cached data.

I also need retryExchange because I need to check if browser or the server is back online.

Actual behavior

I can NOT see my (stale) cached data during the offline state (data: undefined).

(Before https://github.com/FormidableLabs/urql/pull/985 I had issues also with dedupExchange, now this is just a problem with retryExchange).

Additional, my opinion

I think we need to have an indicator for this situation (stale or fetching both is fine for me):

"Hi user, it's me, @urql, I'm trying to get the freshest data from the server for you but apparently something is stopping me. It seems to be a network error, ok, so IN THE MEANTIME I will show you what I have in cache and IN THE MEANTIME I ask my trusty retryExchange to check if the impediment disappears and I can retrieve the data from the server."

@kitten said:

we likely won't set it to true for offline results because it'll only be true when a newer result can be expected, which isn't the case for offline.

I Instead think it is: I can expect a newer result! But I'm TEMPORARILY OFFLINE!

If you need to detect whether you have an offline result specifically (offline mode for an app) then you need to write logic for that state.

My logic is relying on window.navigator.onLine right now (which actually works quite good) and I show an indicator for that, but I still think we are in the situation you claim:

"it'll only be true when a newer result can be expected"

A personal note on this issue

I'm thinking there isn't enough details here to fully jump on this. @wgolledge worked on this but I don't think he'll be able to get an idea of when and how this occurs.

I think, I hope he can now.

can you please list what conditions are a factor

The example is just a basic one, see the urql initialization here: https://codesandbox.io/s/urql-svelte-crud-offline-fetching-stale-data-rj22w?file=/urql.js

We can't exclude the possibility that this isn't an issue with @urql/svelte

I think it is not.

But to confirm this feeling now I'll try to create an equal REPL using React (which I hate viscerally).

But in the meantime, PLEASE, (since it might take some time), could you already start to understand if I'm right and it's not the @urql/svelte package, as I think?

Thanks.

[wgolledge]

Hey @frederikhors, thanks a lot for taking the time to write up the bug report.

We definitely need to try and get to the bottom of why it's not retrieving the stale data from the cache. It'll take a bit more investigation before we can confirm for certain whether the problem lies in the urql/svelte package or elsewhere. Thanks for trying to do a REPL in React, hopefully it's not too time consuming 😅

We will try and investigate this more thoroughly soon.

[kitten]

Can we double check that the teardown is properly handled by this demo app? If I just think about the issue it does sound like it may get stuck if there's never a teardown. The retry times are also very long so it may take up to 10s for any events to show up. Shortening it will make debugging easier.

It's important to remember that if teardowns hypothetically don't work, no other operation is going to send requests to the cache while the retry is ongoing.

[frederikhors]

Can we double check that the teardown is properly handled by this demo app?

I don't know how to do this but I can tell you that if I reduce timing from this:

const retryOptions = {
  initialDelayMs: 1000,
  maxDelayMs: 10000, // this
  randomDelay: false,
  maxNumberAttempts: 30, // this
  retryIf: (err) => err && err.networkError
};

to this:

const retryOptions = {
  initialDelayMs: 1000,
  maxDelayMs: 2000, // this
  randomDelay: false,
  maxNumberAttempts: 3, // this
  retryIf: (err) => err && err.networkError
};

it works!

I mean I can see my (stale) cached list after all the retry waiting time.

Wow! You got it @kitten! As always! :smile:

So... now what? What is the correct way to do this?

I mean, I'm using high times because I wanna try and retry a lot because the app can remain offline for a time ranging from a few minutes to several hours.

_I could also manage this offline checking "externally" and then when back online I can reload the whole page (or urql queries only, but for now there are a lot of integration problems with @urql/svelte and I can't rely on methods that seem to work for React instead)._

But I still believe that if there is a specific exchange for these situations (retryExchange) it is a responsibility of it to manage them, am I wrong?

[kitten]

@frederikhors It's likely a better idea to have something do client.reexecuteOperation using storage.onOnline timings when they've previously failed. I was about to look into that and let it be built into Graphcache but it's risky if that's not desired for some scenarios... but it's worth checking.

So for now you can create an exchange similar to the errorExchange that detects offline errors in front of the retryExchange, stores them, and retries them on storage.onOnline, if they haven't been cancelled with teardown events.

The problem here is that retryExchange is meant to actually retry; so it will not let any data through that it deems to be an error, network errors included. It doesn't do so passively but actively, so that you don't have to handle these errors while the exchange is retrying the operation.

[frederikhors]

I can barely follow you.

I am a bug finder, the solution finder must be you, my dear!

:smile: :smile: :smile:

After a bit of humor, here I am!

This I cannot understand:

that detects offline errors in front of the retryExchange, stores them, and retries them on storage.onOnline, if they haven't been cancelled with teardown events.

And for this:

The problem here is that retryExchange is meant to actually retry; so it will not let any data through that it deems to be an error, network errors included. It doesn't do so passively but actively, so that you don't have to handle these errors while the exchange is retrying the operation.

I understand and love it.

What I'm asking to you is:

There is a quick way now (even using flags / options / variables) to tell to retryExchange:

"Oh my dear, I appreciate the effort you are making to avoid me the bitter morsel, but please - IN THE MEAN TIME YOU TRY AGAIN AND AGAIN so hard - can you let me see the data you already have cached? Please..."

And if it lets this data be displayed I can warn the user that that data MAY be stale because browser is offline or the server is not responding.

[kitten]

I think this ticket has strayed too far from what originally is a question. It's possible this could be revived as a discussion thread. But this is what we intended to be in user code: The retry exchange retries, it doesn't refresh. Not every behaviour can be encoded in exchanges either. If we want the request to be retried while we're offline then that's userland code to us.
In fact, there are browser events for us like in our example storage's onOnline callback.

I'm closing this for now, because I can't seem to see this having a coherent path forward that could have work dedicated towards.

Sorry, @frederikhors, but I also have to note, please tone down your responses. It's very exhausting to come to a conclusion on these threads. If the question is "is there a quick way to [get urql to do what I want it to do]" then our default answer will always be to write a custom exchange if it's inherently not a behaviour we cover.

[frederikhors]

I got it.

The need for a dedicated Exchange for this behavior seems excessive to me.

It would be great if we could have a "showCachedDataDuringRetry" option.

Stop.

:smile:

[kitten]

@frederikhors That's not a thing though, because the retry is completely separate from the cached data.
The retry only catches requests after the cache and retries them if they have a networkError, if you place this after the cache. It doesn't affect the cache response from the cache, if there is a possible one.