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

Bug involving warnAboutUpdateOnUnmounted when using hooks #15303

Closed
jamesplease opened this issue Apr 3, 2019 · 3 comments
Closed

Bug involving warnAboutUpdateOnUnmounted when using hooks #15303

jamesplease opened this issue Apr 3, 2019 · 3 comments

Comments

@jamesplease
Copy link

jamesplease commented Apr 3, 2019

tl;dr

While using hooks, I have run into a number number of situations where particular arrangements of useContext, useState, and useReducer hooks lead to behaviors that I think might be a bug. But it might just also be my lack of understanding of how hooks work and/or hooks best practices.

I'll try to open issues here to track the problemos. Unfortunately, the smallest reproducible examples will likely always be complicated! This is the first one, which is about a warning that seems difficult to avoid.

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

Bug (possibly).

What is the current behavior?

Consider a child component that is conditionally rendered by its parent based on the parent's state. When the user clicks on the child, an asynchronous action is called that ultimately updates the parent's state, unmounting the child. (This asynchronous action represents a successful network request to delete the child.)

The child also happens to have its own state that it is keeping track of using useReducer. At the end of the asynchronous action, it checks to see if it has been unmounted or not. If it is not unmounted, then it dispatches an update. Otherwise, it doesn't.

The child "knows" when it unmounts using an "unmounted hook":

const isUnmountedRef = useRef(false);
  useEffect(() => {
    return () => {
      isUnmountedRef.current = true;
    };
  }, []);

So that's the setup. Onto the problem:

When the child is conditionally rendered "simply,", then you get no warning, because the unmount hook is called before the dispatch call. What I mean by "simply" is that when the child tells the parent to unmount it, it directly updates the state that causes it to unmount. There is no "chain" of hooks before that state gets updated.

However, if there is instead a "chain" of useEffect hooks that cause the child to unmount, then dispatch will be called before the unmount hook is called within the Child component., But due to the order of how the hooks are resolved, the reducer won't actually be called until after the component unmounts. React doesn't like this, and it warns you.

image

I think this is a problem because:

  1. there is no way for the child to know if it is conditionally mounted "simply" or not
  2. I've found that composing hooks naturally leads to "chains" of updates

Link to CodeSandbox: https://codesandbox.io/s/n34k2z321l

In the CodeSandbox, the "mirrored" state within Parent represents the chain of hooks, whereas renderChild represents the "simple" version.

Note 1: this example of "mirroring" the state unnecessarily might seem contrived, but this has taken me a couple of hours to simplify a significantly more complex library that creates this situation internally. The tl;dr is that I am writing a collection of independent hooks and composing them in a way that creates the "chain." I'd be happy to provide the source code of the lib if it would be useful.

Note 2: you must hit the in-page "reload" button for the warning to re-appear after it appears once, as I believe CodeSandbox is using HMR-like technology.

What is the expected behavior?

I didn't expect chaining hooks to cause this warning. I recognize that the warning says that it is a no-op, but there are a few problems with ignoring it:

  1. this for a library that I expect others to use, and they probably won't feel very confident in the library if it is warning out of the box
  2. the warning only logs once per app reload, so this lib could hide other areas of the app where there are problems
  3. the only way that I know to avoid this with certainty would be to use useLayoutEffect anytime that you compose hooks together like in this example, but that doesn't seem ideal

If you update the code to be useLayoutEffect for the "mirroring" code, it solves the problem, but it seems like it will be a challenge to determine whether to use useLayoutEffect or useEffect based on how something "downstream" will respond to the state changes.

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

Versions:

react: 16.8.3
react-dom: 16.8.3

I am not sure if this works in previous versions of React.

@gaearon
Copy link
Collaborator

gaearon commented Apr 3, 2019

Is this the same as #15057?

@jamesplease
Copy link
Author

Looks like it. I'll close this out and follow along on the conversation over there.

@gaearon
Copy link
Collaborator

gaearon commented Jul 30, 2019

This appears fixed in 0.0.0-db3ae32b8 (which is a release candidate for 16.9).

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

2 participants