-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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.
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
docs(core): explaining the difference between "hydration completion" versus "actually being in the browser environment" in useIsBrowser
hook
#8679
base: main
Are you sure you want to change the base?
Conversation
…versus "actually being in the browser environment" in `useIsBrowser()` hook
…ompletion" versus "actually being in the browser environment" in `useIsBrowser()` hook
…planation about "hydration completion" versus "is in the browser environment"
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here to understand better why I think some of your suggestions do not make sense:
You want to add some docs, I'm not against, but your caveat section feels a bit too much. More importantly, this section also applies to any other React SSR framework, so it should probably be documented in a separate place we can link to.
website/docs/advanced/ssg.mdx
Outdated
return null; | ||
} | ||
|
||
return <MyComponent />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use in the parent component, but you still have to split this into 2 components
website/docs/docusaurus-core.mdx
Outdated
There are a couple more alternative solutions to this problem. However all of them require adding checks in **the parent component**: | ||
|
||
1. You can wrap `<MyComponent />` with [`BrowserOnly`](./docusaurus-core.mdx#browseronly) | ||
2. You can use `canUseDOM` from [`ExecutionEnvironment`](./docusaurus-core.mdx#executionenvironment) and `return null` when `canUseDOM` is `false` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no you shouldn't do that, otherwise it leads to an hydration mismatch:
- server: return null
- client first render: return JSX
it should be the same on server/first client render. It's not just JSX1 vs JSX2, but also null vs JSX.
website/docs/advanced/ssg.mdx
Outdated
``` | ||
|
||
#### A caveat to know when using `useIsBrowser` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this duplicated whole paragraph looks overkill to me. We probably don't want twice the same doc fragment like that.
Also these hydration problems are a bit out of the scope of Docusaurus, they also affect any other framework using hydration. You should normally not use any value from window in the render path, including in useState initializers (tried to explain a bit here: https://twitter.com/sebastienlorber/status/1615329010761842689)
website/docs/docusaurus-core.mdx
Outdated
const url = isBrowser ? new URL(window.location.href) : undefined; | ||
const someQueryParam = url?.searchParams.get('someParam'); | ||
const [someParam, setSomeParam] = useState(someQueryParam || 'fallbackValue'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should rather not read the querystring or any client-side value in the render path in any SSR React app (not just Docusaurus). Similarly, this can lead to hydration mismatches, not to mention that the value is not updated on navigation.
Values derived from querystring should also not use memo or whatever but effects instead.
useSyncExternalStore is a great way to read the querystring IMHO, see https://thisweekinreact.com/articles/useSyncExternalStore-the-underrated-react-api
…as they are not in the scope of only Docusaurus but all the React SSR frameworks and extending the existing example to include a bit more context
…IsBrowser` as they are not in the scope of only Docusaurus but all the React SSR frameworks and extending the existing example to include a bit more context
…` documentation to promote use of <BrowserOnly>
Pre-flight checklist
Motivation
Having read the Perils of Rehydration and having known why
useIsBrowser()
does not check the existence of the window, I found that it is helpful to the community by giving an example that explains "hydration completion" versus "is in browser environment" are two different things and their code may not work if they simply trust in the name of the hook which plainly statesuseIsBrowser
while it actually acts asuseHasHydrationCompleted
.As a developer trying to get a successful build and thanks to the error message shown while building for window being undefined, I was able to get a successful build however I ran into problems with some business logic not working as intended and spent some time on debugging and coming to a solution, I think this will help other community members better understand the problem.
The related issue, although long and may be confusing, tries to explain some more fine-grained solutions than simply documenting an example for the issue.
Even if this PR gets merged, these problems will still remain:
useIsBrowser
will keep confusing people because the name of the function does not do what it does internallyuseIsBrowser
only; when you have reference variables derived from the variable that has the check, the values of the referenced variables will be wrong thus cause business logic problemsuseIsBrowser
Although this is issue is tiny and links to the Perils of Rehydration and there are more pressing concerns, it may deserve a bit more thought and discussion and maybe some refactoring/additions later.
Related issues/PRs
Related issue: #8678