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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nynevi
Copy link

@nynevi nynevi commented Feb 17, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

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 states useIsBrowser while it actually acts as useHasHydrationCompleted.

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 internally
  • The fact that you cannot rely on useIsBrowser 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 problems
  • The error message printed on the console does give links to the right areas but lacks the link to the useIsBrowser

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

…versus "actually being in the browser environment" in `useIsBrowser()` hook
…ompletion" versus "actually being in the browser environment" in `useIsBrowser()` hook
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 17, 2023
…planation about "hydration completion" versus "is in the browser environment"
@netlify
Copy link

netlify bot commented Feb 17, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit d53136d
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6440ec2119a6de0008ab9102
😎 Deploy Preview https://deploy-preview-8679--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Feb 17, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 57 🟢 97 🟢 92 🟢 100 🟢 90 Report
/docs/installation 🟠 69 🟢 100 🟢 92 🟢 100 🟢 90 Report

Copy link
Collaborator

@slorber slorber left a 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:

#8678 (comment)

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.

return null;
}

return <MyComponent />;
Copy link
Collaborator

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

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`
Copy link
Collaborator

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.

```

#### A caveat to know when using `useIsBrowser`
Copy link
Collaborator

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)

Comment on lines 447 to 449
const url = isBrowser ? new URL(window.location.href) : undefined;
const someQueryParam = url?.searchParams.get('someParam');
const [someParam, setSomeParam] = useState(someQueryParam || 'fallbackValue');
Copy link
Collaborator

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants