Use Finalization registry to dispose reactions of uncommitted components #332
Conversation
🦋 Changeset detectedLatest commit: 7c7a58b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@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. |
Bnaya
force-pushed
the
finalizationregistry-based-dispose
branch
from
0fdc5c4
to
44a41ef
Compare
Bnaya
force-pushed
the
finalizationregistry-based-dispose
branch
from
c63c079
to
cf1ff3b
Compare
Bnaya
changed the title
package.json
Outdated
| @@ -23,8 +23,9 @@ | |||
| "prettier": "prettier --write \"./{src,test}/*.{js,ts,tsx}\"", | |||
| "lint": "eslint . --ext .js,.ts,.tsx", | |||
| "validate": "tsc --noEmit", | |||
| "test": "jest --watch", | |||
| "test:ci": "jest -i --coverage", | |||
| "jest": "node --expose-gc $(yarn bin jest)", | |||
There was a problem hiding this comment.
This will not work on windows machines.
Any idea what we can do there?
There was a problem hiding this comment.
And why do we need it exactly?
There was a problem hiding this comment.
On the Finalization registry test we use that to force GC so the cleanup will run.
I'v tried other ways without much success.
I can give it another try
There was a problem hiding this comment.
Maybe this could help? https://www.npmjs.com/package/expose-gc
There was a problem hiding this comment.
It doesn't work on windows because of the bash not because of --expose-gc (that works on windows).
$(yarn bin jest) expands to the jest location. I think since the path is already supposed to be set I would use NODE_FLAGS with cross-env (to set them up in Windows).
Alternatively, I think it's fine not to run this test on Windows.
There was a problem hiding this comment.
NODE_FLAGS dose not work with expose gc unfortunately,
Using that package seems working, i will push it soon
| throw new Error("This test must run with node >= 14 and --expose-gc flag") | ||
| } | ||
|
|
||
| const store = mobx.observable({ count1: 0, count2: 0 }) |
There was a problem hiding this comment.
We may explicitly share this code part with the test with timers
| resetCleanupScheduleForTests, | ||
| forceCleanupTimerToRunNowForTests | ||
| } = FinalizationRegistryMaybeUndefined | ||
| ? createReactionCleanupTrackingUsingFinalizationRegister(FinalizationRegistryMaybeUndefined) |
There was a problem hiding this comment.
It would be nice to separate each of these methods into a separate file. For easier maintenance in case, we want to drop timers eventually.
There was a problem hiding this comment.
@Bnaya After splitting this I think we merge :)
|
Didn't review the tests, but the logic itself looks solid to me! Awesome approach :) |
| @@ -10,3 +10,9 @@ export function enableDevEnvironment() { | |||
| process.env.NODE_ENV === "production" | |||
| } | |||
| } | |||
|
|
|||
| export function sleep(time: number) { | |||
There was a problem hiding this comment.
This can be promise/timers or util.promisify(setTimeout) :]
There was a problem hiding this comment.
Echoing @mweststrate 's suggestion that this is useful for more than MobX react and should probably be published as a separate package as well as a generic "useCleanup" hook as well probably?
|
I will address the comments & do more browsers testing tomorrow, |
background: mobxjs/mobx#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:
reactionCleanupTrackingnow expose slightly different api that is covering both mechanisms,so
useObserveris agnostic to the actual implTest changes:
The tests now must run using node 14 +
--expose-gcWe test both impls with single jest run
Code change checklist
READMEif applicable