Remove react-query

[wizardlyhel]

Description

Fix #23

Additional context

Use branch remove-react-query-with-logs to see query behaviours between suspense and our Cache api in the server log.

You can run yarn run dev-worker to run dev project in a miniflare worker instance

---

To-Do

• Sync with main with the recent merged change to useQuery
• Make suspense promise caching the same as what is defined with Cache API
• Remove documentation around react-query
• Write documents around the new preloadQuery functions

Before submitting the PR, please make sure you do the following:

• Add your change under the Unreleased heading in the package's CHANGELOG.md
• Read the Contributing Guidelines
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
• Update docs in this repository for your change, if needed

[blittle]

If we really want to remove all server context, it seems to me that useShop(), with the global shopify context is going to be really obnoxious to remove. Drilling all of those props is nasty. Or maybe those properties are static between requests, so we could just useShop() could just rely on some in memory object instead of context?

[wizardlyhel]

Yea - at least with useShop, this is pretty much a static object that won't change over the course of instance of the server app

[blittle]

Really great work! Would it be worthwhile to add an e2e test that uses useShopQuery?

[jplhomer]

Thanks for diving into this!

I think we're mixing up concepts of long-term cache (Oxygen) and short-term cache (Suspense renders) here.

Since we already support the long-term cache within useQuery, we should only need to introduce a small key-based object that persists the cached value for the lifetime of the request.

Something simple like this: https://github.com/vercel/next-rsc-demo/blob/main/lib/use-data.js without any reference to maxAge, SuspensePromise, etc.

All useQuery is responsible for is passing its queryFn to that object lookup, throw a promise if it's not resolved, or return the result of the data if it is resolved.

I also LOVE the forward-thinking preloadQuery stuff, especially since it ties closely into react-fetch. However, I think we should include it in a follow-up PR instead of including it in this one. That way, we can release it with our recommended usage in the starter template.

[jplhomer]

In order to maintain backwards compatibility with this release, we should still return a nested {data} object from the useQuery response.

[wizardlyhel]

yeaaaa ...this is what confuses me too.

Does react-query returns an object like

{
  data: {
    <results of api>
  }
  errors: {
    <react-query compiled errors?>
  }
}

Cause right now, this result object is returning

{
  data: {
    <results of api>
  }
}

If I make an invalid query, the app just dies with proper error log. So .. what is this errors object?

[wizardlyhel]

oh wait - we do get an error object when query fails

So then react-query was returning an object like this

{
  data: {
    data: {
      <results of api>
    },
    errors: {
      <request errors> 
    }
  }
}

that we no longer need to unwrap

[jplhomer]

react-query returns {data, errors} by default. But when you enable suspense: true (which we do), it only returns {data} and throws any errors to be caught by ErrorBoundary.

So to maintain backwards compat, we should return this shape:

{
  data
}

// for SFAPI calls, looks like:

{
  data: {
    data: {},
    errors: {},
  }
}

[wizardlyhel]

Took me forever to understand your feedback - fixed

[wizardlyhel]

@jplhomer

We have to remove the promise from the short-term cache. Otherwise, the server render will alway returned the already fetched result and never refresh.

And if we remove the short-term cache too early, we run into the risk of making 2 identical promises in the same render

[jplhomer]

We have to remove the promise from the short-term cache. Otherwise, the server render will alway returned the already fetched result and never refresh.

The Suspense cache should have a lifetime of a single request, and its values should not persist between requests. We wouldn't ever need to refresh something within the window of a single request.

[wizardlyhel]

The Suspense cache should have a lifetime of a single request, and its values should not persist between requests. We wouldn't ever need to refresh something within the window of a single request.

But it isn't .. It's not clearing the short term cache. A normal const cache = {} will not clear itself for as long as the server in-memory object exist for this file.

I think what you are thinking is the unstable_getCacheForType that we are not using yet

[wizardlyhel]

👍 I will move the preloadQuery and preloadShopQuery to another PR. Still needs more thoughts how we want to structure this in the starter template and needs more exploration around if could we preload template level queries

[jplhomer]

A normal const cache = {} will not clear itself for as long as the server in-memory object exist for this file.

Ahh, OK. If it's persisting with the dev server, we should clear it out with each request:

// cache.ts

let cache = {};

export function resetSuspenseCache() {
  cache = {};
}

// useQuery/hooks.ts

export function useQuery(key, queryFn, options) {
  // ...

  const data = useData(key, cachedQueryFn);

  return {data};
}

// entry-server.tsx

function stream() {
  resetSuspenseCache();

  // ...
}
// same thing for these:
function render() {}
function hydrate() {}

This way, we don't need to worry about maxAge or eviction or anything, and the cache is a simple JS object until we can rip it out and replace it with getCacheForType which ties it to the React lifecycle.

[wizardlyhel]

Aligned with @jplhomer and @blittle

The short-term cache is an in-memory object that needs to be manually evicted otherwise we run into concurrency issue. We cannot evict per request because JS modules isn't refreshed per request. Therefore, we may be evicting cache for another rendering request.

For now, we will evict this short term cache the moment the data is fulfilled.

Next exploration is React's built-in Suspense cache which said to be rendering process bounded. This feature is behind the --conditions react-server flag which makes it difficult to work with Vite.

I didn't change the SuspensePromise class into that simpler version as linked because I need to separate the act of throwing a promise away from the promise construction to be stored in the cache. This way, I can share the same short term cache with useQuery and preloadQuery. Preloads should not be throwing a promise.

[wizardlyhel]

Really great work! Would it be worthwhile to add an e2e test that uses useShopQuery?

Good question - I don't see any test written for useShopQuery. I guess this is because it needs a ShopifyProvider. I think if we can allow jest test to access env variables to put into ShopifyProvider, we can add test for useShopQuery

[jplhomer]

react-query returns {data, errors} by default. But when you enable suspense: true (which we do), it only returns {data} and throws any errors to be caught by ErrorBoundary.

So to maintain backwards compat, we should return this shape:

{
  data
}

// for SFAPI calls, looks like:

{
  data: {
    data: {},
    errors: {},
  }
}

[jplhomer]

Just double-checking here that we likely need to pass the cache between the two render passes since we're deleting the entry from the cache immediately after it resolves.

The second render pass needs to have no thrown promises, otherwise the data never resolves.

This only happens when:

1. we are in a workers runtime
2. and it doesn't support ReadableStream

So to confirm this, you can use miniflare (e.g. in the server-components-workers playground), short-circuit isReadableStreamSupported to always return false (because miniflare incorrectly supports it, although cloudflare doesn't), and then try to render a page with Suspense queries.

[wizardlyhel]

You are right. This doesn't work with the ssr prepass. If we don't delete the short term cache, it works perfectly.

The only way I can think of to get this working is if we have a request specific unique ID that I can access at useQuery. This id would be perfect for Server Context .. but we don't have one yet, so the next best thing is push this id as props and pass it back with each usage of useQuery or useShopQuery ... which I really don't want to do.

You got ideas @jplhomer ?

[jplhomer]

ha yeah. So your request ID + prop drilling thing works as a possibility... agreed that it would not be fun.

Prior to react-query, we used context to accomplish this. Then, react-query used context to accomplish it.

Could we just use context in the meantime? I know we're trying to move away from this, but until we have some time until either server context OR real RSC can be used.

Context could also be used as a caching mechanism tied to the react render lifecycle, right? 🤔

Additionally, we'll be removing the prepass render method as soon as our primary deployment target supports it (Oxygen & CF both coming soon).

[wizardlyhel]

Added as RequestServerProvider

[blittle]

Do we want to somehow add a timeout in how long we want to wait for this promise to resolve? And if it doesn't resolve within that timeout, throw an error.

[wizardlyhel]

You wouldn't do it here I think - you would make that as part of your promiseFn like

useQuery("key", () => {
  setTimeout(() => {
    throw Error('Took too long');
    // or return an object so that UI can manage error display
    return {
      error: 'Took too long'
    }
  }, 3000);
  return fetch(...)
})

No idea if the above would work as I think it would - prob need to play around with it

[blittle]

Why wouldn't we want to build in some sensible timeout defaults? Right now, none of our requests have a timeout, so if they take a long time, the whole app breaks in RSC-land. That example code won't work because the throw is in the timeout. setTimeout is fired on a new round of the event loop, so it won't get caught by react's suspense boundary.

Sounds like maybe I should address this in a separate PR with error handling. Related, @frehner mentioned this, which might be useful: whatwg/dom#951

[wizardlyhel]

The timing out issue isn't a useQuery problem. It's when the stream connection closes before the api can complete.

For example, if we were to wrap an API call that is taking a long time in a <Suspense> wrapper, it will never get replaced by stream.

And like you said, if such API call isn't wrapped in a <Suspense>, we get a blank page with zero warning.
So, what we need is actually an error detection of - when a stream connection closes, check if there are any unresolved/unfinished RSC work. If there is, raise error.

[jplhomer]

Since we're introducing context already, could we eliminate the need to create or track a requestId altogether?

RequestServerProvider would essentially be:

export function RequestServerProvider({cache, children}: {cache: Record<string, any>}) {
  return <RequestContext value={{ cache }}>{children}</RequestContext>
}

It would be injected like:

// entry-server.tsx

let suspenseCache = {};

<RequestServerProvider cache={suspenseCache}>
  <ReactApp />
</RequestServerProvider>

Within useData:

function useData(key, fetcher) {
  let {cache} = useRequest();

  if (!cache[key]) {
    let data
    let promise
    cache[key] = () => {
      if (data !== undefined) return data
      if (!promise) promise = fetcher().then((r) => (data = r))
      throw promise
    }
  }
  return cache[key]()
}

And within useQuery:

export function useQuery(key, queryFn, options) {
  // ... cachedQueryFn is built here

  const data = useData(key, cachedQueryFn);

  return {data};
}

Benefits of this:

• Simple object: no need for holding the entire request in a Map
• No need to generate requestId just to use for Suspense cache
• Tied to the React render lifecycle, not held in global memory
• Can be used for pre-pass
• No need to clear request cache, since it has a lifetime of React's render lifecycle

Sorry to keep kicking this can down the road — I just want us to find the simplest solution for the time being!

[jplhomer]

Demo here: https://github.com/Shopify/hydrogen/compare/jl-suspense-cache-demo

[wizardlyhel]

Oh~ I like this ... but we need a better name for this RequestServerProvider.

RenderCacheProvider?

[wizardlyhel]

@jplhomer Updated with requested changes

Checked it works in worker mode when ReadableStream is not supported (Yellow colour output is from the Cache API)

[jplhomer]

🚀 Thank you for all your hard work on this!

[jplhomer]

just curious - was this breaking something?

[wizardlyhel]

Breaking document generation

[jplhomer]

ooohhh 👍