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

Re-queries on hydration after ssr using renderToPipeableStream #11319

Open
iamafansev opened this issue Oct 28, 2023 · 12 comments
Open

Re-queries on hydration after ssr using renderToPipeableStream #11319

iamafansev opened this issue Oct 28, 2023 · 12 comments

Comments

@iamafansev
Copy link

iamafansev commented Oct 28, 2023

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
@phryneas
Copy link
Member

phryneas commented Nov 2, 2023

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.

@kcoet
Copy link

kcoet commented Nov 21, 2023

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?

@phryneas
Copy link
Member

@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
Copy link
Member

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 :)

@alvaro-cuesta
Copy link

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.

@jerelmiller jerelmiller removed the 🏓 awaiting-team-response requires input from the apollo team label May 29, 2024
@moishinetzer
Copy link

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

@phryneas
Copy link
Member

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.

@moishinetzer
Copy link

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

@phryneas
Copy link
Member

@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.

@moishinetzer
Copy link

moishinetzer commented Nov 18, 2024

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)
  })
}

@phryneas
Copy link
Member

phryneas commented Nov 18, 2024

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.

@moishinetzer
Copy link

moishinetzer commented Nov 18, 2024

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!

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

No branches or pull requests

7 participants