-
Notifications
You must be signed in to change notification settings - Fork 47k
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
useLayoutEffect in ssr #14927
Comments
It’s there to force you to think about whether you truly need it to be useLayoutEffect (uncommon) or if you’re ok with it being useEffect (common). useLayoutEffect exists to give you strong guarantees about having the ability to adjust layout last minute without painting between. However we cannot guarantee that if you’re server rendering this component. It has to be resilient to work without this guarantee. |
Related thread on some of the effects of this in practice: Twitter Link. |
I can imagine some theoretical cases where it would be legit to want to useLayoutEffect only after some state has switched. However in practice I haven’t seen this yet. All cases I’ve seen so far have been slightly broken when you SSR if you think about it. It would be good to show actual examples if you have them. |
@sebmarkbage Still, the documentation is directly written that the useLayoutEffect phase is the same as componentDidMount / Update and these lifecycle methods are never executed on ssr, so why do the server complain to me about useLayoutEffect? |
I'm integrating a 3rd party lib that mutates DOM elements in the tree, and it needs to happen before first paint to prevent a "jitter" (when the tooltip updates its content while showing, the position needs to be updated synchronously or there will be a flicker). This warning occurs during SSR, but as mentioned above, I don't see why if it's equivalent to the cDM/cDU lifecycles that never did. The library is purely client-side, so it doesn't have any effects on the server markup. Tooltips don't get displayed on page load until hydration phase. So it can't be "broken" in that sense. |
Maybe this could work or would it break the rules of hooks? const useIsomorphicLayoutEffect = isBrowser ? useLayoutEffect : useEffect
function Comp() {
useIsomorphicLayoutEffect(() => {
// ...
})
} |
@atomiks How does that component work with server rendering? Isn’t there a small jitter? |
@sebmarkbage it's a tooltip library that creates a separate element that only exists after mounting. The element it's attached to gets rendered normally (server or client), but the tooltip doesn't appear until after hydration. DetailsThe content is rendered in a portal. When the content gets updated by React, the `.set()` method should be be called synchronously for the position to be updated. Because it uses position: absolute and doesn't exist in the normal flow of the document, changes to the size of the tooltip causes it to be repositioned in the wrong place, so the `translate` transform needs to be updated immediately before painting, or there will be a jitter.A ResizeObserver could also work, but not enough support yet. Demo with
No jitters with |
Any answers? |
@dimensi I am using the trick in #14927 (comment) and it seems to be working fine for my component library. I had to release a patch just to get around it. I don't see why there's even a warning at all, what's the point? We need to use hacks to get around it anyway because it is required in many legitimate cases, but still want to use it in SSR without problems (no-op behavior is fine anyway)... |
The point of the warning is to warn you that your component will behave weirdly before hydration. You may disagree or ignore the recommendation in specific cases if you want but it's pointing out a legitimate problem. |
If something only exists after hydration then the canonical solution to it is to render a fallback view first. And then flip a flag inside function useIsMounted() {
let [mounted, setMounted] = useState(false);
useEffect(() => {
setMounted(true);
}, []);
return mounted;
}
function Foo() {
let ref = useRef();
let isMounted = useIsMounted();
useEffect(() => {
if (isMounted) {
$.myJqueryPlugin(ref.current);
}
}, [isMounted]);
if (!isMounted) {
return <FallbackView />;
}
return <div ref={ref} />;
} |
It's not a problem in this case though, as mentioned above. And I still need Basically, I think warnings that can't be turned off when there are use cases for it should not exist. Just make it prominent on the docs which effect hook to use instead. |
I generally agree with that. That’s why the issue is still open. If there are legit use cases we’ll want to either adjust the warning or write documentation for it. Currently there are higher priority issues so we’re looking into this first. Since you can work around. We’ll get back to this and fix it after those. |
One thing that's a bit worrying with I think the differences between the hooks should be highlighted better (when to use them, etc.) I almost feel like |
The thing with jitter is that once you see it, you can fix it. With a layout effect. Or by calculating the correct initial state early (if you jitter due to a setState). It’s more annoying to first experience it — but it’s intentional that sometimes you’d see it and that would tell you “okay, here I need a layout effect instead”. This is feature working as designed. However the majority of code people put into DidMount/DidUpdate actually doesn’t need to be sync. So it’s a worse perf default which becomes death by a thousand cuts in a larger trees. In the past all code was sync. This has bad consequences for perf. Now it’s usually async, and when it causes jitter you just opt back into sync for that specific case. I think this makes sense. The naming is intentionally shorter for useEffect because we want you to try it first. And only replace it it actually causes a problem. (For most application code it doesn’t.) I agree it can be annoying if you see a warning which can’t be fixed. Collecting more use cases that seem legit here would help. For example I’m not sure the scroll one is legit if you use SSR. Scroll does represent a “layout effect” to me because you want it to happen together with layout instead or flickering. But what about SSR? You don’t want to start interacting with a page, scroll it, and then hydration scrolls it back. This is bad UX. So maybe your initial scroll logic should be outside of React components altogether, and could be inline in HTML. Then you’d be sure not to “scroll to top” too late. What do you think? More detailed use cases like this would help. |
This should probably be mentioned in the docs: https://reactjs.org/docs/hooks-reference.html#uselayouteffect That section makes it pretty clear to me that I should use Happy to submit a docs PR if I'm not the only one that finds the current documentation confusing. |
Sure, to be honest I want to rewrite that whole page because a lot of people find different parts about it confusing. |
I tweaked the docs to explain why this warning exists: https://reactjs.org/docs/hooks-reference.html#uselayouteffect |
Regarding your example in #14927 (comment). You didn't provide any details at all. What are the steps I need to do? What is the expected behavior? What is the actual behavior? Can you make a screenshot of the issue? It's very hard to help when you provide a couple hundred lines with no explanation of what problem you're running into. I understand the "abstract" problem (useLayoutEffect helps you in some way) but it would be so much easier to think about this if you provided the actual reproduction steps. |
It's starting to look like React-Redux's v7 implementation of The repro on this is somewhere between "complex" and "convoluted", but it involves a mixture of a sync I'm not sure I can even come up with a good TL;DR: for this one. @alexreardon has repro sandboxes linked in reduxjs/react-redux#1177 (comment) , and I wrote up a step-by-step description of what's going on in reduxjs/react-redux#1177 (comment) .
But, given that React-Redux is widely used for SSR, I don't want all our users to be seeing this warning printed all the time. As an additional edge case: I know I've seen cases where |
For timing reasons,
|
For the However you probably don't want to swap out the tree when this happens and you can't really swap out the component because it's a custom Hook. This sounds like a good use case for "progressively enhancing hooks" which is a thing we've been thinking about. It's basically a way to let you load new Hooks into a component while it's already running. These new Hooks can then useLayoutEffect because they're loaded late and never during SSR. This guarantees that even in the client case, this speeds up initial rendering and still forces the component to deal with the gap between initial render and progressive enhancement. |
For the Redux case I believe this is related to #15122 (comment) useEffect is basically just a concurrent mode-light and if you can't deal with the gap between rendering completing and commit phase, you're probably going to have even bigger problems when each render can also yield and gets dispatches injected in the middle. |
@gaearon |
as reduxjs/react-redux#1177 (comment) i suggest IMHO, that you explain in hooks docs, ref.current should be update as soon as there is a new value, like "last props received" As i can see, this seems to be a common useRef error. That's all |
Closing this as answered. Facebook essentially uses this strategy internally, and solving for the case where an effect condition can only happen due to an update is something we will look to support in the future as we introduce new APIs. |
To clarify, the link above is an escape hatch and should only be used when you understand the consequences. Before you use this, read this part of the docs:
If you absolutely need to work around it, sure, #14927 (comment) works. Although I'd say At FB, this Hook is called |
Is there a definitive implementation of the For React-Redux, we've had to keep making this more complicated, and run into issues due to folks running tests with Jest+JSDOM, as well as handling React Native. Issues for reference:
Frankly, this warning continues to be a pain for us to deal with. |
Giving this another bump, as |
@gaearon Regarding the tweak to the docs you made. I've been reading up a fair bit on the useLayoutEffect warning and SSR conversation. I think I fully understand the "what's happening" but still completely confused as to the why have a warning for only useLayoutEffect From those docs...
Why then are we providing a warning only for useLayoutEffect? Both will cause renders on the client and not on the server. The most I can reason, is that the react team have identified that many people are using For us, we explicitly use It seems there a plenty of valid use cases for |
Currently the warning does not work for the community
I think, I understand the original intent of introducing this warning to prevent UI jumps. But apparently the community does not benefit from this warning, it is mostly seen as an obstacle. Maybe it is time to revisit the warning, if its only purpose is to be suppressed by various workarounds? |
I wouldn't rely on isomorphic layout effect package. It's not maintaining regularly and it switches between so Abramov's gist more valid than the isomorphic package. |
Hi, I do not understand the situation with this hook a bit. I use this hook to perform the animation synchronously with the state update, if I use useEffect, then I have jumps in the animation, because the animation library does not have time to start. Also, the documentation states that useLayoutEffect runs on the same phase as componentDidMount (that is, on the client side), and here my server issues complaints to me about my code. Why is that?
https://codesandbox.io/s/oo47nj9mk9
Originally posted by @dimensi in #14596 (comment)
The text was updated successfully, but these errors were encountered: