apollographql / apollo-client-nextjs

Apollo Client support for the Next.js App Router
https://www.npmjs.com/package/@apollo/experimental-nextjs-app-support
MIT License
438 stars 32 forks source link

Not sending data to the client. #325

Closed nitedani closed 2 months ago

nitedani commented 3 months ago

I am trying to integrate this with

It's almost working, my code looks like this:

import { WrapApolloProvider } from '@apollo/client-react-streaming'
import { buildManualDataTransport } from '@apollo/client-react-streaming/manual-transport'
import { useStream } from 'react-streaming'
import { renderToString } from 'react-dom/server'
import React from 'react'

export const WrappedApolloProvider = WrapApolloProvider(
  buildManualDataTransport({
    useInsertHtml() {
      const stream = useStream()
      if (!stream) {
        return () => {}
      }
      return (callback: () => React.ReactNode) => {
        const reactNode = callback()
        const injectionsString = renderToString(reactNode)
        console.log({ reactNode, injectionsString })
        stream.injectToStream(injectionsString)
      }
    }
  })
)

Issue: the data is not sent to the client. The output of the console.log above is the following:

{
  reactNode: {
    '$$typeof': Symbol(react.transitional.element),
    type: [Function: RehydrateOnClient],
    key: null,
    props: {},
    _owner: null,
    _store: {}
  },
  injectionsString: ''
}
{
  reactNode: {
    '$$typeof': Symbol(react.transitional.element),
    type: [Function: RehydrateOnClient],
    key: null,
    props: {},
    _owner: null,
    _store: {}
  },
  injectionsString: '<script>(window[Symbol.for("ApolloSSRDataTransport")] ??= []).push({"rehydrate":{},"events":[{"type":"started","options":{"fetchPolicy":"cache-first","query":"query Dragons{dragons{name first_flight diameter{feet}launch_payload_mass{lb}}}","notifyOnNetworkStatusChange":false,"nextFetchPolicy":undefined},"id":"1"}]})</script>'
}
{
  reactNode: {
    '$$typeof': Symbol(react.transitional.element),
    type: [Function: RehydrateOnClient],
    key: null,
    props: {},
    _owner: null,
    _store: {}
  },
  injectionsString: ''
}
{
  reactNode: {
    '$$typeof': Symbol(react.transitional.element),
    type: [Function: RehydrateOnClient],
    key: null,
    props: {},
    _owner: null,
    _store: {}
  },
  injectionsString: '<script>(window[Symbol.for("ApolloSSRDataTransport")] ??= []).push({"rehydrate":{},"events":[{"type":"complete","id":"1"}]})</script>'
}

As you can see, the callback is called 4 times, but it produces only 2 script tags, with no data included. image

nitedani commented 3 months ago

I can prepare a reproduction if it would help.

nitedani commented 3 months ago

It works in your vite example because renderInjectedHtml is called with await, so it executes after Reflect.set(...args). https://github.com/apollographql/apollo-client-nextjs/blob/e7a59cb26716a77bbfac659f435f89f5af8eff61/packages/client-react-streaming/src/stream-utils/createInjectionTransformStream.tsx#L91

This is the cause: https://github.com/apollographql/apollo-client-nextjs/blob/e7a59cb26716a77bbfac659f435f89f5af8eff61/packages/client-react-streaming/src/ManualDataTransport/RehydrationContext.tsx#L102

I see that renderInjectedHtml needs to be delayed to send the data, because ensureInserted() is called before Reflect.set(...args). You are calling renderInjectedHtml with await, that delays it just enough. Is this intentional?

nitedani commented 3 months ago

I figured it out, the following works: Just needed to add await in front of the callback. 😄 Had to dig a little.

import { WrapApolloProvider } from '@apollo/client-react-streaming'
import { buildManualDataTransport } from '@apollo/client-react-streaming/manual-transport'
import { useStream } from 'react-streaming'
import { renderToString } from 'react-dom/server'
import React from 'react'

export const WrappedApolloProvider = WrapApolloProvider(
  buildManualDataTransport({
    useInsertHtml() {
      const stream = useStream()
      if (!stream) {
        return () => {}
      }
      return async (callback: () => React.ReactNode) => {
        const reactNode = await callback()  // <--- added await
        const injectionsString = renderToString(reactNode)
        stream.injectToStream(injectionsString)
      }
    }
  })
)
github-actions[bot] commented 3 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

phryneas commented 3 months ago

Hmm... I believe what you have here doesn't work:

return async (callback: () => React.ReactNode) => {
        const reactNode = await callback()  // <--- added await
        const injectionsString = renderToString(reactNode)
        stream.injectToStream(injectionsString)
      }

This delays the injection into the stream - but without blocking the stream, so React will already start to stream content that depends on the value that will only be injected later.

You'd need to pause the stream, await, inject, and then resume normal stream functionality.

PS: super cool to see more people playing with this! Please share your final result - maybe we can even add it as another integration test?

brillout commented 3 months ago

Thanks for your answer, that makes sense.

I'm currently working on fixing it (I'm very looking forward to vike-react-apollo) but I'm having issues because the injected chunks are being inserted in the wrong place.

Is there a rule of when it's allowed to inject to the stream? I've yet to read an official guide from React about this.

PS: super cool to see more people playing with this! Please share your final result - maybe we can even add it as another integration test?

Apollo has quite a lot of goodies to make integration easy, it's quite neat. And, yes, we'd be up for an integration test!

phryneas commented 3 months ago

There's no official guidance on when it is safe to inject - our createInjectionTransformStream is based on the implementation from Next.js in the hopes that they know what they are doing.

Generally: it seems that React might inject multiple chunks that belong together, but always does so synchronously. So waiting for the next Promise.resolve().then, setImmediate or similar should always give you a safe point in time to inject.

phryneas commented 3 months ago

Generally, did you see https://github.com/apollographql/apollo-client/pull/11807/files#diff-bd3d926d0da12f95cf1980b882dc8bb6f70b4e542f3673ea96f0bb21c281118f ?

brillout commented 3 months ago

it seems that React might inject multiple chunks that belong together, but always does so synchronously

That was my guess as well. (Funny coincidence.)

Good to know that I ain't alone with my guess.

Generally, did you see https://github.com/apollographql/apollo-client/pull/11807/files#diff-bd3d926d0da12f95cf1980b882dc8bb6f70b4e542f3673ea96f0bb21c281118f ?

That looks quite neat CC @nitedani.

brillout commented 3 months ago

Generally: it seems that React might inject multiple chunks that belong together, but always does so synchronously. So waiting for the next Promise.resolve().then, setImmediate or similar should always give you a safe point in time to inject.

FYI that's what react-streaming originally did, but I moved away from this approach. Instead, whenever React writes, react-streaming synchronously adds React's chunk to its internal buffer (an array of chunks), so that injection order is respected. I find this approach a lot more reliable and flexible, and easier to reason about.

phryneas commented 3 months ago

FYI that's what react-streaming originally did, but I moved away from this approach. Instead, whenever React writes, react-streaming synchronously adds React's chunk to its internal buffer (an array of chunks), so that injection order is respected. I find this approach a lot more reliable and flexible, and easier to reason about.

Interesting!

Tbh., I don't really want to overengineer that part. For what we are doing, I see it as a crutch we have to use until React gets their story on stream injection of values from SSR to the Browser in place. I still hope that we'll get an early React 19.x minor that has that functionality natively and then we can drop it.

brillout commented 3 months ago

I still hope that we'll get an early React 19.x minor that has that functionality natively and then we can drop it.

Me too, although I haven't heard any news about this nor injectToStream.

phryneas commented 3 months ago

Last I heard from the team they were trying to find more granular things than an injectToStream helper, but I have no idea on their current stance.

I tried to hack together my "optimal" api by just editing React sources, and have multiple attempts, if you're interested in the exploration :)

brillout commented 2 months ago

I guess we can close this (I can't do it as I ain't OP), thank you @phryneas for your help!

phryneas commented 2 months ago

Good call - I believe the release of this was just announced at https://x.com/vike_js/status/1813474021201027129, so I'm going to close this here :)

github-actions[bot] commented 2 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.