Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using [email protected] or 6 the cache in the default storage is not loaded anymore on the first page load #3093

Closed
3 tasks done
frederikhors opened this issue Mar 23, 2023 · 24 comments · Fixed by #3196 or #3200
Closed
3 tasks done

Comments

@frederikhors
Copy link
Contributor

frederikhors commented Mar 23, 2023

After upgrading from 4 to 5 (or 6) @urql/exchange-graphcache doing some tests with a fake delay on the backend I discovered that when I refresh the page (F5 or Ctrl+R) the cache present in the IndexedDB is not used.

The REPL (with SvelteKit too) is here: https://codesandbox.io/p/sandbox/romantic-feather-xmc826

Steps:

  1. Go to https://xmc826-5173.csb.app/todos

  2. the todos are downloaded, showed and stored in the IndexedDB after 3 seconds of fake delay on the server resolver

  3. Click on the reload button in the navbar (or the browser's one)

  4. The "Loading listStore..." message appears during the 3 fake delay seconds

  5. I expect instead to see the todos immediately (during the cache-and-network call which is still happening)

If I rollback only "@urql/exchange-graphcache" to "4.4.3" this "cold cache" behavior works again.

What could be causing the problem?

Thank you in advance.

Urql version

package.json

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@kitten
Copy link
Member

kitten commented Mar 23, 2023

I can't find the original issue for this now, but basically, this has to do with a change that affected SSR + Offline support.

With the prior implementation, the offline storage rehydration would block all operations and hence, would block any cache or the SSR Exchange responses from being used, breaking pages initial states.

We also realised that blocking operations and queuing them to wait for the storage to hydrate isn't really idiomatic.

The new behaviour will hydrate the storage asynchronously and its data will be available as soon as IndexedDB responds with data, typically after the initial load.

This means that during this time, Graphcache will behave as if it wasn't in Offline mode and will fail if the network is unavailable and retry against the rehydrated storage as soon as that happens.

Basically, looking at cache-and-network here; this has the same effect. We essentially stop offline data from being used until it's available and just defer to the next exchange in the chain.

@frederikhors
Copy link
Contributor Author

Thanks for the answer.

I disabled SSR entirely in my web app.

And since my app is a PWA with web-worker and the web app is often loaded where there is no internet connection (or there is an intermittent one) that behavior was amazing!

Now with the 5.0.0 I can't load the data at all if the server doesn't answer me first and this is absurd because the data is already in the browser!

How can I fix it and go back to the old v4 - GORGEOUS - working?

@frederikhors
Copy link
Contributor Author

This means that during this time, Graphcache will behave as if it wasn't in Offline mode and will fail if the network is unavailable and retry against the rehydrated storage as soon as that happens.

Basically, looking at cache-and-network here; this has the same effect. We essentially stop offline data from being used until it's available and just defer to the next exchange in the chain.

I think this is not working...

@frederikhors
Copy link
Contributor Author

Another very bad case is when the server is late in giving the first response: for example it takes 5 seconds.

For 5 seconds I see the loading spinner instead of my old data which is in the IndexedDB and which instead I could already see.

@frederikhors
Copy link
Contributor Author

If I try to use [email protected] with @urql/[email protected] it throws on mutation with:

offlineExchange.ts:194 Uncaught (in promise) TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)
    at offlineExchange.ts:194:4
    at wonka.mjs:200:17
    at Array.<anonymous> (operators.ts:285:9)
    at operators.ts:768:70
    at next (sources.ts:230:21)
    at next (sources.ts:279:17)
    at urql-core.mjs:585:28
    at operators.ts:611:9
    at operators.ts:317:9

This does not happen with @urql/[email protected].

@frederikhors
Copy link
Contributor Author

Me watching Kitten solve other people's issues at lightning speed.

image

@kitten
Copy link
Member

kitten commented Apr 20, 2023

I mean, I can't do magic here and haven't seen enough yet to even consider this a bug, but rather, it's another feature requests. It's also one that's likely out of reach for what we could reasonably do.

As explained, while the storage is rehydrating an initial request is attempted. If you have SSR set up then those results will obviously be used and take priority. If the network is available then that takes priority and will be used.

So, what instead should be happening here is that a request is attempted and otherwise the operation is called against Graphcache's rehydrated storage — at least that's as far as I know and recall

@frederikhors
Copy link
Contributor Author

I think the problems are different here. I mean I think I'm referring to a solvable problem. In the next few hours I will create a REPL to have a common base.

@frederikhors
Copy link
Contributor Author

frederikhors commented Apr 25, 2023

I updated the first post with full REPL of the issue.

@frederikhors frederikhors changed the title Using [email protected] & svelte the cache in the default storage is not loaded on the first page load Using [email protected] or 6 the cache in the default storage is not loaded on the first page load Apr 25, 2023
@frederikhors frederikhors changed the title Using [email protected] or 6 the cache in the default storage is not loaded on the first page load Using [email protected] or 6 the cache in the default storage is not loaded anymore on the first page load Apr 25, 2023
@strewhella
Copy link

I'm seeing the same issue after upgrading to @urql/[email protected]. In a React Native app, when offline and using the cache-and-network request policy, the offlineExchange doesn't return the cached data at all, data is always null, though stale is true.

Reverting to @urql/[email protected] resolves the problem, and cached data is returned as expected.

@frederikhors
Copy link
Contributor Author

@strewhella thank you very much for your comment!

This is the comparison between 5.0.8 and 5.0.9: @urql/[email protected]...@urql/[email protected].

I think this PR is responsible for the problem: #2945.

@trcoffman can I ask you if you can detect the issue in the code and help us solve this problem?

@trcoffman
Copy link
Contributor

trcoffman commented Apr 28, 2023

I don't mind taking a look at this and trying to figure this stuff out, but I just wanted to say that I don't like the way you went about passive aggressively calling out @kitten. Have you actually spent any time actually stepping into urql code in a debugger and trying to solve this issue and come up with a PR?

I don't know what's going on here, we haven't updated @urql/exchange-graphcache in a while.

Our react-native app is using a patched @urql/[email protected]. The patch was the basis of the PR #2945 that I submitted. I never circled back and updated the dependency so that I could remove the patch when the PR got merged upstream. graphcache is working flawlessly for us so that our queries resolve as expected whether we are offline or online.

This is the patch that we are using.

diff --git a/node_modules/@urql/exchange-graphcache/dist/urql-exchange-graphcache.js b/node_modules/@urql/exchange-graphcache/dist/urql-exchange-graphcache.js
index 7b5d489..9368d1d 100644
--- a/node_modules/@urql/exchange-graphcache/dist/urql-exchange-graphcache.js
+++ b/node_modules/@urql/exchange-graphcache/dist/urql-exchange-graphcache.js
@@ -2349,7 +2349,6 @@ function offlineExchange(opts) {
         })(outerForward(ops$));
       };
 
-      storage.onOnline(flushQueue);
       storage.readMetadata().then(function (mutations) {
         if (mutations) {
           for (var i = 0; i < mutations.length; i++) {
@@ -2358,7 +2357,7 @@ function offlineExchange(opts) {
 
           flushQueue();
         }
-      });
+      }).then(() => storage.onOnline(flushQueue));
       var cacheResults$ = cacheExchange(opts)({
         client: client,
         dispatchDebug: dispatchDebug,

Only thing that stands out to me was that I revised the .then to be a .finally as I wanted to make sure that onOnline would still be called if there was an error in rehydrating the storage in the PR but it's not like that in my patch.

@trcoffman
Copy link
Contributor

@strewhella thank you very much for your comment!

This is the comparison between 5.0.8 and 5.0.9: @urql/[email protected]...@urql/[email protected].

I think this PR is responsible for the problem: #2945.

@trcoffman can I ask you if you can detect the issue in the code and help us solve this problem?

I just forked your codesandbox and tried out 5.0.8, and this problem is still present. So i don't think that my PR #2945 has anything to do with this. If you go back to 4.4.3 it works just the way you want it to.

@kitten
Copy link
Member

kitten commented Apr 28, 2023

I haven't really looked at this yet since I reckon a resolution will require a larger refactor and changes.

The reason why I believe(d) this is that the description of the issue implies that this is related to a different change where we ensured that the ssrExchange still works together with the offlineExchange.

This change however was made in 5.0.0: #2612

Also #2945 relates to the replays after hydration , so I don't see that exactly as responsible, and at most coincidentally changing behaviour.

So, that's conflicting information to me right now, since I originally assumed that the issue is rather that, while offline or online, the offlineExchange currently does not replay queriy operations after rehydrating the cache from storage.

Then again, I'd assume that some of this relates to this check and failedQueue push:

failedQueue.push(res.operation);

There we actually do queue failed queries. However, this could've only coincidentally made this work in the past, since it'd only work if onOnline is calling its callback after it succeeded.

Therefore the only logical conclusion is that flushQueue is called but either failedQueue isn't properly populated with queries, or it doesn't attempt to retry them. Another possibility is that the result eventually does come in properly but the initial fallback to cache-only is confusing.

Anyway, that's my assumption from before and right now with the updated messages. I can see whether I'll find time for this in the next 48h-ish or so. That said, the offlineExchange is only ~180 very "thin" lines of code without comments, so I can only encourage y'all to look into it and throw in some debugging of your own if you get to it sooner than myself 😇

@frederikhors
Copy link
Contributor Author

frederikhors commented Apr 28, 2023

I just wanted to say that I don't like the way you went about passive aggressively calling out @kitten.

Oh wow. I'm sorry I don't understand what you mean with this. I'm not even english native and it's difficult for me to understand the real meaning or your nuances of passive aggressively calling out.

If I have offended someone: IT WAS NOT MY INTENTION! SORRY!

Are you referring to the animated gif? It was a joke, really, I think it's clear: since I have the subscription on the issues it was a joke (and an implied compliment) about how fast he is at solving people issues.

I apologize again if it seemed like I wanted to treat someone badly (let alone treat a genius like @kitten badly, whom I've been thanking for this project for years now!).

@kitten, sorry either way and thanks for your help fixing this.

Have you actually spent any time actually stepping into urql code in a debugger and trying to solve this issue and come up with a PR?

I have no idea where to start or how to do it. I'm not a PRO, otherwise I would have already created the PR. Lucky you that you can do it (I'm not ironic: congratulations, really).

Anyway @trcoffman sorry for asking you about a problem you apparently didn't cause. And thank you also because you taught me never to take funny jokes for granted.

@kitten
Copy link
Member

kitten commented Apr 28, 2023

This should address the issue: #3196
However, the reproduction didn't seem to validate anything but that operations are simply waiting for an API request, and I don't have a full understanding of @strewhella’s issue either. So, if you could both validate this, that'd be really helpful.

The Checks CI task on the PR contains a prebuilt tarball, and I've forked the sandbox and copied the fixed bundle in there manually: https://codesandbox.io/p/sandbox/issue-urql-5-no-data-load-at-startup-forked-gj2hqw

That said, I'm not 100% convinced this will resolve all issues, but it's likely to restore legacy behaviour

@trcoffman
Copy link
Contributor

@frederikhors, no worries, I think I just have become very (perhaps overly) sensitive due to the way a lot of open source maintainers are treated by the community. I've seen a lot of maintainers get burned out and just have to quit working on their open source projects because people feel entitled to make endless demands.

Your codesandbox REPL is very helpful and clearly shows that for your use-case (and also the use-case of our team), @urql/exchange-graphcache 5.0.0+ is not as useful as it used to be.

@kitten, I think what's going on is that flushQueue as it exists in v5 is not a good replacement for the original buffering of operations that was taking place while storage was being rehydrated.

My understanding of what's happening in @frederikhors REPL is that the graphQL query to fetch the TODO list is being forwarded out of the exchange before the storage has been rehydrated. The operation is never installed into the failedQueue because the user is online, so the request never actually fails. When reading the data from storage is complete, the call to flushQueue doesn't help us because the failedQueue is empty.

I'll take a look at #3196 and see if I think it addresses the issue.

@frederikhors
Copy link
Contributor Author

Ok I updated from:

"@urql/core": "2.6.1",
"@urql/devtools": "2.0.3",
"@urql/exchange-graphcache": "4.4.3",
"@urql/exchange-persisted-fetch": "1.3.4",
"@urql/exchange-retry": "0.3.3",
"@urql/svelte": "2.0.2",

to

"@urql/core": "4.0.7",
"@urql/devtools": "2.0.3",
"@urql/exchange-graphcache": "6.0.4",
"@urql/exchange-persisted-fetch": "2.1.0",
"@urql/exchange-retry": "1.1.1",
"@urql/svelte": "4.0.1",

I tried urql-exchange-graphcache-v6.0.4 from https://github.com/urql-graphql/urql/actions/runs/4835712980.

It fixes my issue for the reload.

I'm now trying to fix the news (like the deduplicate ones because now all the queries are not deduplicate anymore, but I know I need to change something, no problem).

Again, like for years now: THANKS!

@frederikhors
Copy link
Contributor Author

I think we should re-open this due to #3199.

@kitten
Copy link
Member

kitten commented Apr 29, 2023

@frederikhors: I haven't had time to test this yet, but just going from gut feeling #3200 is probably going to resolve this

@frederikhors
Copy link
Contributor Author

frederikhors commented Apr 29, 2023

#3200 doesn't fix the duplicated requests unfortunately.

https://codesandbox.io/p/sandbox/issue-urql-6-duplicated-requests-x2y2ub

@strewhella
Copy link

Thanks for taking a look at this so quickly @kitten and @trcoffman, really appreciate it. I'll have a look and see whether 6.0.4 resolves the problem I was seeing. Apologies for not providing more information in my initial comment

@frederikhors
Copy link
Contributor Author

frederikhors commented Apr 30, 2023

6.0.4 doesn't fix. @kitten is working on a solution: #3093 (comment).

From #3199:

Describe the bug

I have an issue with duplicated requests (I remove the now deprecated dedupExchange: I read all CHANGELOGs and announcement issues like #3114).

If I use this code:

import { getContextClient, queryStore } from '@urql/svelte';

const currentPlayerStore = queryStore({
  client: getContextClient(),
  query: CurrentPlayerDocument,
  // the default requestPolicy is 'cache-first' // one request only, ok
  // requestPolicy: 'cache-and-network', // two requests, duplicated
  // requestPolicy: 'network-only' // THREE requests! Duplicated!
});

$: currentPlayer = $currentPlayerStore.data?.currentPlayer;

As you can read in the comments I isolated the issue: if I use the 'cache-first' policy it only send one request.

If I use the requestPolicy: 'cache-and-network' the requests are 2, duplicated, the same ones.

If I use the requestPolicy: 'network-only' the requests are 3 now! Crazy!

What am I doing wrong?

Reproduction

This repl is using your modified graphcache version that fixes #3093.

REPL: https://codesandbox.io/p/sandbox/issue-urql-6-duplicated-requests-x2y2ub

Steps:

  1. click on "Todos list"

  2. open the browser dev tools

  3. reload the page

  4. the requests are 2!

  5. If you use network-only in the list file the requests are 3 now

Urql version

I upgraded:

"devDependencies": {        
  "@graphql-codegen/cli": "3.3.1",
  "@graphql-codegen/typed-document-node": "4.0.1",
  "@graphql-codegen/typescript": "3.0.4", 
- "@urql/core": "2.6.1",  
+ "@urql/core": "4.0.7",  
  "@urql/devtools": "2.0.3",
- "@urql/exchange-graphcache": "4.4.3", 
+ "@urql/exchange-graphcache": "6.0.4", 
- "@urql/exchange-persisted-fetch": "1.3.4",  
+ "@urql/exchange-persisted": "4.0.0",
- "@urql/exchange-retry": "0.3.3",    
+ "@urql/exchange-retry": "1.1.1",    
- "@urql/svelte": "2.0.2",    
+ "@urql/svelte": "4.0.1",
- "graphql": "16.6.0",  
- "graphql-web-lite": "16.1.1000",
},

@frederikhors
Copy link
Contributor Author

The amazing @kitten is working on this here.

With the latest commit the things are working now, except this:

REPRODUCTION: https://codesandbox.io/p/sandbox/issue-urql-6-duplicated-requests-forked-0ybcue.

Steps:

  1. click on "Todos list"

  2. open the browser dev tools

  3. reload the page

  4. see in the console call addPendingRequest and immediately (not after the fake delay of the server) call deletePendingRequest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants