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 90 forks source link

Use Finalization registry to dispose reactions of uncommitted components #332

Closed Bnaya closed 3 years ago

Bnaya commented 3 years ago

background: https://github.com/mobxjs/mobx/discussions/2562

On this PR we introduce additional mechanism to dispose reactions from uncommitted components

The new mechanism will kick-in when platforms supports finalization registry

High-level changes: reactionCleanupTracking now expose slightly different api that is covering both mechanisms, so useObserver is agnostic to the actual impl

Test changes: The tests now must run using node 14 + --expose-gc We test both impls with single jest run

Code change checklist

changeset-bot[bot] commented 3 years ago

🦋 Changeset detected

Latest commit: 7c7a58b44205f6b6ae9e9e8a08b80efacad91084

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | --------------- | ----- | | mobx-react-lite | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

benjamingr commented 3 years ago

@littledan - I don't know if this is interesting to you but I remember talking about use cases and behaviours for FinalizationRegistry some time ago (in the Node collab summit). This is a pretty solid one where React no longer guarantees it will call cleanup functions, MobX needs to run cleanup sometime in the future after the state/object is no longer retained and MobX will use a FinalizationRegistry to accomplish this.

I would be curious about whether or not you think this is a good idea (it's not like we have a choice anyway 😅 🤷‍♂️ )

Bnaya commented 3 years ago

@littledan - I don't know if this is interesting to you but I remember talking about use cases and behaviours for FinalizationRegistry some time ago (in the Node collab summit). This is a pretty solid one where React no longer guarantees it will call cleanup functions, MobX needs to run cleanup sometime in the future after the state/object is no longer retained and MobX will use a FinalizationRegistry to accomplish this.

I would be curious about whether or not you think this is a good idea (it's not like we have a choice anyway 😅 🤷‍♂️ )

Worth nothing that, this comes to replace heuristic, setTimeout based user-land garbage collector. If the component wasn't committed after XXX time, we will dispose the reaction.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+1.08%) to 94.828% when pulling ea867164b136724e5769afd8985263bb274d8a38 on finalizationregistry-based-dispose into 365c3e35b74eee93f6c2d3fa9b013441612210e5 on master.

mweststrate commented 3 years ago

Didn't review the tests, but the logic itself looks solid to me! Awesome approach :)

Bnaya commented 3 years ago

I will address the comments & do more browsers testing tomorrow, Hopefully also merge it