-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Name of useIsBrowser hook and ExecutionEnvironment is misleading and the example given in the documentation needs a note added #8678
Comments
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
We can't do that because React server-side-render and first client-side-render must render exactly the same thing. The behavior of 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. |
@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 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. |
@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. |
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: https://docusaurus.io/docs/docusaurus-core#useIsBrowser We could rename this hook to We are not alone using a confusing name, the You mention the introduction of a useful 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, Note that any usage of Hope this makes sense? |
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:
Outputs before hydration, on first render:
Outputs after hydration:
So the name of
useIsBrowser
actually should be along the lines ofuseHasHydrationCompleted
.When there are a lot of
window
references in a component, and you try to build the application, you get this error:So naturally your first instinct as a developer is to add
<BrowserOnly>
anduseIsBrowser
to everywhere you seewindow
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 ofCanUse
with:CanUse.DOM
CanUse.IntersectionObserver
CanUse.Viewport
What
ExecutionEnvironment.canUseDOM
does is actually whatuseIsBrowser
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 ontestUrl
and you suddenly returnundefined
with it, you end up modifying the entire stack of code to compensate fortestUrl
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 betest123
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 becausewindow.location.href
in the browser is actually defined because after all literally the log you see is being printed in browser's console so whyuseIsBrowser
is false, you start to wonder. When you removeisBrowser ?
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 oftraditionalIsBrowser
in that code.I would normally suggest changing the implementation of
useIsBrowser
totypeof 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
touseHasHydrationCompleted
but that would lead to many dependent applications breaking on the next version. Then we could add another hookuseIsWindowDefined
that doestypeof window !== 'undefined'
and then document it together withhasHydrationCompleted
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 theuseIsWindowDefined
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):window
problems to also documentuseIsBrowser
linking to the documentationI 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
The text was updated successfully, but these errors were encountered: