facebook / react

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

Bug: React DOM Server triggers setStates only on the top component #25318

Closed davesnx closed 6 months ago

davesnx commented 2 years ago

React version: Tested in 16 and 18.

While trying to SSR (both toStaticMarkup and toString) an app in React with a node server found suspicious behaviour.

State updates run if somehow are present during the component runtime, but ignored when those updates are down the react tree.

Steps To Reproduce

Made a minimum repro with node under code-sandbox. To see the output (sometimes code-sandbox output doesn't render on the side panel) Open the Terminal > Press "+" (in the top right of the Terminal) > run "node index.js"

https://codesandbox.io/s/react-dom-server-demo-0b52zt?file=/index.js

Within other implementations of SSR in React-like, such as preact https://github.com/preactjs/preact-render-to-string/tree/master/test, this "issue" isn't present.

After digging a bit, I found the code on server/node_modules/react-dom/cjs/react-dom-server.node.development where useReducer checks if it's currently rerendering and process the hook.

I have read it once or twice on Twitter/new documentation (can't really point where exactly) where It's a totally valid approach to run effects with setState within the body of the component. I'm wondering If this behaviour is expected within now, and with future releases and how it relates to Suspense with SSR (which I know is disabled, but It might be supported in the future?)

The current behavior

The demo shows in the first example that the "app" component is rendered twice since there's a setState call, while the following example only renders one.

The expected behavior

I'm not entirely sure what's the proper behaviour in this case. From the perspective of this issue, might make sense to have one "unified" behaviour where both updaters are either not called, or called. But it seems a lot of work to rerender the entire React tree to check if those updates happen on the runtime of some components.

gaearon commented 2 years ago

Re-rendering in principle doesn’t make sense in SSR. The goal is to do everything in a single pass. The thing you read about is about adjusting state in response to another update but there are no real updates on the server because there is no user interaction.

We plan to add a warning (deprecate) calling setState during initial render, both on the server and on the client.

gaearon commented 2 years ago

So, to clarify — the existing behavior is inconsistent for backwards compat reasons. But we should deprecate adjusting it during initial render (which includes SSR), which in turn will let us stop supporting this in SSR altogether.

davesnx commented 2 years ago

Understood, make sense.

The thing you read about is about adjusting state in response to another update but there are no real updates on the server because there is no user interaction.

In this case, components can contain any side-effect (like fetch) on the component body and then set the state right away, that would still derive this state from another, right? This will be the breaking change I suppose.

Those warnings are part of react-dom/server and have nothing to do with React.StrictMode?

Thanks for the fast answer

gaearon commented 2 years ago

In this case, components can contain any side-effect (like fetch) on the component body and then set the state right away, that would still derive this state from another, right? This will be the breaking change I suppose.

Not sure I understand. fetch() is asynchronous, right? How would it set state right away? But yes, setting state in general during the initial render would be deprecated. It's never needed by definition — if the state is available during initial render, then there's no point in setting it — make it the initial state.

Those warnings are part of react-dom/server and have nothing to do with React.StrictMode?

They would be a part of react-dom/server and probably react-dom on the client too.

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] commented 6 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!