apollo-client icon indicating copy to clipboard operation
apollo-client copied to clipboard

Re-queries on hydration after ssr using renderToPipeableStream

Open iamafansev opened this issue 2 years ago • 5 comments

Issue Description

version: 3.8.* To begin with, it is better to consider the problem using an example: Example with useSuspenseQuery & renderToPipeableStream https://github.com/iamafansev/test-suspense-query

In this example, the fetchPolicy is set to cache-and-network. We expect that there will be no duplicate requests during hydration since we have a valid cache.

However, we see repeated outgoing requests in the developer tools.

At the same time, if you set the caching policy to cache-first, then repeated requests are not executed. This confirms that the cache contains correct data and the request need not be executed.

When I encountered this problem, I thought that the problem was only when using new features such as useSuspenseQuery, but I have another example: Example with useQuery / getDataFromTree & renderToPipeableStream (workaround to use React.(Suspense / lazy)) https://github.com/iamafansev/use-query-with-lazy-ssr

There is some workaround here to use pause rendering just to wait for lazy components. In the server-side implementation, we first traverse the tree using getDataFromTree and only after that, we pass the tree to renderToPipeableStream.

Similar to the first example, a repeated request is performed here with the cache-and-network caching policy and is not performed with cache-first. At the same time, if you do not use renderToPipeableStream for the example with useQuery, then repeated requests will not be executed under the cache-and-network caching policy. The problem also appears in version 3.7.17

Link to Reproduction

https://github.com/iamafansev/test-suspense-query

Reproduction Steps

  • yarn install
  • yarn start
  • go to http://localhost:4000/
  • devtools
  • network tab
  • We make sure there are repeat requests

iamafansev avatar Oct 28 '23 08:10 iamafansev

Hi there, lots of things to unpack here!

Unfortunately, you've fallen into a big pitfall the React team hasn't really documented well: there is currently no data fetching story for renderToPipeableStream in React.
See twitter discussion here (I'm really sorry that I have to give twitter as a reference here - I'd love to link you some docs, but this just hasn't been documented at all 🤦).

You can do the workaround you are currently doing - running getDataFromTree, gather all the data you need and then run renderToPipeableStream - but unless I miss some nuance here, there isn't really anything you gain from it: you essentially fully block the render, and then start a stream that could at that point in time just as well be a synchronous render (after all, you did all the async work before!).
It's also not really something we support, since - see above - it doesn't seem like it really makes sense to use renderToPipeableStream under these circumstances.

I have recently talked with Josh Story about this, and there is a good chance that React 19 might include a few helpers that would be necessary for us to get this working - but as of right now, there is just no React API for us to inject into the stream, so we cannot support this.
We do work around that and do have support for renderToPipeableStream in our Next.js helper library, but that relies on Next.js internals that kinda make up for the lack of React features here.

TLDR: as of right now, we cannot support renderToPipeableStream as React is lacking the primitives for us (or any other data fetching library) to do so. I'm really sorry.

phryneas avatar Nov 02 '23 11:11 phryneas

I was stuck in the same problem for several days.

I hope the Apollo team can provide examples of suggested SSRs now that as with libraries in general, they have an examples directory.

@phryneas Do you have any suggestions what is the best current method for building SSR with Apollo? is it still renderToStaticMarkup like before?

kcoet avatar Nov 21 '23 14:11 kcoet

@kcoet as I wrote above, renderToPipeableStream is literally impossible to use with any form of server-side data fetching (not just Apollo Client, also libraries like React Query or RTK Query - as I said above, React 18 doesn't contain the necessary primitives), you'd just do SSR as you did before it existed. No change in recommendations.

It sucks, but we cannot change it. I have a working prototype of hooks that I added myself to the React core to make this work, but of course we won't ask you to patch your react package.

As it stands, just stay clear of renderToPipeableStream. I'm sorry :/

phryneas avatar Nov 21 '23 14:11 phryneas

Small update on this - this is still difficult to set up, but not impossible. I'm currently in the process of writing docs on this, you can follow the PR in #11807 - I'd be happy for any feedback :)

phryneas avatar May 15 '24 09:05 phryneas

For anyone interested, I hacked around this by only sending the renderToPipeableStream response on onAllReady.

So basically... by not streaming, but works for my use case since I'm just migrating from React 17 + Loadable Components and I needed Reac.lazy SSR support, which only renderToPipeableStream supports... and as a side effect the cache for suspended queries is fully populated on hydration, so there are no additional queries for me.

Sharing this in case someone is in a migration like me and got scared by this thread... there's hope!

Can't wait to have proper streaming implemented.

alvaro-cuesta avatar May 29 '24 17:05 alvaro-cuesta

I'd love to see an update on this.

The documentation is very misleading, as React says very explicitly that renderToString does not support Suspense, so it feels hacky that we are allowing suspense boundaries while not using the pipe-able API.

I think it would be very helpful to have this ironed out in the docs.

Thanks for your hard work

moishinetzer avatar Nov 15 '24 14:11 moishinetzer

React says very explicitly that renderToString does not support Suspense

Neither does renderToReadableStream, at least for data fetching during SSR with only React primitives - you still need tons of workarounds.

@moishinetzer have you taken a look at this page from the PR I linked to above?

The whole story seems to move too quickly right now to write "final" documentation on it, but what I wrote there should help you set up something with the necessary workarounds until React itself finally gets to a state where this is supported.

phryneas avatar Nov 15 '24 14:11 phryneas

Thanks for the swift response!

I'll give it a shot and update here on the thread.

My context is that we have a really large production react app and we're trying to migrate from Webpack with loadable (to split up our routes), to Vite with lazy loading.

However, lazy loading (as the user above mentioned) is only supported with the streaming APIs

moishinetzer avatar Nov 15 '24 15:11 moishinetzer

@moishinetzer then you probably have to go with the setup in the documentation that I linked to.

That said, I remember setting up streaming SSR with Vite to be quite the beast. You might want to look into something like Vike - they also have an Apollo Client extension based on our streaming package.

phryneas avatar Nov 18 '24 09:11 phryneas

The truth is that we don't really care for the useSuspense queries or even streaming capabilities. Our issue is very specifically to do with lazy loading huge components. Do you know if there is a way to block the render until the apollo-client has completed all the queries? (Together with renderToPipeableStream that is.)

In fact, we actually like the UX of making the user wait until the page has fully loaded, without any spinners rendering etc. even if it's a faster TTFB.

I can use the pipe to push to a string however React doesn't wait for apollo to finish querying on the server side.


function renderToStringWithLazy(reactNode: React.ReactNode): Promise<string> {
  return new Promise((resolve: (html: string) => void, reject) => {
    // Collect all chunks into this string
    let html = ''

    // Create a writable stream that concatenates chunks
    const writableStream = new Writable({
      write(chunk, encoding, callback) {
        html += chunk
        callback()
      },
    })

    // Track error state
    let didError = false
    let error: Error | null = null

    const { pipe } = renderToPipeableStream(reactNode, {
      onShellError(err) {
        didError = true
        error = err as Error
        reject(error)
      },

      onError(err) {
        didError = true
        error = err as Error
        console.error('Error during rendering:', err)
      },

      // We use onAllReady instead of onShellReady to ensure all lazy/suspended content is loaded
      onAllReady() {
        try {
          // If we encountered an error earlier, don't proceed
          if (didError) {
            return reject(error)
          }

          // Pipe the complete content to our writable stream
          pipe(writableStream)

          // When the stream is done, resolve with the complete HTML
          writableStream.on('finish', () => {
            resolve(html)
          })

          writableStream.on('error', (err) => {
            reject(err)
          })
        } catch (err) {
          reject(err)
        }
      },
    })

    setTimeout(() => {
      reject(new Error(`Rendering timed out after 10 seconds`))
    }, 10_000)
  })
}

moishinetzer avatar Nov 18 '24 09:11 moishinetzer

That's the thing. SSR is one render. That means with the classical hooks you render a loading state and that's it.

What we did in the past was restart that render after all network requests finished, until no more network requests were started by Apollo Hooks, and see that as "the end of the render", and that would be what you'd have to reimplement: getMarkupFromTree

But you can see how that always was a suboptimal solution now that React can just "pause" rendering a component and waiting for the finished result with suspense - resulting in only one render of the tree.

phryneas avatar Nov 18 '24 10:11 phryneas

I see, so for now I should be able to go ahead with my method above so long as I change all my queries to use the useSuspenseQuery hook instead of useQuery!

moishinetzer avatar Nov 18 '24 11:11 moishinetzer