Fix hydration tests

[eps1lon]

Ref: https://twitter.com/TkDodo/status/1460639477379084299

act warnings

I don't get any missing act warnings locally

"Cannot update a component (...) while rendering a different component"

Getting "Cannot update a component (Page) while rendering a different component" locally. From my experience these are always implementation issues. Maybe you're setting state of some component while rendering another?

test mismatches

These look like they're asserting on render counts. I personally would not care about these.

[eps1lon]

These comment nodes have special meaning in React. I think the better assertion would use innerText to ignore HTML comment nodes.

[artem-malko]

Do you know, why React needs these comments? What is the purpose? As I understand, HTML-comments are ignored by React during hydration. And React uses it for its internal processes during selective hydration. Maybe you know more info about it?)

[eps1lon]

And React uses it for its internal processes during selective hydration.

That's pretty spot on. They have different markers for different markers. I know one type of marker is used for Suspense boundaries. This one my be for marking roots. But it doesn't really matter in the end for libraries.

[TkDodo]

thank again, but one question: I really don't understand why the number of calls to the mocked callback would be different? That doesn't make much sense to me :/

[eps1lon]

I'll take a look where this is called from.

[eps1lon]

Added an additional assertion to make it clear that one call is during SSR and one in CSR.

I guess the second call during hydration is unexpected?

Though it seems like this is expected considering it originates from https://github.com/eps1lon/react-query/blob/b92b2113ad52783c2e7794248b93f22ed2fba171/src/core/queryObserver.ts#L107 which sounds like that fetching on mount is intended. Earlier tests probably didn't catch this because you didin't flush effects scheduled during hydration i.e. wrapped hydrate in act.

I noticed some other oddities with how you use useSyncExternalStore:

1. https://github.com/eps1lon/react-query/blob/2ad404b62ce77e48d2108edf3ce2ed399482518d/src/react/useBaseQuery.ts#L89-L90
   I made that mistake as well originally. You really have to make sure that getServerSnapshot uses the server snapshot not the current snapshot i.e. getSnapshot and getServerSnapshot cannot read the same value. React 18: hydration mismatch when an external store is updated in an effect facebook/react#22361 looks a lot like what you're doing. Does this make sense?
2. https://github.com/eps1lon/react-query/blob/2ad404b62ce77e48d2108edf3ce2ed399482518d/src/react/useBaseQuery.ts#L97-L101
   That shouldn't be needed. That behavior should be implemented by useSyncExternalStore

[TkDodo]

Thanks again for looking into this. Yes, a refetch on mount is expected unless an explicit staleTime is set. This is actually mentioned right here in the docs, so I don't know how I didn't see it 🤦 . I'll probably add another test that makes sure we don't actually refetch on mount if we have a staleTime set, because I think that's missing.

ad 1: Thanks for pointing that out, I think you are right, this is exactly what I'm doing, mainly because I have no idea how I would compute a server snapshot correctly 😔 .

ad 2: It's indeed a leftover from the previous implementation. Do you know if the shim will also have this behaviour implemented? Because if it's just falling back to useEffect / forceRender, I think we'd need that to still work with React 17.

[TkDodo]

I also can't get the tests to run in v17, I think it's because of the uSES shim. What those tests do is they do something "on the server", then switch to client mode, render the same thing on the client and see if they match. However, the uSES shim imports two different versions (one for server, one for client). If I set the jest-environment to node, the client part fails because document.createElement doesn't exist. If I set it to jsdom, the server part fails because the client version of the shim uses useLayoutEffect.

we have this setIsServer function that tries to mock our internal utils so that RQ itself is in server/client mode, but I have no idea how to make the shim recognize that, too. Judging from the code, they also only check for window.document, but mocking this inside setIsServer had no effect so far...

https://github.com/facebook/react/blob/f320ef88f51fbdc5aba38bdf07108678a84f7339/packages/shared/ExecutionEnvironment.js#L10-L14

[eps1lon]

I guess so, too, but I'm still very fuzzy on the detail of how to send it from the server to the client, as I've never really worked in that area at all. I guess I'll have to wait until redux has this implemented or so for inspiration

I think you can't really solve that with zero config. You probably have to go document something like

const queryCache = {}
const finalHTML = renderToString(<CacheProvider><App /></CacheProvider>);
// queryCache is now populated

response.send(`<script>window.reactQueryCache = ${JSON.stringify(queryCache)};</script>`)
response.send(`<div id="react-container">{finalHTML}</div>`);

and then your getServerSnapshot reads from that global reactQueryCache.

Though this approach doesn't work that well for streaming rendering I believe. May make more sense to switch a complete Suspense implementation using reactwg/react-18#25 in React 18. The core team was asking for feedback on their cache API so this might be a good opportunity.

[TkDodo]

At this point I'm just going to loop in @artem-malko who has written a proposal for :

• react-query and New Suspense SSR Architecture in React 18 TanStack/query#2942

But without going all that way, what else could we do to make it backwards compatible? Should getServerSnapshot in useSyncExternalStorage just return undefined for now?

[eps1lon]

I think you can stick with the getSnapshot === getServerSnapshot approach to make it backwards compatible. But you have to be aware that this will lead to hydration mismatches with selective hydration ( see "React 18: Streaming HTML and Selective Hydration").

Regarding caching I really shouldn't be talking about it and the new cache API since I haven't worked in either problem space beyond testing.

[artem-malko]

@TkDodo Thx for the mention) I think, I'll create a separate test-case for SSR with renderToPipeableStream usage. So, I do not think, I will face with compatibility problems, cause I will use React 18 there. But, will see, how it will go)

@eps1lon, please, could you mention me, if you will find any cases of React.Cache usage in SSR streaming? This could help me a lot.

[Ephem]

This is an old discussion on a closed issue, but I'll post here anyway since it seemed a good place. 😄

About getServerSnapshot, this should be based off the initial queryCache like @eps1lon noted. When people do SSR with RQ today, this is already available in the form of the dehydratedState (or rather the internal representation of the queryCache right after that state has been hydrated, before anything has changed). The way the dehydratedState ends up on the client varies by framework, in the Next.js-demo it looks like this for example: https://github.com/TkDodo/react-query/blob/master/examples/nextjs/pages/_app.js#L10

It might not always be that straightforward though, I see in the WIP PR that different things gets passed into getSnapshot in different places, for example () => observer.getCurrentResult(). The getServerSnapshot would have to be the same thing, but calculated from the initial cache so to speak.

I noted this over in TanStack#2942 (comment) as well, but I think we should probably focus on the "everything is available before the client starts hydrating" case first before we start experimenting with the experimental Suspense cache. 😃

I plan to dive into this more and look at the WIP PR in more detail, but wanted to comment this right away.

[TkDodo]

I have, for now, decided to disable the ssr-hydration test for the react 17 env, as I couldn't solve the useLayoutEffect problem with the shim. They run fine under react 18 with your adaptions, and I could also fix the act warnings. I've adapted your changes already, so closing this PR. Thank you again for you help here 🙏