urql icon indicating copy to clipboard operation
urql copied to clipboard

RFC: Graphcache's cacheExchange should forward operations while hydration is ongoing

Open oreqizer opened this issue 4 years ago • 11 comments

Mod Edit by @kitten: For the proposed RFC, up for discussion, please read the comment below with the RFC text for solving this reported issue.

Initial render is missing SSR data when adding storage to offlineExchange, renders loading instead

urql version & exchanges:

  • "urql": "^2.0.1",
  • "@urql/devtools": "^2.0.3",
  • "@urql/exchange-graphcache": "^4.0.0",
  • "@urql/exchange-multipart-fetch": "^0.1.11",

Steps to reproduce

  1. create an offlineExchange and ssrExchange
  2. run application without specifying storage, all is OK
  3. add storage (default one), SSR breaks, initial data is missing

Expected behavior

Initial render should keep SSR data

Actual behavior

SSR data are ignored and loading is flashed instead

Example

This is an actual example from my application, dumbed down from ballast (like resolvers, optimistic mutations...)

import type { Exchange } from "urql";
import { Client, dedupExchange, ssrExchange } from "urql";
import { offlineExchange } from "@urql/exchange-graphcache";
import type { IntrospectionData } from "@urql/exchange-graphcache/dist/types/ast";
import { multipartFetchExchange } from "@urql/exchange-multipart-fetch";
import { devtoolsExchange } from "@urql/devtools";

import schema from "schema.min.json";

const cache = offlineExchange({
  schema: schema as IntrospectionData,
  // === HERE ===
  // uncommenting this breaks SSR:
  //
  // storage: makeDefaultStorage({
  //   idbName: "gainwagon",
  //   maxAge: 7, // The maximum age of the persisted data in days
  // }),
});

export const makeClient = () =>
  new Client({
    url: String(process.env.API_URL),
    exchanges: [
      __DEV__ && devtoolsExchange,
      dedupExchange,
      cache,
      // Put the exchange returned by calling ssrExchange after your cacheExchange,
      // but before any asynchronous exchanges like the fetchExchange:
      ssrExchange({
        isClient: true,
        initialState: window.__URQL__,
      }),
      multipartFetchExchange,
    ].filter(Boolean) as Exchange[],
  });

oreqizer avatar Apr 04 '21 07:04 oreqizer

This is currently expected behaviour. The problem is that SSR and Offline storages aren't easily reconcilable. The reason for that being that persistence for Graphcache must typically be asynchronous while the usual caching processes are synchronous (including SSR, Graphcache, and the default cache)

This means that since the storage read is asynchronous there's not really much of an alternative to waiting for it to complete

kitten avatar Apr 04 '21 11:04 kitten

is there a plan to support this behavior? maybe add something like initialData as options? or a way to make this work somehow, like "swap" the initial SSR-compatible client for an offline one after the initial mount?

oreqizer avatar Apr 04 '21 11:04 oreqizer

@kitten isn't it related? https://github.com/FormidableLabs/urql/discussions/1510

RIP21 avatar Apr 04 '21 13:04 RIP21

@oreqizer its unfortunately not that simple. The SSR logic is decoupled and there's a possible mismatch between what your storage has and what the SSR queries deliver. For all of this to be accurate the SSR results need to be fed back into Graphcache (and hence the storage) and for anything like that to happen the storage's data first needs to be read out into memory.

@RIP21 Not as far as I can tell? Your issue was describing an infinite loop, right? Regardless of SSR? It may be related to duplicate queries but that being said, it's hard to tell while we haven't tried to reproduce it yet

kitten avatar Apr 04 '21 13:04 kitten

@kitten sort of infinite loading while using offlineExchange but only for a few first queries, SPA usage. Absolutely the same setup, but with cacheExchange, works without any problems.

RIP21 avatar Apr 04 '21 13:04 RIP21

There's an important distinction between expected behaviour and unexpected behaviour here though.

A loading loop would be unexpected. However, a delay is expected on startup with the offlineExchange because it has to load data in the storage asynchronously.

Edit: Also to clarify, if we do work around this other issues pop up re. this specific issue. Specifically, SSR + Offline is an odd choice. If we add a service worker that allows a page to be stored then we're clearly dealing with an SPA. But if we do make the mistake of storing entire HTML results on a service worker then we'd be dealing with stale SSR results, so without or with the delay we'd be dealing with old data being fed into the cache

kitten avatar Apr 04 '21 13:04 kitten

@kitten let's go to the discussion about my particular case (my case is SPA) :) As I'm unsure how I can achieve a "delay" and why it's never mentioned in the docs :P As, well, usually you have providers rendering in the very root and I'm unsure how it can be delayed. Unless there is some Suspense trick in the sleeve.

RIP21 avatar Apr 04 '21 13:04 RIP21

I've been thinking about this a little and there is in fact a way to solve this. Hence, I'm writing up a small RFC here to detail on how this could work.

Edit: I also wanted to clarify why we don't have initialData or anything of the likes. This violates our principles of extensibility since it doesn't concern the Client what data should be initialised. But ignoring that it's also isolated to the ssrExchange because SSR-data is just a different form of cached data. Given how persistence hydration is asynchronous it wouldn't even help us if Graphcache could take over for the ssrExchange. It would just mix the concerns but not solve the problem that it needs to reconcile async and sync state.

Summary

When storage is used in Graphcache's cacheExchange it will trigger the storage's hydration and queue up all operations while this is ongoing. This essentially ignores potential ssrExchange data (or other secondary caches) that may be placed after the cacheExchange or offlineExchange. This means that the initial mount may not synchronously have all the data it expects in the case of React's SSR hydration.

Proposed Solution

The cacheExchange in @urql/exchange-graphcache should continue to forward all operations to the exchanges after in its forward() stream and should process results without writing to storage, i.e. either blocking the GC & persistence runs or making the call a noop during this time. This forwarding enables the ssrExchange (or other secondary caches) to respond synchronously and hence secondary cache data is unblocked. It also enables the API to respond if it's quicker than the storage hydration.

Before hydration completes when a result comes in for one of the forwarded (previously "buffered operations") then this operation should be removed from the buffer, should update the cache, and should be returned, essentially as if hydration isn't ongoing.

Once hydration completes we'll only find operations in the buffer / queue that haven't received a result yet, for which we'll send teardown operations to forward() and not via the entire Client / client.reexecuteOperation. This will cancel out potential fetchExchange API requests. The buffered operations should then be processed as they are today, as the cache's data has been enriched with the storage data.

Requirements

  • The bufferedOperations should still be forwarded while hydration is ongoing
  • Any result that is received while hydration is ongoing should remove operations from this buffer
  • The received results should be processed as usual (Careful: The data must be layered on top of whatever will be hydrated)
  • Once hydration completes for each remaining operation in the buffer a teardown should be forwarded
  • These teardown operations shouldn't be visible to preceding exchanges in the chain or the Client
  • For each remaining operation the usual buffered operations logic (same as today) should then be applied

Potential Challenge: This is quite complex, but could end up looking rather simple in the implementation. However, what worries me is that all results must be layered on top of existing storage data.

Potential Issue: Furthermore, if any resolvers are used for these results then the issue here is that they may not be able to read from the cache. This means that we may have to reexecute operations as well after hydration completes, which is complex!

kitten avatar May 02 '21 15:05 kitten

Does this relate to react 18 and data fetching on the server too?

As it seems to me data is supposed to be sent along with the components within a suspense boundary during rendering using pipeToNodeWriteable. Is it somehow possible to send the SSRcache incrementally per suspense boundary?

I looked at the example posted on the react working group that says data fetching is not yet implemented, I am guessing they mean in react and not just the example.

So I wonder if the react team will add a way for hooking into the suspense with data loading without explicitly rendering a component or doing a custom suspense component. The magic seems to be happening here: https://github.com/facebook/react/blob/1314299c7f70914d61d8e1cef56767f112110674/packages/react-server/src/ReactFizzServer.js#L412 .

https://github.com/reactwg/react-18/discussions/47#discussioncomment-847004 Seems to be the answer

fivethreeo avatar Sep 06 '21 00:09 fivethreeo

Does this issue require offline storage? Or is it more general than that? I'm seeing cache exchange misses and (extraneous) requests happening even though the data is in ssr initialData around hydration time -- just want to rule this out before diving deeper into it.

tjk avatar Oct 11 '21 23:10 tjk

It should not relate to that, the issue you're describing might be due to cache and network, partial data or operation-key missmatches

JoviDeCroock avatar Oct 12 '21 03:10 JoviDeCroock