Bug: uSES with synchronous updating a component causes a warning
Nodge opened this issue · 6 comments
React version: 18.2.0
The case:
- We have a global state (e.g. Redux) and are using
useSyncExternalStoreto subscribe a React component to the state. - We want to synchronously re-render the component by dispatching an update to the state, for example, when a prop changes.
In this case, React logs a warning when we update the store:
Warning: Cannot update a component (`GlobalState`) while rendering a different component (`GlobalState`).
To locate the bad setState() call inside `GlobalState`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
Here is a simplified example to better illustrate the case:
function App(props) {
const state = useSyncExternalStore(store.subscribe, store.getState);
if (isUpdateNeeded(props)) {
// re-render the component with the updated state
store.dispatch(update());
}
}Strange Behavior 1:
Actually, if there are only two sequential renders (one re-render), there is no warning:
function App(props) {
const state = useSyncExternalStore(store.subscribe, store.getState);
if (isUpdateNeeded(props)) {
// no warning
store.dispatch(firstUpdate());
}
}However, if we have three or more sequential renders, React logs a warning:
function App(props) {
const state = useSyncExternalStore(store.subscribe, store.getState);
if (isFirstUpdateNeeded(props)) {
// no warning
store.dispatch(update());
}
if (isSecondUpdateNeeded(props)) {
// react logs a warning
store.dispatch(update());
}
}I know the simplified example looks strange (why do we need to re-render multiple times, right?). But the second update might be necessary when the first update triggers additional logic. In our real-world case, the first update occurs due to user interaction, and the second update occurs because a conditional react-query hook sets isLoading to the store.
Strange behavior 2:
It is possible to fix the issue by adding local state and updating both global and local state simultaneously.
function App(props) {
const state = useSyncExternalStore(store.subscribe, store.getState);
const [_, rerender] = useReducer(() => ({}), {});
if (isFirstUpdateNeeded(props)) {
rerender();
// no warning
store.dispatch(update());
}
if (isSecondUpdateNeeded(props)) {
rerender();
// no warning this time!
store.dispatch(update());
}
}Steps To Reproduce
- Open the codesandbox example: https://jlgcck-5173.csb.app/
- Open the browser console
- The warning is there
Link to code example: https://codesandbox.io/p/sandbox/react-uses-sync-update-jlgcck?file=/src/App.tsx
The current behavior
React logs a warning, but everything else seems to work correctly.
The warning message that states "the bad setState() call" is a little confusing since there is no setState call in a component.
The expected behavior
React does not produce a warning.
@Nodge : are you manually calling useSyncExternalStore?
Is there a reason you're not using React-Redux for that? (It already relies on uSES internally)
Also, in general it looks like the pattern you're using definitely breaks the rules of React rendering.
You can call setState() while rendering, in very specific cases, to force an immediate re-render:
But you can't trigger renders for other components while rendering, and you shouldn't be dispatching global actions while rendering either.
I simplified the example code to only include the necessary parts that reproduce the problem. But in my actual project, I’m using slightly modified versions of React-Redux and RTKQ, to integrate it with Suspense for data fetching. Basically, during the render phase, the only action I dispatch is the RTKQ endpoint initiate which gives me a promise for Suspense integration. I understand that dispatching global actions on the render phase is an anti-pattern, and I should find a way to rework it. I agree that the warning is not a bug in React.
The only thing left to address in this issue is the warning logging behavior. Should React log the warning more consistently? (see "strange behavior 1,2" in the description)
@Nodge huh. Getting off-topic, but: "modified versions of React-Redux / RTKQ for Suspense"?
Could you file a discussion thread over in the Redux Toolkit repo to discuss what you're trying to do? We don't have an official Suspense solution yet, largely because too many things with React have been changing and it's unclear how we're supposed to support things. But, it would be nice to see what kinds of use cases people have and what you're trying to do to play with those.
(Not sure on what's up with the warning behavior, tbh.)
I seem to have the same problem. Everything renders fine, but the warning frightens me a bit. Here's a simplified playground:
The store looks like this:
/*
Here we store <select> options in the form of:
{
[name]: {
loading: bool // if the options are still loading from some backend
options: [...] // list of string values
}
}
*/
let snapshot = {};
// Array of listeners of the store
let listeners = [];
const Store = {
setValuesLoading(name, silent = false) {
snapshot = { ...snapshot, [name]: { loading: true, options: [] } };
if (!silent) {
notify();
}
},
setOptions(name, options) {
snapshot = { ...snapshot, [name]: { loading: false, options } };
notify();
},
subscribe(listener) {
listeners = [...listeners, listener];
return () => {
listeners = listeners.filter(l => !Object.is(l, listener));
};
},
getSnapshot() {
return snapshot;
}
};
// Trivial listener notifier.
const notify = () => {
for (let listener of listeners) {
listener();
}
};To use it, I've written a custom hook, that triggers an "API" to load data if necessary:
export function useOptions(name) {
let store = useSyncExternalStore(Store.subscribe, Store.getSnapshot);
// If data is in cache – return it
if (store[name]) {
return store[name];
}
// Updating the store, do not notify anyone (we are within render currently)
Store.setValuesLoading(name, false);
// loading data
loadOptions(name).then(options => Store.setOptions(name, options));
return { loading: true, options: [] };
}
// Imitate API fetch
function loadOptions(name) {
return new Promise(resolve => {
setTimeout(() => resolve(["one", "two", "three"]), 1000);
});
}Usage is trivial:
import { useOptions } from './store.jsx';
export function Select({ name }) {
let { options } = useOptions(name);
return (
<select>
{options.map(opt => (
<option value={opt} key={opt}>
{opt}
</option>
))}
</select>
);
}The problem somehow happens ONLY if I dynamically add a Select. Multiple Select's, rendered statically, don't cause any warnings.
import React, { useState } from 'react';
import { Select } from './Select'
export function App(props) {
let [show, setShow] = useState(false)
return (
<div className='App'>
<Select name="one" />
<Select name="two" />
<button onClick={() => setShow(true)}>CLICK ME TO TRIGGER A WARNING IN CONSOLE</button>
{show ? <Select name="three" /> : null}
</div>
);
}React 18.2.0
Fwiw/afaiu this also easily/regularly happens with any library that is trying to bring observables/signals to React, i.e. Mobx or Legend State, and wants to cross the "React props <=> observables/signal world" divide.
For example, if I have two components:
- Parent component
<Author name=bob />and - Child component
<AuthorTitle author=authorSignal />
Where the Author component internally initializes an observable/signal, and then passes the authorSignal around to other components.
If the Author is passed a new name prop, and naively does:
function Author({ name }) {
// create a stable observable on 1st render
const author = useMemo(() => observable({ name });
// Keep the observable updated on Nth render
author.name = name;
// render name + any components using the new name
return <div>{name} <AuthorTitle author={author} /></div>
});Then the author.name = name line will cause this warning that the AuthorTitle reactive infra (i.e. a useComputed or observer HOC, whether either useState/tick-based or useSES-based) registered a change for AuthorTitle to re-render.
Granted, the correct behavior is to shunt all "maybe updating an observable" operations through useEffects:
function Author({ name }) {
// create a stable observable
const author = useMemo(() => observable({ name });
// Keep the observable updated
useEffect(() => author.name = name, [name]);
// render the potentially-updated name
return <div>{name} <AuthorTitle author={author} /></div>
});But that is tedious, boilerplatey, and can also cause double renders if Author is itself using a calculated value from the observable:
function Author({ name }) {
// create a stable observable
const author = useMemo(() => observable({
name,
// use some reactive/computed business logic
initials: () => getInitials(this.name)
});
// Keep the observable updated
useEffect(() => author.name = name, [name]);
// When props.name changes, we'll do one render with new name +
// old initials (not updated yet) and then after useEffect has
// updated the observable, a 2nd render that has the non-torn
// new name/new initials
return <div>{name} {author.initials} <AuthorTitle author={author} /></div>
});This update-observables-via-useEffect pales particularly to the props-based way of telling AuthorTitle to re-render, which is just "pass the prop via JSX":
return <div>{name} <AuthorTitle authorName={name} /></div>So, using props/JSX to tell AuthorTitle to render during my render is fine, but I'm unable to tell AuthorTitle to render during my render if I'm using an observable.
Granted, I get it, in React props are first-class and observables are not, so I shouldn't be surprised that observables have worse ergonomics.
But imo this dichotomy is really what I expected useSES to fix, b/c fundamentally it focus on components needing to "reactively" re-render based on "other things happening", i.e. external syncs, typically AJAX updates, 3rd-party-stores whatever.
Because, for those of us pretending to write Solid in React :-D, the props of components like Author (that cross the props/observable divide), if you squint, are essentially their own "external source of change" for components that are consuming the observable.
Hence, I think this warning should be removed for useSES.