Update HydrationStreamProvider.tsx + Advanced SSR docs to fix unnecessary suspensions #6753
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e895537:
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e895537. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
packages/react-query-next-experimental/src/HydrationStreamProvider.tsx
Outdated
Show resolved
Hide resolved
docs/react/guides/advanced-ssr.md
Outdated
| // Browser: make a new query client if we don't already have one | ||
| // This is very important so we don't re-make a new client if React | ||
| // supsends during the initial render |
There was a problem hiding this comment.
I guess that's why a Suspense boundary is a good idea - so that you can create a queryClient in React State outside of it, and it won't be thrown away. I don't mind the makeQueryClient()function - it's good that it uses a singleton on the client and a new one every time on the server 👍 . I would still probably mention somewhere that this is only needed if you don't have an explicit suspense boundary further down the tree...
|
@juliusmarminge would you mind taking a look here? Also I think the comment in the issue is super interesting: |
| // Setup and run the onEntries handler on the client only, but do it during | ||
| // the initial render so children have access to the data immediately | ||
| // This is important to avoid the client suspending during the initial render | ||
| // if the data has not yet been hydrated. |
There was a problem hiding this comment.
I'm not super on top of the react-query-next-experimental library so not sure if use cases and edge cases align, but, I thought I'd note that it might be worth checking out this comment (and implementation) in the HydrationBoundary for how we handle hydration there: https://github.com/TanStack/query/blob/main/packages/react-query/src/HydrationBoundary.tsx#L35-L49
Short version, what we do there is that if data is not already in the cache, we hydrate in render, if it is, we hydrate in an effect so we don't cause side effects inside of render.
I'm not sure how likely this is to happen with the streaming library, if it can at all, but I thought I'd mention it.
There was a problem hiding this comment.
Interesting callout! I think it's a different situation here, and what we're doing is safe. HydrationBoundary could happen at any part of the tree, whereas there will only be one ReactQueryStreamedHydration and it will be high up in the tree / wrapping the whole thing. This code will only be called once on boot, and we went any streamed data to be available ASAP on the initial render phase.
Definitely let me know if you see any problems with that logic!
There was a problem hiding this comment.
I also think the whole hydration can only happen once, on the first render, where the cache is still empty. I'd give this a go as-is.
I added a small refactoring commit: bb51270
There was a problem hiding this comment.
Thanks for taking this across the finish line @TkDodo!
docs/react/guides/advanced-ssr.md
Outdated
| function makeQueryClient() { | ||
| return new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| // With SSR, we usually want to set some default staleTime | ||
| // above 0 to avoid refetching immediately on the client | ||
| staleTime: 60 * 1000, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| let browserQueryClient: QueryClient | undefined = undefined | ||
|
|
||
| function getQueryClient() { | ||
| if (typeof window === 'undefined') { | ||
| // Server: always make a new query client | ||
| return makeQueryClient() | ||
| } else { | ||
| // Browser: make a new query client if we don't already have one | ||
| // This is very important so we don't re-make a new client if React | ||
| // supsends during the initial render | ||
| if (!browserQueryClient) browserQueryClient = makeQueryClient() | ||
| return browserQueryClient | ||
| } | ||
| } | ||
|
|
||
| export default function Providers({ children }) { | ||
| const [queryClient] = useState( | ||
| () => | ||
| new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| // With SSR, we usually want to set some default staleTime | ||
| // above 0 to avoid refetching immediately on the client | ||
| staleTime: 60 * 1000, | ||
| }, | ||
| }, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
I'm not sure these parts of the docs needs to change? Note that most of the examples on this page until the streaming library part uses useQuery and prefetching, which I don't think should be susceptible to this edge case? But I haven't thought about this as long as you have, so maybe I'm missing something?
Not something I feel strongly about though and happy to keep it as is to keep it aligned too.
If we do keep the original docs, I would still love a comment here along the lines of "if you are using useSuspenseQuery, initiating the QueryClient as state can have edge cases, see the docs on streaming further down".
There was a problem hiding this comment.
I think it's important to put this in the docs in both places. I'm betting that many folks (like myself) will copy-paste the first example that they see, and having the useState in there will lead to a broken example by default until they add a suspense boundary.
cleanup ts-expect-error by re-using `win`, which is already `any`
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #6753 +/- ##
=======================================
Coverage 41.78% 41.78%
=======================================
Files 178 178
Lines 7014 7014
Branches 1416 1416
=======================================
Hits 2931 2931
Misses 3721 3721
Partials 362 362 |
This PR is based on my findings in #6116 and fixes two cases of unnecessary suspensions during streaming SSR, one on code and the other in the advanced SSR docs. See the full writeup in this comment, which should have all the info needed for reviewing this PR.
fixes #6116