search

facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.6k stars 8.5k forks source link

Name of useIsBrowser hook and ExecutionEnvironment is misleading and the example given in the documentation needs a note added #8678

Open nynevi opened 1 year ago

nynevi commented 1 year ago

Have you read the Contributing Guidelines on issues?

Description

Hello,

Thanks for the amazing library.

The comment here states that useIsBrowser returns true only if the first hydration has completed.

But opening this Stackblits Example and checking the console, you see logs printed for the below code:

  const isBrowser = useIsBrowser();
  const traditionalIsBrowser = typeof window !== 'undefined';
  const testUrl = isBrowser ? new URL(window.location.href) : undefined;

  console.log('Has completed first hydration?', isBrowser);
  console.log('Are we actually on browser?', traditionalIsBrowser);
  console.log('Test URL:', testUrl);

Outputs before hydration, on first render:

Has completed first hydration? false
Are we actually on browser? true
Test URL: undefined

Outputs after hydration:

Has completed first hydration? true
Are we actually on browser? true
Test URL: URL {origin: 'https://githubev3fmt-xvco--3000.local-credentialless.webcontainer.io', protocol: 'https:', username: '', password: '', host: 'githubev3fmt-xvco--3000.local-credentialless.webcontainer.io', …}

So the name of useIsBrowser actually should be along the lines of useHasHydrationCompleted.

When there are a lot of window references in a component, and you try to build the application, you get this error:

It looks like you are using code that should run on the client-side only.
To get around it, try using `<BrowserOnly>` (https://docusaurus.io/docs/docusaurus-core/#browseronly) or `ExecutionEnvironment` (https://docusaurus.io/docs/docusaurus-core/#executionenvironment).
It might also require to wrap your client code in `useEffect` hook and/or import a third-party library dynamically (if any).

ReferenceError: window is not defined

So naturally your first instinct as a developer is to add <BrowserOnly> and useIsBrowser to everywhere you see window references unless you also read about ExecutionEnvironment, which I did much much later after I've already wasted tons of time.

I did not read ExecutionEnvironment up until creating this ticket because the ExecutionEnvironment did not sound like something I might need (I am burnt out enough to skip reading about it even though the error basically tells you to read it) however it's name is also misleading, I understand that it has canUseDOM for such use case but maybe it should rather be something along the lines of CanUse with:

What ExecutionEnvironment.canUseDOM does is actually what useIsBrowser should be doing therefore maybe these can be extracted into several hooks with appropriate names.

As a rule of thumb while naming things, I tend to name them according to what they are doing. So these would be:

When you have code like this: const testUrl = new URL(window.location.href), you end up making it (that is also what useIsBrowser documentation tells you to do) const testUrl = isBrowser ? new URL(window.location.href) : undefined

But because you just added isBrowser and make it purposefully return something else such asundefined, the code dependent starts acting as it should not. Then if you have tons of code that relies on testUrl and you suddenly return undefined with it, you end up modifying the entire stack of code to compensate for testUrl being undefined.

Now on the same Stackblitz example change the URL to include ?params=test123. You will see in the page under "Docusaurus Tutorial - 5min" area, the value rendered is "defaultValue" which is wrong (actually correct in terms of how React works with useState however wrong in terms of the end business goal). The correct would be test123

You start digging into what might be the reason and come to realization that you actually don't need to put isBrowser ? there at all because window.location.href in the browser is actually defined because after all literally the log you see is being printed in browser's console so why useIsBrowser is false, you start to wonder. When you remove isBrowser ? the code starts working but then the build fails with the above message again. Then you either go back to the documentation to read about that non-bold part easy to miss or dig into the source code of Docusaurus like I did to learn about Perils of Hydration.

As it is very cumbersome that you have to not only compensate now for the value being undefined, you also need to compensate for the fact that it may have falled back to another value. You start adding isBrowser checks for those parts too like const someQueryParam = isBrowser ? useState(isBrowser ? then it starts to not feel the right thing to do and look for another solution. I ended up doing this Stackblitz example with a proxy. Include ?params=test123 in the URL and the value rendered is now the expected value. Just to remind you that I did not read about ExecutionEnvironment yet at this point but even if I did, I would still be forced to do this proxy implementation with ExecutionEnvironment.canUseDom instead of traditionalIsBrowser in that code.

I would normally suggest changing the implementation of useIsBrowser to typeof window !== 'undefined' because as the developer, the goal of yours is to have your app build successfully by bypassing window being undefined. You don't care about perils of hydration, you just want to get your app built. But I've run into the problem of mismatching rendered HTML tons of times in NextJS and I know how painful it is to debug that so it makes sense to keep the best practice and don't change the implementation.

So an alternative we can do is to change the name of useIsBrowser to useHasHydrationCompleted but that would lead to many dependent applications breaking on the next version. Then we could add another hook useIsWindowDefined that does typeof window !== 'undefined' and then document it together with hasHydrationCompleted so people that are just getting started could use them together. Existing applications could maybe utilize codemods, and on next upgrade of an existing application, the codemod could run automatically and rename it and if it is possible that the codemod can add, then also add the useIsWindowDefined automatically. We can also link in the documentation simplified and shorter version of The perils of Rehydration article to help them explain why both of them need to be used.

Alternatively, if we don't change any names and leave it misleading as is, I have another suggestion in addition to having new hook called useIsWindowDefined (as changing name of ExecutionEnvironment, is yet another set of breaking changes and codemodes to write):

  1. improve the error message printed when build fails due to window problems to also document useIsBrowser linking to the documentation

    It looks like you are using code that should run on the client-side only. To get around it, try using one of:

    It might also require to wrap your client code in useEffect hook and/or import a third-party library dynamically (if any).

  2. improve useIsBrowser section that explains after first client render part a bit more clearly:

    You can also use the useIsBrowser() hook to test if the component has completed it's hydration. It returns false in SSR and true is CSR but only after first client render. If your build fails because window is undefined, and you actually need to check if you are in browser environment, you should combine this hook with useWindowIsDefined hook because this hook will return false when actually in browser environment and hydration has not completed and this may result in some of your code client side code that also rely on window not working properly. The example below kind of indicates that window.location.href is only available after first client render has completed, however it is actually already available when typeof window is defined, even before the first hydration has completed.

    Use this hook if you only need to perform certain conditional operations on client-side, but not render an entirely different UI.

I can have a PR for this, but I am not a native English speaker thus the wording would require a review on the PR and also I just need confirmation which path to take, as what I think "may be right" could be wrong for someone else thus we should come up with an optimal solution as the community then create a PR for it

Self-service

slorber commented 1 year ago

Hey,

That's a lot of text, hard to follow and I'll only get back to Docusaurus next week.

Maybe try to explain in a concise way what you are trying to achieve exactly, and I'll look into it once I have time.

The useIsBrowser() is indeed false on the first client render/hydration.

CleanShot 2023-02-17 at 19 23 37@2x

I would normally suggest changing the implementation of useIsBrowser to typeof window !== 'undefined'

We can't do that because React server-side-render and first client-side-render must render exactly the same thing. The behavior of useIsBrowser() is actually a way to prevent you from shooting in your own foot by doing what you seem to want to do, which will lead to React hydration mismatch problems. These problems are not apparent on React 17 (Docusaurus v2), but will throw exceptions on React 18 (coming with Docusaurus v3)

There are many articles online about these problems, for example https://www.joshwcomeau.com/react/the-perils-of-rehydration/

I'm sorry, I didn't have time to analyze all of your text in depth this week, but will look into it next week. Please try to be concise and talk about a specific problem you are trying to solve, instead of trying to say what you think we should do. I'll explain to you why we have this behavior, and how to overcome your problem based on a more concrete use-case. A wall of text is hardly actionable from my perspective and is likely a time sink leading to long discussions, time that I'd rather spend on other more concrete tasks. Hope you understand.

nynevi commented 1 year ago

@slorber yes I do understand that there are more pressing concerns and you'd prefer having the text shorter.

Now that I've read the Contributing guide, this may have been a proposal issue. Sorry if it is created with the wrong template

I can only do a TLDR as a comment because I do think the above wall of text, actually clearly describes the issue if you give it the time to read. There are nothing that says "you should do", it just says "I think this makes more sense and I am willing to contribute" and that you can only understand when you actually read the text. I think I did a good job of explaining the problem with enough examples and links to the appropriate areas and potential suggestions so that I can actually contribute by some guidance from you, as the maintainer. You could have actually saved minutes of your time trying to explain me things that I already explain myself as part of the description. Anyways the TLDR is:

"We" might need to "improve" parts of the build error messages about window being undefined and the part that explains useIsBrowser in the documentation because clearly, as that huge wall of text tries to explain, useIsBrowser does a different thing then what it's name states. It does useHasHydrationCompleted kind of behavior.

Anyways I don't have any blockers and no further enquiry about this task, again want to thank you and the community for the awesome library. Please let me know if I can be of help.

nynevi commented 1 year ago

@slorber I want to apologize for the huge wall of text. I created a PR with some code examples that explains the issue, it is obviously up to you to merge, my main motivation is to prevent another user from having the same confusion and be of help.

The huge wall of text is trying to actually explain the source problem whereas the PR is just a temporary remedy. It is up to the team and community to discuss further or not.

slorber commented 1 year ago

Hey @nynevi, I understand where you come from, how the name of this hook and all the hydration mismatch thing can be confusing sometimes.

For what it's worth, I think even if the name feels unintuitive, we document its behavior quite enough already:

CleanShot 2023-02-23 at 19 43 19@2x

https://docusaurus.io/docs/docusaurus-core#useIsBrowser

We could rename this hook to useHasHydrationCompleted but I think it's actually not too bad. If it was named useHasHydrationCompleted() then people may not find/use it and decide to use typeof window instead or something else we don't want.

We are not alone using a confusing name, the useSyncExternalStore exposes a getServerSnapshot() method that is also run on the 1st client hydration. This is IMHO a good way to encourage users to do the right thing and prevent hydration mismatches by default. See https://beta.reactjs.org/reference/react/useSyncExternalStore#usesyncexternalstore

You mention the introduction of a useful useIsWindowDefined() hook instead that would return true even on the first render. This is definitively an anti-pattern. Hooks are meant to be used in React components, and their return values are likely used in the render phase. We don't want that return value to be used in the render phase.

I tried to explain this briefly here: https://twitter.com/sebastienlorber/status/1615329010761842689

To clarify these 4 code snippets all lead to the exact same hydration mismatch problem:

function Comp() {
  const isBrowser = typeof window !== 'undefined';
  return isBrowser ? <div /> : <span />
}
function Comp() {
  const isBrowser = useIsWindowDefined();
  return isBrowser ? <div /> : <span />
}
function Comp() {
  const [bool, setBool] = useState(typeof window !== 'undefined');
  return bool ? <div /> : <span />
}
function Comp() {
  const isBrowser = useIsWindowDefined();
  const [bool, setBool] = useState(isBrowser);
  return bool ? <div /> : <span />
}

This hook is very likely to lead to anti-patterns, unless the return value is only used in callbacks.

For callbacks, ExecutionEnvironment or typeof window is fine.

Note that any usage of useIsWindowDefined() in the render path can lead to problems. This includes data derivation in useMemo, passing it as a props to subcomponents, using it as an arg to libraries like react-query and others.

Hope this makes sense?