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.

Sign up for GitHub

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

Jump to bottom

Fix hydration tests #2

Closed
wants to merge 1 commit into from
Closed

Fix hydration tests #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions src/react/tests/ssr-hydration.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import React from 'react'
import ReactDOM from 'react-dom'
import ReactDOM, { Root } from 'react-dom'
import ReactDOMTestUtils from 'react-dom/test-utils'
import ReactDOMServer from 'react-dom/server'
// eslint-disable-next-line import/no-unresolved -- types only for module augmentation
import type {} from 'react-dom/next'

import {
useQuery,
Expand All @@ -24,13 +26,12 @@ function setIsServer(isServer: boolean) {
const ReactHydrate = (element: React.ReactElement, container: Element) => {
// @ts-expect-error
if (String(process.env.REACTJS_VERSION || '18') === '18') {
let root: any
let root: Root
ReactDOMTestUtils.act(() => {
// @ts-expect-error
root = ReactDOM.hydrateRoot(container, element)
})
return () => {
root?.unmount()
root!.unmount()
}
}

Expand All @@ -49,7 +50,7 @@ function PrintStateComponent({ componentName, result }: any): any {
return `${componentName} - status:${result.status} fetching:${result.isFetching} data:${result.data}`
}

describe.skip('Server side rendering with de/rehydration', () => {
describe('Server side rendering with de/rehydration', () => {
let previousIsReactActEnvironment: unknown
beforeAll(() => {
// @ts-expect-error we expect IS_REACT_ACT_ENVIRONMENT to exist
Expand Down Expand Up @@ -96,6 +97,7 @@ describe.skip('Server side rendering with de/rehydration', () => {
'SuccessComponent - status:success fetching:true data:success'

expect(markup).toBe(expectedMarkup)
expect(fetchDataSuccess).toHaveBeenCalledTimes(1)

// -- Client part --
const consoleMock = mockConsoleError()
Expand All @@ -115,8 +117,8 @@ describe.skip('Server side rendering with de/rehydration', () => {

// Check that we have no React hydration mismatches
expect(consoleMock).not.toHaveBeenCalled()
expect(fetchDataSuccess).toHaveBeenCalledTimes(1)
expect(el.innerText).toBe(expectedMarkup)
expect(fetchDataSuccess).toHaveBeenCalledTimes(2)
expect(el.innerHTML).toBe(expectedMarkup)

unmount()
consoleMock.mockRestore()
Expand Down Expand Up @@ -177,12 +179,12 @@ describe.skip('Server side rendering with de/rehydration', () => {

// We expect exactly one console.error here, which is from the
expect(consoleMock).toHaveBeenCalledTimes(1)
expect(fetchDataError).toHaveBeenCalledTimes(1)
expect(fetchDataError).toHaveBeenCalledTimes(2)
expect(el.innerHTML).toBe(expectedMarkup)
await sleep(50)
expect(fetchDataError).toHaveBeenCalledTimes(2)
expect(el.innerHTML).toBe(
'ErrorComponent - status:error fetching:false data:undefined'
'<!-- -->ErrorComponent - status:error fetching:false data:undefined'
)

unmount()
Expand Down Expand Up @@ -240,12 +242,12 @@ describe.skip('Server side rendering with de/rehydration', () => {

// Check that we have no React hydration mismatches
expect(consoleMock).not.toHaveBeenCalled()
expect(fetchDataSuccess).toHaveBeenCalledTimes(0)
expect(fetchDataSuccess).toHaveBeenCalledTimes(1)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(el.innerHTML).toBe(expectedMarkup)
await sleep(50)
expect(fetchDataSuccess).toHaveBeenCalledTimes(1)
expect(el.innerHTML).toBe(
'SuccessComponent - status:success fetching:false data:success!'
'<!-- -->SuccessComponent - status:success fetching:false data:success!'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

unmount()
Expand Down