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
useState not bailing out when state does not change #14994
Comments
I think at the time we decided that we don't actually know if it's safe to bail out in all cases until we try render again. The "bailout" here means that it doesn't go ahead to render children. But re-running the same function might be necessary (for example, if a reducer is inline and we don't know if we bail out until we re-run the reducer on next render). So for consistency we always re-run it on updates during the render phase. I don't think I agree with you the pattern in https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops is broken. It's different from your code sample which doesn't track |
@gaearon Thanks for your response! |
Let’s keep it open so I remember to look into it and reply. |
Thanks for looking into this! |
I believe I am seeing the same issue, but with slightly different reproduction. I agree with @perrin4869 that the docs seem to be clear that updating the state to the exact Below is an example jest spec that reproduces the issue import { createElement, useCallback, useEffect, useState } from 'react';
import { fireEvent, render } from 'react-testing-library';
// This component is contrived but shows the behavior
function ComponentSettingState({
results
}: {
results: (value: number) => void;
}) {
const [state, setState] = useState(0);
// Call with our current state
results(state);
// A never changing callback
const handleClick = useCallback(() => setState(1), []);
// Contrived effect that sets state to the same current value.
useEffect(
() => setState((prevState) => prevState) // This still triggers a re-render after the click occurs
/* run after ever render */
);
return <button onClick={handleClick}>click</button>;
}
test('useState setting is re-rendering even if state does not change', () => {
const results = jest.fn();
const { getByText } = render(<ComponentSettingState results={results} />);
expect(results).toHaveBeenCalledTimes(1);
expect(results).toHaveBeenNthCalledWith(1, 0); // Only rendered once
fireEvent.click(getByText('click'));
expect(results).toHaveBeenCalledTimes(2); // ERROR! it was called 3 times. It rendered twice here instead of the expected once
expect(results).toHaveBeenNthCalledWith(2, 1);
// The following makes the test pass
// expect(results).toHaveBeenCalledTimes(3);
// expect(results).toHaveBeenNthCalledWith(2, 1);
// expect(results).toHaveBeenNthCalledWith(3, 1);
}); I am able to do manual strict equality checks on the state before calling setState to bypass the issue. But it seems to contradict the docs. |
I want to subscribe this issue/bug, seems like React takes care of updates in theory but not in practice, React will redo calculations even if the n state and n-1 state are the same. useReducer dispatch function also triggers |
Just to clarify — you’re worrying about this because of some perf issue right? It shouldn’t affect the end result. Or does it? I haven’t looked into this report yet. |
Also to be clear the bailout isn’t supposed to prevent calling the render. It prevents rendering child components. So you shouldn’t worry about this component’s own render running again. Does that clarify things? |
Yes, it is a perf issue from my point of view, render result is the same cause the state is the same. Then I would like to ask how can we tell React that this component does not need a render at all if state does not change? Right now, I am not calling a set function from useState or useReducer if I know it is not going to change the state (using previousState and conditions) |
What exactly are you doing inside your render that is so expensive? I'd like to understand the practical issue you're running into. Usually bailing out of rendering child subtree is enough. |
|
I think the main issue here is that the |
@gaearon this issue cause this problem. |
If you have a problem, please provide a concrete runnable example showing the problem. We can't help if you provide no runnable code example (such as a sandbox). Thanks. |
example
|
Regarding example in https://codesandbox.io/s/84qo167659, the fix to this is to use render phase update to keep track of the previous value, just like in the documentation: https://codesandbox.io/s/v34o9rx5m5 I'm not sure I understand why you're trying to do things differently since I already linked to this doc section in an earlier comment. Maybe I'm missing something :-) |
@vipcxj I don't understand what problem this example is demonstrating. Can you rephrase this example from a hypothetical one with counters to a more concrete one where it's actually causing problems in your app? |
|
What is the actual problem you're solving in your app? I imagine it's not actually a counter app. Explaining it from user perspective might help. |
I have a big state in a function component. And I have a effect with a async callback. There are a lot of setState in it. And lots of them may not change the state. But they are async, so they will not be batched up. I have no way to skip rerender if the state is not really changed. |
@gaearon I have updated the example |
I added a child component with render counter to each version: https://codesandbox.io/s/oxz2l25yr5 Can you please explain what sequence of steps I need to do in order to show that function children re-render more often than class children? |
@gaearon I want to build a very dynamic dom tree depend on a remote config. So I do a complicate calculation of dom tree depend on the remote config in the render method (of course, the network call is in the |
You can always split a component in two and do expensive calculation in the inner one. |
I have two sandboxes with essentially exactly the same code. The only difference is the second one is a fork from a @gaearon sandbox he included in https://overreacted.io/a-complete-guide-to-useeffect/, but his original code has been replaced. What is super weird is that in one sandbox (first one), updating the state with the same value using the function returned from Frustrating because I am trying to get to the bottom of the issue of whether or not updating the state with the same value causes a rerender, and I have essentially the same code doing different things. Maybe because they are using different versions of React? Here are the two sandboxes: https://codesandbox.io/s/r0vz0n13mm |
@rscharfer Seems like in the second one, you've used an alpha version of 16.8.0 which might be buggy. Always go with final versions just to be safe. |
@privateOmega according to @gaearon , the first one is the buggy one. |
@gaearon Do you know if there is solution? Same here happens to me, it re-renders every time even if state is not changing. Thanks in advance buddy |
About preventing re-renders when state does not change. I use a custom hook like this: function areStrictEqual(a, b) {
return a === b;
}
function useStateUpdate(initialState, areEqual = areStrictEqual) {
const [state, setState] = React.useState(initialState);
const stateRef = React.useRef(state);
const areEqualRef = React.useRef(areEqual);
areEqualRef.current = areEqual;
const updateState = React.useCallback(stateOrReducer => {
const nextState =
typeof stateOrReducer === "function"
? stateOrReducer(stateRef.current)
: stateOrReducer;
if (!areEqualRef.current(stateRef.current, nextState)) {
stateRef.current = nextState;
setState(nextState);
}
}, []);
return [state, updateState];
} Example usage: https://codesandbox.io/s/misty-frost-yiogh |
@mctep That custom hook doesn't look concurrent-mode safe. An in-progress render could call I think the ref update needs to happen in the commit phase function useStateUpdate(initialState) {
const [state, setState] = React.useState(initialState);
const stateRef = React.useRef(state);
React.useLayoutEffect(() => {
stateRef.current = state;
});
const updateState = React.useCallback(stateOrReducer => {
const nextState =
typeof stateOrReducer === "function"
? stateOrReducer(stateRef.current)
: stateOrReducer;
if (stateRef.current !== nextState) {
setState(nextState);
}
}, []);
return [state, updateState];
} https://codesandbox.io/s/elated-cdn-pk37w?file=/src/index.js |
Maybe the docs should be updated to make the behaviour super clear? The 'Functional Updates' part says:
https://reactjs.org/docs/hooks-reference.html#functional-updates Need to scroll a little further down to read the longer 'Bailing out of a state update' section which explains that only children-renders and effects are not run. |
@mctep @accordeiro Thanks for a solution but we should know about this issue |
Looking through the implementation, function Component() {
const value = useHookThatReturnsValue();
const [state, dispatch] = useReducer((state, action) => {
// inline reducer uses 'value' so is run as part of the render
// so it is sure to access the most up-to-date values
return updateState(state, action, value);
)};
return <Child state={state} dispatch={dispatch} />
} This means that I am presuming that function Component() {
console.log('3. pre-useState');
const [state, setState] = React.useState(0);
console.log('5. post-useState');
if (state === 0) {
console.log('1. pre-SetState');
setState(() => {
// delayed until the Component is rendered again
console.log('4. setState');
return 1;
});
console.log('2. post-SetState');
}
return <div />;
} logs:
https://codesandbox.io/s/patient-darkness-exvkp?file=/src/App.js |
I don't think this is a good justification for the current behavior. You shouldn't rely on the number of renders to begin with. Even if you use the same I think the takeaway is that you should stop worrying about render counts until you identified a performance bottleneck. If your render function does expensive work then you can memoize that expensive work with |
For me it's not performance but infinite renders. It would be handy if I didn't have to worry about checking the current state before resetting it to something. function Component(props) {
const [state, setState] = useState(0);
if (someCondition(props)) {
setState(0); // Error! Too Many ReRenders
}
return <div />;
} Can fix here without issue, but in a more complex component, with custom hooks passing state around, this is slightly harder to achieve. |
@acutmore That's a really good example why this is an issue. If we'd fix your particular issue, React wouldn't need to bailout since the usercode is already responsible for that. I guess for now we need to advise to check if the state wouldn't change when implementing the "get-derived-state-from-props"-pattern. Edit: In all fairness: The docs do document it with a guard against |
@acutmore Calling setState directly from a component render function is a footgun and should probably be an explicit error because sometimes it works and sometimes it doesn't. Your state change needs to be extracted to an effect hook. function Component(props) {
const [state, setState] = useState(0);
const condition = someCondition(props);
useEffect(() => {
if (condition) {
setState(0); // This is fine
}
}, [ condition ]);
return <div />;
} |
@laverdet absolutely that would avoid the too many re-render errors. Though now the child effects are potentially being run twice. (child effects run before parent effects) https://codesandbox.io/s/busy-burnell-by65v?file=/src/App.js |
Sure but now we're back to an issue of efficiency instead of an issue of correctness. |
React does not offer a strong guarantee about when it invokes render functions. Render functions are assumed to be pure and there should be absolutely no difference for correctness regardless of how many times they're called. If calling it an extra time causes a bug, it's a problem with the code that needs to be fixed. From the performance point of view, the cost of re-running a single function is usually negligible. If it's not, you should add some I don't think there is something actionable here, so I think we can close this. |
Do you want to request a feature or report a bug?
Bug
What is the current behavior?
As demonstrated in this codesandbox, trying to implement a pattern similar to the one discussed in https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops results in an infinite loop, even if the value of the state does not change. This seems like a bug, because, as documented here,
If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
What is the expected behavior?
Since the state does not change, bail out on the re-render.
This can be worked around by adding a check before
setState
to check if the state has changed before calling the function.Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
The text was updated successfully, but these errors were encountered: