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

Warning: Can't perform a React state update on an unmounted component. #196

Closed
orballo opened this issue Jul 4, 2020 · 9 comments
Closed
Labels

Comments

@orballo
Copy link

orballo commented Jul 4, 2020

React Easy State version: 6.3.0
Platform: browser

Describe the bug
I get the following warning when editing an <input/> field while using React.StrictMode:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
    in _c (at App.js:46)

For tougher bugs

To Reproduce
In this CodeSandbox
Click Switch to /about, then click again Switch to / and enter some value in the <input /> element.
You'll see the warning in console.

Expected behavior
I shouldn't see any warning.
I'm not sure if this is a bug or I'm doing something wrong. Either way I cannot figure out what's happening :/

@orballo orballo added the bug label Jul 4, 2020
@baragona
Copy link

baragona commented Sep 8, 2020

I agree, this appears to be a bug. I would guess that somehow the observer is not getting cancelled when it should.

@cphillips
Copy link

Interestingly I ran into this using react-osh, and then switched to react-easy-view only to find the issue here is well. It makes sense now as both libraries are using @nx-js/observer-util

@irgijs
Copy link

irgijs commented Oct 20, 2020

I am also running into this bug. Does someone have a solution for this?

@mattmccray
Copy link

Is this a serious bug, or is it a "safe to ignore bug," like some that React reports?

@irgijs
Copy link

irgijs commented Jan 19, 2021

@mattmccray It may be serious. I would not recommend using this library until this is fixed. I haven't been able to find the culprit causing the issue yet, so our team decided to use different state management mechanisms.

@mattmccray
Copy link

It appears to be in the view hoc for function components. If you convert components in the example codesandbox to class components, the warning goes away.

https://codesandbox.io/s/errors-with-react-easy-state-forked-8yqc1?file=/src/App2.js

@luisherranz
Copy link

I took a look at this problem and it is interesting because this is not an actual bug. At least not yet.

This warning is caused because React, in StrictMode, renders components two times in development mode. Then, it discards the first render:

This is intentional. The React team does it to break components that contain side-effects. And in this case, they succeed.

The specific problem of view is:

  • Each run of the component creates an internal reaction (observe(Comp)).
  • Only one of those components is actually mounted.
  • The other component still exists and it is still registered by react-easy-state as a valid reaction.
  • When the observable changes, react-easy-state invalidates both reactions, internally calling the setState({}) for both.
  • React complains about the component that was never mounted, because you are trying to call the setState of an unmounted component.

I have forked @orballo's CodeSandbox and added console logs to make more obvious what is happening: https://codesandbox.io/s/errors-with-react-easy-state-forked-inj5z?file=/src/view.js. I have applied a fix to bypass the rerender on unmounted components.

I say this is not an actual bug because rendering a component and never mounting it is something that React doesn't do right now, even in StrictMode. It does it in the development mode of StrictMode because React is trying to prepare libraries for the upcoming Concurrent mode.

I have done a PR (#227) that checks if the component is mounted or not before triggering a rerender. It is the same fix I used in the CodeSandbox. But it is important to notice two things:

  1. This PR doesn't fix the memory leak that will happen in Concurrent mode, only fixes the warning. I will open a new issue to discuss how we can solve the memory leak, which requires a more complex solution.
  2. This memory leak problem doesn't happen right now. It will happen in React Concurrent.

So, as a summary:

  1. The current version of RES is 100% safe to use in:

    • React without StrictMode.
    • React with StrictMode in production.
  2. The current version of RES has a memory leak caused intentionally by React to show a future problem in:

    • React with StrictMode in development.
  3. The current version of RES has e memory leak problem in:

    • Future React in Concurrent mode.
  4. The PR I did, once merged and released, will solve the React warning, but not the memory leak in:

    • React with StrictMode in development.
    • Future React in Concurrent mode.

Finally, I'd like to mention that the memory leak forced by React in the development mode of StrictMode is minimal and shouldn't have any measurable impact while developing.

@luisherranz
Copy link

In case you are curious, this is my proposal to add support for React Concurrent: #228

@jonlepage
Copy link

Dam, before search on this issue, i just pass some hour to try understand !
And is true, i cant found memory leak in memory of my app !!!
This message is hellish and misleading due to the terminology used and give me big headhash !
image
image

So i can confirm what is say upper ! no memleak, its just react in strictMode!
i think they should use better word ! like << hey , your approach may cause memoryleak >>
image

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

No branches or pull requests

7 participants