Skip to content
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

Closed
perrin4869 opened this issue Mar 1, 2019 · 41 comments
Closed

useState not bailing out when state does not change #14994

perrin4869 opened this issue Mar 1, 2019 · 41 comments

Comments

@perrin4869
Copy link

perrin4869 commented Mar 1, 2019

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?

@gaearon
Copy link
Collaborator

gaearon commented Mar 1, 2019

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

@gaearon gaearon closed this as completed Mar 1, 2019
@perrin4869
Copy link
Author

@gaearon Thanks for your response!
I elavorated on my first example here: https://codesandbox.io/s/84qo167659
I was indeed thinking about the use-case where you have a previous value, only instead of storing the previous value inside the state, I was storing it using usePrevious hook. The problem with that approach is that the effect inside usePrevious never has a chance to fire, updating the previous value, and stopping the infinite renders... thus requiring a check before firing setState to check if the state actually changes, which is not very intuitive in my opinion... In the specific use-case where state does not change, maybe it shouldn't re-run the render phase?

@gaearon
Copy link
Collaborator

gaearon commented Mar 2, 2019

Let’s keep it open so I remember to look into it and reply.

@gaearon gaearon reopened this Mar 2, 2019
@perrin4869
Copy link
Author

Thanks for looking into this!
Maybe another solution could be to introduce an "official" usePrevious hook which does not rely on useEffect but updates itself using React internals, even between synchronous render calls

@joeljeske
Copy link

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 === same value will bail out of re-rendering.

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.

@angelgarpi
Copy link

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

@gaearon
Copy link
Collaborator

gaearon commented Mar 14, 2019

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.

@gaearon
Copy link
Collaborator

gaearon commented Mar 14, 2019

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?

@angelgarpi
Copy link

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)

@gaearon
Copy link
Collaborator

gaearon commented Mar 14, 2019

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.

@lokhmakov
Copy link

lokhmakov commented Mar 17, 2019

function BatchFunction() {
  const [value, valueSet] = React.useState(0)

  const onClick = React.useCallback(() => {
    valueSet(1)
  }, [])

  console.log(`BatchFunction`, `RENDER`, { value })

  return (
    <div onClick={onClick}>
      BatchFunction
    </div>
  )
}
  1. First onClick call rerender for value changed from 0 to 1.
  2. Second onClick call rerender for value that not changed, withot rerender childrens.

@jddxf
Copy link
Contributor

jddxf commented Mar 18, 2019

I think the main issue here is that the usePrevious hook given in docs didn't take render phase updates into account. It doesn't update the previous value until after render phase. But in this case, we need to read the latest previous prop immediately after an update is scheduled during render phase.

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

@gaearon this issue cause this problem.
If we call a setState with no change new state in a async call back, we can't get a valid previous value in a async callback to decide whether to call the setState. Because if we use a callback version of setState to get the previous value, the render always trigger. However if setState does not trigger render like dependents of useEffect and other hooks, we still can force updating by using setState({}) or setState(preValue => preValue + 1) or setState(preValue => !preValue).

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

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.

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

example
This is the example.
click async button first, and immediate the sync button. Randomly, console will output this:

We got a random next counter with 2. If the next counter is equal to current, we shouldn't call the setCounter, But how can we know it? 
The real pre counter is 2, And the counter has not changed yet. But we still call the setCounter, because we have not a way to detect it. 

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

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 :-)

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

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

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

  1. you click the async button. it will delay a second and relay on the last counter send to the callback to decide whether to call setState.
  2. after you click the async button and before a second. you click the sync button. This will cause counter become a new random counter which is 1 or 2. At this moment, The setState in the async callback is not triggered. However the counter in the async callback is invalid now. Because you have replaced it in the last sync callback. The only way to get a valid counter is to use the callback version setState like in the example. But I do not want to call it without knowing whether the new counter is equal to the previous one.

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

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.

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

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.
The async callback will last long time, the state may be changed outside. So the previous state in the async callback is not reliable, the only way to get the reliable current state is use setState(preState => XXX), That's what I'm trying to avoid

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

@gaearon I have updated the example
I added a comparison example which using class component. In the class component, I always can get the current state by using this.state, this is impossible in function component, this is why I think the useState hook should deal with the comparision of state itself

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

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?

@vipcxj
Copy link

vipcxj commented Mar 18, 2019

@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 useEffect). Too many render in the current level may be expensive. So bailing out of rendering child subtree may be not enough to me. On the other hand, too many render is hard to debug. If I put a breakpoint in the render method, it always stop in useless situation.

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2019

You can always split a component in two and do expensive calculation in the inner one.

@rscharfer
Copy link

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 useState does not cause a re-render. In the second one (fork), it causes a re-render.

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
https://codesandbox.io/s/0o1917z8ln

@privateOmega
Copy link

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

@vipcxj
Copy link

vipcxj commented May 10, 2019

@privateOmega according to @gaearon , the first one is the buggy one. setState cause render being called even when the arguments of setSate not change. However the children component will not rerender when the arguments of setSate not change. I think it is so weird, but it is the designed behaviour.

@lolzivkovic
Copy link

@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

@mctep
Copy link

mctep commented Oct 24, 2019

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

@acutmore
Copy link

@mctep That custom hook doesn't look concurrent-mode safe. An in-progress render could call updateState which would update the stateRef, but that render might be thrown away, leaving the stateRef out of sync with state.

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

@acutmore
Copy link

acutmore commented Apr 23, 2020

Maybe the docs should be updated to make the behaviour super clear?

The 'Functional Updates' part says:

If your update function returns the exact same value as the current state, the subsequent rerender will be skipped completely

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.

@behnammodi
Copy link
Contributor

@gaearon Why we can't prevent to re-render? #18719

@behnammodi
Copy link
Contributor

@mctep @accordeiro Thanks for a solution but we should know about this issue

@acutmore
Copy link

acutmore commented Apr 23, 2020

Looking through the implementation, useState is implemented using useReducer, and reducers are run during a render because the function might be defined inline and access the values returned by earlier hooks. e.g.:

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 setState also runs as part of a new render, not before.

I am presuming that setState(currentState) does not bail out so that there are no observable differences between setState(currentState) and setState(() => currentState) or when changing a useState to a useReducer. The component will always render the same number of times. i.e. It is might be possible to change setState to bail out earlier, but then there would be subtle differences between useState and useReducer.

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:

3. pre-useState 
5. post-useState 
1. pre-SetState 
2. post-SetState 
3. pre-useState 
4. setState 
5. post-useState 

https://codesandbox.io/s/patient-darkness-exvkp?file=/src/App.js

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 23, 2020

I am presuming that setState(currentState) does not bail out so that there are no observable differences between setState(currentState) and setState(() => currentState) or when changing a useState to a useReducer. The component will always render the same number of times

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 setState signature you could still end up with a different number of renders in concurrent mode.

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 useMemo. That will not run again on a render that was triggered by setState(currentState). The same applies to useEffect: https://codesandbox.io/s/why-render-doesnt-matter-zrqpb

@acutmore
Copy link

acutmore commented Apr 24, 2020

stop worrying about render counts until you identified a performance bottleneck

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.

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 24, 2020

@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 setState(prevState). See https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops

@laverdet
Copy link
Contributor

@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 />;
}

@acutmore
Copy link

acutmore commented Apr 24, 2020

@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

@laverdet
Copy link
Contributor

Sure but now we're back to an issue of efficiency instead of an issue of correctness.

@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

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 useMemos to the parts that are expensive. React does guarantee that if a bailout happens, it will not go updating child components. Even though it might in some cases re-run the component function itself.

I don't think there is something actionable here, so I think we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests