React: Warn when calling dispatch() from useEffect() cleanup function on unmounting

[merongmerongmerong]

Do you want to request a feature or report a bug?

bug

What is the current behavior?

action dispatched in return callback of useEffect seem to not work

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:

https://codesandbox.io/s/5yqmo128v4

only foo -> baz is logged

import React, { useState, useEffect, useReducer } from "react";
import ReactDOM from "react-dom";

import "./styles.css";

function reducer(state, action) {
  console.log("bar", action); // not logged
  // debugger
  return state;
}

function Foo({ value }) {
  const [state, dispatch] = useReducer(reducer, {});

  useEffect(
    () => {
      return () => {
        console.log("foo");
        // debugger
        dispatch({ type: "foo" });
        // debugger
        console.log("baz");
      };
    },
    [state, value]
  );

  return <p>{value}</p>;
}

function App() {
  const [value, set] = useState(0);

  return (
    <div className="App">
      <button onClick={() => set(value + 1)}>INC</button>
      {value % 2 ? <Foo value={value} /> : null}
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

What is the expected behavior?
bar is logged in console
(foo -> baz -> baraction)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

react: "16.7.0-alpha.2",
react-dom: "16.7.0-alpha.2"

[aweary]

@merongmerongmerong what exactly would you expect to happen here? In this example the Foo component being unmounted when the effect's cleanup funciton is called. You can't update state for an unmounted component, so this behavior seems expected.

If you have an effect that gets cleaned up in a component that isn't being unmounted it calls the reducer as expected: https://codesandbox.io/s/vqr9olv4o7

In this case the behavior seems correct, but maybe it would be good to warn here.

[merongmerongmerong]

@aweary
thanks for your reply, I was trying to manage blob data using reducer(calling createObjectURL, revokeObjectURL). I expected the cleanup function would dispatch action which calls revokeObjectURL.

I came to think of it now, I should not use reducer in this case. but, It is strange that dispach(action) is called but nothing is happened

[gaearon]

Reducers should be pure — so you shouldn't perform side effects like revokeObjectURL there.

dispatch doesn't do anything when unmounting, just like setState doesn't do anything. Their only purpose is to re-render, but when unmounting it's too late for rendering.

It is strange that dispach(action) is called but nothing is happened

We should probably add a warning. I'll rename this issue to track that.

[ThierryCols]

Hi @gaearon,

Looks doable, can I try this one?

[gaearon]

Sure

[ThierryCols]

Damn, totally forgot I'm actually leaving for two weeks. I'll be more than happy to handle this when I'm back but if anybody feels like doing it I totally understand. 😢

[tijwelch]

@gaearon It seems the reported issue of calling dispatch() from useEffect() cleanup function and not seeing "bar" logged to the console (because the reducer function was not evaluated on an unmounted component) is no longer the case on master. The same example logs "foo", "bar", "baz" to the console on master.

DispatchAction now eagerly calls the reducer here https://github.com/facebook/react/blob/c21c41ecfad46de0a718d059374e48d13cf08ced/packages/react-reconciler/src/ReactFiberHooks.js#L1092

In a scenario like above where dispatch() was called from useEffect cleanup function, eagerState may not equal currentState and an extra scheduleWork is called (even though the component is unmounting).

This is my first dive into the React codebase so I may be misunderstanding this, but wanted to check in after doing some digging and make sure adding a warning is enough, or if the fact that the cleanup function is actually scheduling work changes this (or none of the above 🙂)

[gaearon]

@tijwelch I think the actionable thing here is to add a test to ReactHooks-test so that we can confirm if the behavior is intentional and don't regress on it. Wanna do it?

[tijwelch]

Sure. Will try this weekend.

[tijwelch]

Hi @gaearon. I submitted a PR with this test.

When this issue was created, calling dispatch from the useEffect cleanup function did not call the reducer. As described in comments above, this seemed ok because the component was unmounting. In the current version of React, the reducer does get called in this scenario. The test in my PR confirms this.

[tijwelch]

As @acdlite explained here https://github.com/facebook/react/pull/14809#issuecomment-462493837, it's not ideal that the reducer gets called for a component that is being unmounted, but due to the eagerReducer optimization, this is currently happening.

Would be curious to see how you think about this kind of thing. Does this warrant a warning when calling the reducer on an unmounting component? Or should the eager reducer optimization have a check to avoid calling the reducer if the component is unmounted? Or maybe the current state is fine.

I realize this might be low on the hooks priority list but it's helping me understand the internals (and how decisions are made)

[gaearon]

If we know for sure the component is being unmounted it seems wasteful to call the reducer. So maybe you can find a fix that prevents that from happening?

[bySabi]

@gaearon can we have a useReducer version without the Warn or can tell me how to create one?.
I some uses cases is desirable do nothing on dispatch calls. I just trying to avoid the useEffect cleanup cause I don't need make this check in a custom hook and because useEffect is a little heavy and slow down my code 3x

[bySabi]

Something is wrong with current useEffect implementation.

This is my case: rendering 5000 component like this:

 const Comp = () => {
     useState();
     useEffect(()=>{}, []);
     return null;
}

The line useEffect(()=>{}, []); that not do nothing slow down rendering time 3X compare to the same Comp without useEffect.

Testing Comp with useLayoutEffect(()=>{}, []) it is working fine, it does not have any performance penalty.

I do 4 rounds of render 5000 components and the problem is only on the first round.