search

mobxjs / mobx-react-lite

Lightweight React bindings for MobX based on React 16.8 and Hooks
https://mobx.js.org/react-integration.html
MIT License
2.13k stars 91 forks source link

Fix for a possible memory leak in observer #278

Closed hzzcc closed 3 years ago

hzzcc commented 4 years ago

Reaction cause memory leak

Making a change to documentation only? Delete the rest of the template and go ahead.

Code change checklist

danielkcz commented 4 years ago

Please fix error in failing build. And can you elaborate on why this change? Did you encounter some issue that you are able to reproduce? In that case, can you add a test that covers that issue?

mweststrate commented 4 years ago

Thanks for taking the effort of creating this PR, but please elaborate on how this prevents a memory leak, as we can't accept changes without some rationally or verification

hzzcc commented 4 years ago

image image

danielkcz commented 4 years ago

Sorry, but one screenshot and tsignore (😮) in the code won't do. Can you explain the problem in words?

hzzcc commented 4 years ago

https://github.com/hzzcc/mobx-react-lite-test Here is the test repo.

hzzcc commented 4 years ago

The test flow:

  1. click "onChange"
  2. change the input
  3. click "onChange" to unmount input
  4. profile the memory
danielkcz commented 4 years ago

Sorry, but the test repo is not gonna be enough. If you have discovered a flaw, we need to figure out to reproduce it in a test file and that needs to be added as a part of this PR. The reason is to avoid regressions for future changes.

danielkcz commented 4 years ago

I will keep it open as it might worth investigating if there is an actual memory leak. Help is certainly wanted here.

danielkcz commented 3 years ago

We now have an alternative solution #332 so it's probably not worth it to worry about a memory leak in here. Besides nobody else seem to have noticed anything.