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

Question about getting the latest state value in the concurrent mode #20924

Closed
hosseini44444 opened this issue Mar 3, 2021 · 10 comments
Closed

Comments

@hosseini44444
Copy link

React version: 17.0.1

Steps To Reproduce

  1. Enable strict mode for checking for possible issues in the future concurrent mode
  2. create the below component and run the code
import { useCallback, useState } from "react";

const Example = ({ onIncrement }) => {
  const [count, setCount] = useState(0);

  const incrementHandler = useCallback(() => {
    onIncrement(count, count + 1);  // Is count guaranteed to be the latest state here due to including count in the useCallback dependency array?
    setCount((count) => count + 1);
  }, [count, onIncrement]);

  return (
    <>
      <span>{count}</span>
      <button onClick={incrementHandler}>increment</button>
    </>
  );
};

const Parent = () => (
  <Example
    onIncrement={(currentCount, incrementedCount) =>
      alert(
        `count before incrementing: ${currentCount}, after increment: ${incrementedCount}`
      )
    }
  />
);

export default Parent;

The current behavior

In this simple example everything seems to be fine but in a more complicated situation full of event handlers that change the count or async callbacks that may change the count( like data fetching callbacks) the count value is not guaranteed to be the latest state and if I change the incrementHandler function like below:

const incrementHandler = useCallback(() => {
    setCount((count) => {
      onIncrement(count, count + 1);  
      return count + 1
    });
  }, [count, onIncrement]);

then the onIncrement will run twice in development while in strict mode and may run twice in production in concurrent mode according to documentation.
and If you suggest running the onIncrement in useEffect callback with count and onIncrement in effect's dependencies array how can I know that the onClick event of the increment button has caused the effect and not another event for example decrement or anything else.

you may say by setting another state that shows what is responsible for the effect, then I may need the previous state which unlike this example may be impossible to recalculate.

you may suggest using a ref for storing the previous state (count) then I will end up with one extra state or ref for storing what is responsible for the effect to run, one extra ref for storing the previous state, and a useEffect hook to run the onIncrement click event handler

The expected behavior

Providing a second callback argument to setState like in class Components that will run after this state update so we can save the current and next state and use it in the callback like below:

const incrementHandler = useCallback(() => {
    let prevCount, nextCount;
    setCount(
      (count) => {
        prevCount = count;
        nextCount = count + 1;
        return nextCount;
      },
      () => onIncrement(prevCount, nextCount)
    );
  }, [onIncrement]);

In my humble opinion, this doesn't collide with the async nature of setCount and can be implemented.

unlike belowgetState proposals that if it will be asynchronous it may not return the desired state. and if it will be synchronous it will not return the latest state too because setState is not executed yet.

wrong solution:

const [count, setCount, getCount] = useState(0);

  const incrementHandler = useCallback(() => {
    setCount((count) => count + 1);
    const currentCount = getCount();
    const nextCount = currentCount + 1;
    onIncrement(currentCount, nextCount)
  }, [onIncrement]);

or providing a third array to useCallback for accessing the latest state can not be implemented due to the same problem with getState and async nature of setState.

Please tell me if I'm missing something or I've misunderstood things.

If not, please tell me if there is a simple solution for this scenario or similar ones, or tell me the best practices for running a callback or event handler with the latest state.

Thank you!

@hosseini44444 hosseini44444 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 3, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Mar 3, 2021

I skimmed this, but I think this is key of the problem you're reporting:

const incrementHandler = () =>
  setCount((count) => {
    // Side effects aren't safe here!
    // State updater functions should be pure!
    onIncrement(count, count + 1);

    return count + 1;
  });

(I've removed useCallback from this section of code because it complicates the problem.)

The onIncrement prop is an external callback. Calling it is a "side effect" in React terminology and is not something you should from a state updater function (because those might be run more than once). This is something StrictMode does intentionally to help identify problems like this by making them always run more than once in DEV mode.

Fortunately the fix for this is tiny:

const incrementHandler = () => {
  const newCount = count + 1;

  // Event handlers can have side effects!
  // Calling onIncrement here even has an added benefit:
  // If onIncrement also updates state, the updates will get batched by React — which is faster!
  onIncrement(count, newCount);

  // You can also use the simpler updater form of just passing the new value in this case.
  setCount(newCount);
};

If you want or need to memoize incrementHandler, you can useCallback to do it. You'll just need to include count as a dependency. (React lint rules will point this out for you if you forget):

const incrementHandler = useCallback(() => {
  // Because count is part of the dependencies,
  // you have the latest value here (just like the example above).
  const newCount = count + 1;

  onIncrement(newCount);
  setCount(newCount);
}, [count, onIncrement]);

(Going to leave this issue open for a bit in case my answer above reflects a misunderstanding of the question you're asking.)

@bvaughn bvaughn added Type: Question and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Mar 3, 2021
@bvaughn bvaughn changed the title Bug: It seems impossible to get the latest state in the concurrent mode without a workaround Question about getting the latest state value in the concurrent mode Mar 3, 2021
@hosseini44444
Copy link
Author

hosseini44444 commented Mar 3, 2021

Thank you so much for answering my question!

The source of my confusion was articles that said react may postpone state updates due to heavy workload and that you shouldn't rely on the state value outside of setState callback.
After a lot of research and according to this answer, now I know that react may batch state changes but it always keeps the order.

If I've understood well, React will invoke the render phase after each state change batch, like event handlers in my case, and for sure it will invoke render phases in order but may choose to postpone them or not to commit them in concurrent mode. Am I right?

The other source of my confusion was the below quote from 'React docs':

The commit phase is usually very fast, but rendering can be slow. For this reason, the upcoming concurrent mode (which is not enabled by default yet) breaks the rendering work into pieces, pausing and resuming the work to avoid blocking the browser. This means that React may invoke render phase lifecycles more than once before committing, or it may invoke them without committing at all (because of an error or a higher priority interruption).

which I had misunderstood and I thought that React might choose to ignore invoking some render phase completely, but the reality is that react will invoke it but might choose not to commit it.

@gaearon
Copy link
Collaborator

gaearon commented Mar 3, 2021

@hosseini44444 Can you describe what your onIncrement function is doing in more detail? It's a bit hard to answer in abstract. A complete example of what you're trying to do would help a lot. Preferably, grounded in a real use case.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 3, 2021

The source of my confusion was articles that said react may postpone state updates due to heavy workload and that you shouldn't rely on the state value outside of setState callback.

In the scenario you posted above though, it seemed like you wanted to trigger some side effect (onIncrement) in response to an event (a user clicking a button). In that case, I think the appropriate time to trigger that side effect is when the user performs the action and the value at that time- whatever the current value was- is the right value to use.

@hosseini44444
Copy link
Author

@gaearon

@hosseini44444 Can you describe what your onIncrement function is doing in more detail? It's a bit hard to answer in abstract. A complete example of what you're trying to do would help a lot. Preferably, grounded in a real use case.

This is the code which I'm using in a slider component:

const nextHandler = useCallback(() => {
    setCurrentIndex((currentIndex) => {
      let nextIndex = currentIndex + 1;
      if (nextIndex >= slidesCount) nextIndex = infinite ? 0 : currentIndex;
      return nextIndex;
    });
  }, [infinite, slidesCount]);

The component has a lot of event handlers and one of them is onNextClick which accepts a callback that will be executed only after the user clicks the next button and I need to pass the currentIndex and nextIndex values to the onNextClick callback so he/she can benefit form these. there are a lot of events that may happen like backButtonClick, touch slide, clicking an indicator, clicking or sliding thumbs that will change the currentIndex too and most important is that the setIndex can be invoked as a method out of the Slider component, one other thing is that there are other handlers and callbacks like onSlideChange and slideChangeHandler that may be called concurrently with the onNextClick callback.
Do you think that it's fine to do this like this as @bvaughn has suggested?

const nextHandler = useCallback(() => {
    let nextIndex = currentIndex + 1;
    if (nextIndex >= slidesCount) nextIndex = infinite ? 0 : currentIndex;
    onNextClick && onNextClick(currentIndex, nextIndex);
    setCurrentIndex(nextIndex);
  }, [currentIndex, infinite, slidesCount, onNextClick]);

onNextClick is a callback that user may provide to the component as a prop.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 3, 2021

Libraries I've worked on (like react-window) have similar callback props (like onScroll) that often set state. For this reason, I think, calling this type of prop from an event handler is better than calling from an effect (the only other "valid" option) b'c updates get batched and overall this makes things faster. Calling from a state updater function is kind of like calling from a render method. It's a side effect that can definitely cause problems in certain circumstances.

@hosseini44444
Copy link
Author

hosseini44444 commented Mar 3, 2021

@bvaughn @gaearon

Consider a scenario in which the user has clicked a thumb that will change the currentIndex to the thumb's index but due to heavy workload react has decided to postpone the render and in the meanwhile, the user clicks the next button, the action of clicking the thumb will invoke the render phase then React may decide to postpone it or decide not to commit it due to another user interaction like clicking the next button or even typing the index in an input directly or reading it from the backend.

In this scenario, if react decides to invoke but not commit the first render phase which is an effect of the user clicking a thumb then the user will see the slide that is shown before clicking the thumb but after the first render phase the state changes to the index of the thumb which was clicked but will not see the result and then the second render phase will begin and will be committed then what the user will see on screen is 1st slide switching to 3rd slide but what he will get as currentIndex and nextIndex is 2nd slide's index and 3rd slide's index.

So What You See Is Not What You Get (WYSINWYG).

It may confuse or seem like a bug to the end-user in some scenarios, don't you agree?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 3, 2021

I think regardless of when React performs the update (soon vs deferring it) when the action was performed, ie when the user clicked, the count/index that was shown on the screen was the one that was closed over by the event handler that's getting called in your example code above. So it seems best to call the onIncrement callback or whatever at the time of the event/action.

As Dan said above though, it can be difficult talking about this stuff abstractly.

@gaearon
Copy link
Collaborator

gaearon commented Mar 3, 2021

I think one crucial nuance we're overlooking here is React will not actually do any of that. In particular, React does offer a guarantee that you will not get a render of the "next click" before the "previous click" has been flushed. This has already been a guarantee with Concurrent Mode, but we're making it even a stronger one — click events (and similar "intentional" user event like keyboard input) will always flush without interruption in a microtask. In practice, this means events for clicks and other intentional events will never get "ignored" as in this hypothetical scenario we're discussing. We'll include this in the documentation when it's stable.

@hosseini44444
Copy link
Author

hosseini44444 commented Mar 4, 2021

Thanks so much.
Batching event callbacks using a microtask is a smart move.
It really needs to be included in the docs, not everybody knows about how React works internally.
I'll close the issue now that everything is clear.

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

3 participants