reactjs/rfcs

[Hooks] Abortable useEffect

sag1v opened this issue · 7 comments

sag1v commented

When ever we use effect with a state update inside an async callback (or after an await) we probably want to check that we are not trying to update an unmounted component or worse, we are in a race condition and updating the state with the wrong data.

With hooks, this is fairly simple to achieve using a boolean and a closure:

useEffect(() => {
  let current = true;
  asyncOp(arg).then(res => {
    current && setState(res);
  });

  // cleanup
  return () => current = false
}, [arg]);

But doing that in each effect, every time we are dealing with a state update in an async operation can become tedious.

I played around and created an "abortable" effect, this effect is just a wrapper over useEffect and its providing a status indicator so the consumer can check if the effect was "cleaned":

function useAbortableEffect(effect, dependencies) {
  const status = {}; // mutable status object
  useEffect(() => {
    status.aborted = false;
    // pass the mutuable object to the effect callback
    // store the returned value for cleanup
    const cleanUpFn = effect(status);
    return () => {
      // mutate the object to signal the consumer this effect is cleaning up
      status.aborted = true;
      if (typeof cleanUpFn === "function") {
        // run the cleanup function
        cleanUpFn();
      }
    };
    // not the best way to pass dependencies, but what other options do we have
  }, [...dependencies]);
}

And the usage:

useAbortableEffect(status => {
  asyncOp(arg).then(res => {
    !status.aborted && setState(res);
  });
}, [arg]);

I wish that useEffect would provide this information out of the box instead of us writing it in "user land".

Shouldn't status and effect be included in the dependencies array? Also, since dependencies is not being used inside the side effect itself, it shouldn't be included.

sag1v commented

@courtneynguyen

Shouldn't status and effect be included in the dependencies array?

Well status is mutable so i'm not sure how effective it will be to include it as dependency, i think its the same as a ref.

As for effect, again this won't make much sense as it will be a new function reference on each cycle because most if not all the time we will pass effect as an inline function.

Also, since dependencies is not being used inside the side effect itself, it shouldn't be included.

We are basically wrapping useEffect here so we should keep the same API, if we won't use dependencies then the consumers won't be able to sync it with dependencies like they can with the builtin useEffect.

You probably wouldn't want status changing to rerun the effect, since the job will run a second time. This is a ref, where the callback that becomes executed can understand if it has since become aborted

nice idea

Flufd commented

I wrote this small library that does basically this a while ago. https://github.com/Flufd/use-current-effect
The implementation is almost exactly the same but the hook passes a boolean returning function to check the clean-up state instead of { status: boolean }
I do think it would work nicely in React by default though.

Hi, thanks for your suggestion. RFCs should be submitted as pull requests, not issues. I will close this issue but feel free to resubmit in the PR format.

sag1v commented

RFC is now opened in #202. We should continue further discussion in there.