Support async render function in getDataFromTree / getMarkupFromTree

[richardscarrott]

We've found React.renderToString / React.renderToStaticMarkup is a) very expensive and b) blocking, so we're looking to replace it with a non-blocking async version.

However, the most costly call site is in getDataFromTree due to the recursive nature, so it would be great if Apollo Client would support async render functions -- I don't believe there will be any performance penalty for those using sync functions as it's already running in a microtask.

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[apollo-cla]

@richardscarrott: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[benjamn]

I like this idea and I don't see any problems with it, since this code is already async. However, I think we can avoid using async/await syntax here, in favor of the vanilla Promise API, so I pushed a couple of commits to do that. No action needed on your part, unless you have additional thoughts. Thanks!

[benjamn]

Heads up: we're not quite ready to publish @apollo/client@3.1.0 yet, but I've published an @apollo/client@3.1.0-pre.0 release that allows an async options.renderFunction, if you want to try that in the meantime. 🙏

[richardscarrott]

Hey @benjamn thanks for merging this, much appreciated!

Does @apollo/client export getDataFromTree / getMarkupFromTree or will I need a prerelease of @apollo/react-ssr also?

[richardscarrott]

Somewhat relatedly, I quickly setup a test repo for this change and noticed, even when going through the normal sync approach, calling renderToString(root) after await getDataFromTree(root) ends up in a loading state for some reason -- pretty certain this doesn't happen in older versions -- I will test again when I'm able to import a matching @apollo/react-ssr version.

https://github.com/richardscarrott/apollo-client-async-ssr-pr-test/blob/master/src/index.tsx#L73

[benjamn]

@richardscarrott You can now import { getDataFromTree } from "@apollo/client/react/ssr"!

In fact, if you keep using @apollo/react-ssr, you may not get the latest code, depending on what version of that package you're using. We hope the consolidation of React-related packages into @apollo/client/react/* (which will be improved in v3.1.0 by #6656) makes these imports a lot more consistent/predictable.

[j-5-s]

This is exactly the feature I needed and seems to work with renderToNodeStream. However i am seeing another issue in 3.1.0-pre.2. I'm using the MockedProvider not just for tests but for mocking in a sandbox ui environment. With this new version importing @apollo/client/testing will throw the below error:

(node:81032) UnhandledPromiseRejectionWarning: ReferenceError: it is not defined
    at Object.<anonymous> (...@apollo/client/utilities/testing/itAsync.js:7:22)

This does not happen in 3.0.2.

[benjamn]

@JamesCharlesworth See 4589606 for a quick fix for that problem. Thanks for letting us know about it.

[benjamn]

Following up: we just published @apollo/client@3.1.0 to npm!

[j-5-s]

Great, thanks for the quick response @benjamn!

[richardscarrott]

@benjamn Just tested with the correct import and it works great, thanks!

As an aside, I just noticed Apollo exposes renderToStringWithData which calls into getMarkupFromTree using React.renderToString instead of React.renderToStaticMarkup and wondered whether it's safe to avoid the extra user land React.renderToString call when using getDataFromTree/ getMarkupFromTree as recommended in the docs when a) you're not hydrating in the client or b) you're passing in a React.renderToString equivalent as the renderFunction?

Also, I guess there must have once been a significant performance difference between React.renderToString and React.renderToStaticMarkup to warrant the separate APIs, however, at least in latest react, my benchmarks show just a 0.1-3.45% performance difference in favour of renderToStaticMarkup depending on the size of the component tree so it probably doesn't outweigh the need for an additional render in user code anymore?

[ndreckshage]

seems to work with renderToNodeStream

This is not entirely correct for anyone else that comes across this. Just FYI cc @JamesCharlesworth

https://github.com/apollographql/apollo-client/blob/master/src/react/ssr/getDataFromTree.ts#L52 this throws away HTML and then repeats the process if if finds promises from rendering.

Therefor if you attempt to pass in render to node stream with:

const htmlStream = await getMarkupFromTree({
  tree,
  renderFunction: renderToNodeStream,
});

// ...

htmlStream.on('data', (chunk) => writeChunk(chunk.toString()));

It will "work" but HTML will have been already flushed, meaning Apollo would have rendered in the loading state.

This won't work until SSR suspense supported: https://reactjs.org/docs/concurrent-mode-suspense.html (mentioned here as well: #5357)

[j-5-s]

@ndreckshage yes, you are correct. I feel like it had some performance benefit as more non-blocking that renderToStaticMarkup but purely anecdotal and dont have anything to back it up.

I did find a way to call renderToNodeStream though for certain use cases. If you know your data needs at the request time (the queries that will fire from a react tree), call graphql (using client.query) before I render the react tree then tell apollo to only read from cache serverside. This makes apollo never go into loading state and has data right when useQuery is invoked, allowing renderToNodeStream to work.

[richardscarrott]

@ndreckshage @JamesCharlesworth you're right you can't use renderToNodeStream directly but you can try wrapping it in a promise as I've done here https://github.com/richardscarrott/react-render-to-string-async/blob/master/src/index.js

We've been using renderToStringAsync in production (with getDataFromTree) and it's been fine, I've not performed any load testing to confirm whether it's beneficial or not but that repo has a few benchmarks which seem to suggest it can be slightly faster regardless of load 🤷‍♂️.