facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.02k stars 46.86k forks source link

useLayoutEffect in ssr #14927

Closed dimensi closed 4 years ago

dimensi commented 5 years ago

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 https://github.com/facebook/react/pull/14596#issuecomment-466023638

4px commented 5 years ago

I think the point of the warning is that the server rendering DOM and DOM built on the client will be different when the layoutEffect does something with DOM. But there are times when you need to do something before browser rendering. For example set scrollTo(0, 0) or set attributes of a title and meta tags (SSR does it too, but without effects). With useEffect there is a delay. In this case it remains only to make a stub:

import React from 'react';

// Skip layoutEffects on server rendering to prevent warning
const isServerSide = typeof window == 'undefined';
const useLayoutEffect = isServerSide ? () => {} : React.useLayoutEffect;

...
EECOLOR commented 5 years ago

@4px I afree with what you are saying. In my mind it however does not make sense if you have to do this in every project while it can be solved by the library.

Another (maybe) less intrusive option would be to give the ability to disable the warning. This however is a bit more dangerous because people might forget to add the following header to the effect function:

React.useLayoutEffect(
  () => {
    if (typeof window !== 'undefined') return
    ...
  },
  []
)
markerikson commented 5 years ago

React-Redux has actually just run into a potential bug, as our current feature-detection logic appears to mis-identify RN as Node:

https://github.com/reduxjs/react-redux/issues/1437

The current implementation is:

const useIsomorphicLayoutEffect =
  typeof window !== 'undefined' &&
  typeof window.document !== 'undefined' &&
  typeof window.document.createElement !== 'undefined'
    ? useLayoutEffect
    : useEffect

We may need to add a check for RN in there, possibly something like navigator && navigator.product = 'ReactNative' (per https://stackoverflow.com/a/39473604/62937 ).

Anyone else see something like this?

And yes, all these shenanigans wouldn't be necessary if this warning just wasn't printed in the first place.

eps1lon commented 5 years ago

Anyone else see something like this?

The problem generally is that this warning depends on what API you use not what environment you're in. So anything relying on environment sniffing will never be complete since you can use most of react-dom/server in the browser or in a node test environment with a global jsdom.

And yes, all these shenanigans wouldn't be necessary if this warning just wasn't printed in the first place.

Agreed. The issue described in the warning cannot be fixed anyway. You get layout shift whether you use useLayoutEffect, or useEffect or csr only mounting in any way. The workarounds in the docs do not fix the issue at all but make them more obvious. Apparently it's better to always have a layout shift instead of only once?

Especially statements like

This way, the UI doesn’t appear broken before hydration.

are incredibly frustrating. Just because we have to handle layout on the client doesn't mean the UI is "broken". And deferring it to a useEffect does not magically make the UI not broken. I guess this is where something like an enhanced effect hook would be nice: useEffect when hydrating but useLayoutEffect on render.

aleclarson commented 4 years ago

The easiest solution for library authors is to use this package.

necolas commented 4 years ago

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.

gaearon commented 4 years ago

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 use server rendering, keep in mind that neither useLayoutEffect nor useEffect can run until the JavaScript is downloaded. This is why React warns when a server-rendered component contains useLayoutEffect. To fix this, either move that logic to useEffect (if it isn’t necessary for the first render), or delay showing that component until after the client renders (if the HTML looks broken until useLayoutEffect runs).

To exclude a component that needs layout effects from the server-rendered HTML, render it conditionally with showChild && <Child /> and defer showing it with useEffect(() => { setShowChild(true); }, []). This way, the UI doesn’t appear broken before hydration.

If you absolutely need to work around it, sure, https://github.com/facebook/react/issues/14927#issuecomment-466815232 works. Although I'd say useIsomorphicLayoutEffect is an extremely confusing name for this pattern.

At FB, this Hook is called useLayoutEffect_SAFE_FOR_SSR which calls out that you understand the consequences. Maybe you'll want to name yours something similar.

markerikson commented 4 years ago

Is there a definitive implementation of the isBrowser check that can be officially recommended?

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.

StevieJayCee commented 3 years ago

Giving this another bump, as useIsomorphicLayoutEffect gets confused by Jest+JSDOM as @markerikson pointed out. We either have lots of test log noise withthis error warning or have a separate maintenance overhead with separated test classes for SSR, annotated with the alternative jest node environment.

iDVB commented 3 years ago

@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...

"keep in mind that neither useLayoutEffect nor useEffect can run until the JavaScript is downloaded. This is why React warns when a server-rendered component contains useLayoutEffect. "

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 useLayoutEffect when it would be better to use useEffect. This seems like something better to leave to docs and education rather than forced warnings.

For us, we explicitly use useLayoutEffect in order to setup complex timelined animations. Using useEffect causes blinking between routes and on mount, as would lazy loading those animations. Using useLayoutEffect has served us perfectly for years by rendering the animations, element locations etc before things render to the browser, so there is no blink. So the solution for us to avoid these warnings is to implement isomorphicLayoutEffect like switches.

It seems there a plenty of valid use cases for useLayoutEffect to warrant finding a more subtle / off-build way to advise developers of when best to use it. No?

just-boris commented 2 years ago

Currently the warning does not work for the community

  1. use-isomorphic-layout-effect npm package exists for the sole purpose of hiding this warning and it has 2.6 M weekly downloads, 20% of total react downloads. So, every 5th project includes this dependency.
  2. There is also a whole lot of custom versions of this hack (https://github.com/facebook/react/issues/14927#issuecomment-466815232) (github search)
  3. The most popular answer on stackoverflow also suggests to circumvent the warning by swapping the hook

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?

Kjaer commented 1 year ago

Currently the warning does not work for the community

  1. use-isomorphic-layout-effect npm package exists for the sole purpose of hiding this warning and it has 2.6 M weekly downloads, 20% of total react downloads. So, every 5th project includes this dependency.
  2. There is also a whole lot of custom versions of this hack (useLayoutEffect in ssr #14927 (comment)) (github search)
  3. The most popular answer on stackoverflow also suggests to circumvent the warning by swapping the hook

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 useEffect and useLayoutEffect based on the availability of document. https://github.com/Andarist/use-isomorphic-layout-effect/blob/main/src/index.ts

so Abramov's gist more valid than the isomorphic package. https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85