-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
useObserver
registry refactor
#3598
Changes from all commits
06a8746
965b6f9
55e2350
5864ff7
d78789e
b6e7128
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"mobx-react-lite": patch | ||
--- | ||
|
||
refactor reaction tracking |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,20 @@ | ||
import { cleanup, render } from "@testing-library/react" | ||
import * as mobx from "mobx" | ||
import * as React from "react" | ||
|
||
import { useObserver } from "../src/useObserver" | ||
import { sleep } from "./utils" | ||
import { FinalizationRegistry } from "../src/utils/FinalizationRegistryWrapper" | ||
|
||
// @ts-ignore | ||
import gc from "expose-gc/function" | ||
import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" | ||
|
||
if (typeof globalThis.FinalizationRegistry !== "function") { | ||
throw new Error("This test must run with node >= 14") | ||
} | ||
|
||
expect(observerFinalizationRegistry).toBeInstanceOf(globalThis.FinalizationRegistry) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does it do if it fails? it's not inside any |
||
|
||
afterEach(cleanup) | ||
|
||
test("uncommitted components should not leak observations", async () => { | ||
if (!FinalizationRegistry) { | ||
throw new Error("This test must run with node >= 14") | ||
} | ||
|
||
const store = mobx.observable({ count1: 0, count2: 0 }) | ||
|
||
// Track whether counts are observed | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,22 @@ import "./utils/killFinalizationRegistry" | |
import { act, cleanup, render } from "@testing-library/react" | ||
import * as mobx from "mobx" | ||
import * as React from "react" | ||
|
||
import { useObserver } from "../src/useObserver" | ||
import { | ||
forceCleanupTimerToRunNowForTests, | ||
resetCleanupScheduleForTests | ||
} from "../src/utils/reactionCleanupTracking" | ||
import { | ||
CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, | ||
CLEANUP_TIMER_LOOP_MILLIS | ||
} from "../src/utils/reactionCleanupTrackingCommon" | ||
REGISTRY_FINALIZE_AFTER, | ||
REGISTRY_SWEEP_INTERVAL | ||
} from "../src/utils/UniversalFinalizationRegistry" | ||
import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" | ||
import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry" | ||
|
||
expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegistry) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Throws?
Does it matter? It's mainly intented to assert environment (bug in the test), rather than testing that |
||
|
||
const registry = observerFinalizationRegistry as TimerBasedFinalizationRegistry<unknown> | ||
|
||
afterEach(cleanup) | ||
|
||
test("uncommitted components should not leak observations", async () => { | ||
resetCleanupScheduleForTests() | ||
registry.finalizeAllImmediately() | ||
|
||
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake | ||
// that out in parallel to Jest useFakeTimers | ||
|
@@ -51,7 +52,7 @@ test("uncommitted components should not leak observations", async () => { | |
) | ||
|
||
// Allow any reaction-disposal cleanup timers to run | ||
const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS) | ||
const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) | ||
fakeNow += skip | ||
jest.advanceTimersByTime(skip) | ||
|
||
|
@@ -72,7 +73,7 @@ test("cleanup timer should not clean up recently-pended reactions", () => { | |
// 5. The commit phase runs for component A, but reaction R2 has already been disposed. Game over. | ||
|
||
// This unit test attempts to replicate that scenario: | ||
resetCleanupScheduleForTests() | ||
registry.finalizeAllImmediately() | ||
|
||
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake | ||
// that out in parallel to Jest useFakeTimers | ||
|
@@ -106,7 +107,7 @@ test("cleanup timer should not clean up recently-pended reactions", () => { | |
// We force our cleanup loop to run even though enough time hasn't _really_ | ||
// elapsed. In theory, it won't do anything because not enough time has | ||
// elapsed since the reactions were queued, and so they won't be disposed. | ||
forceCleanupTimerToRunNowForTests() | ||
registry.sweep() | ||
|
||
// Advance time enough to allow any timer-queued effects to run | ||
jest.advanceTimersByTime(500) | ||
|
@@ -137,7 +138,7 @@ test.skip("component should recreate reaction if necessary", () => { | |
|
||
// This unit test attempts to replicate that scenario: | ||
|
||
resetCleanupScheduleForTests() | ||
registry.finalizeAllImmediately() | ||
|
||
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake | ||
// that out in parallel to Jest useFakeTimers | ||
|
@@ -166,9 +167,9 @@ test.skip("component should recreate reaction if necessary", () => { | |
// and _then_ the component commits. | ||
|
||
// Force everything to be disposed. | ||
const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS) | ||
const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) | ||
fakeNow += skip | ||
forceCleanupTimerToRunNowForTests() | ||
registry.sweep() | ||
|
||
// The reaction should have been cleaned up. | ||
expect(countIsObserved).toBeFalsy() | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
export declare class FinalizationRegistryType<T> { | ||
constructor(finalize: (value: T) => void) | ||
register(target: object, value: T, token?: object): void | ||
unregister(token: object): void | ||
} | ||
|
||
declare const FinalizationRegistry: typeof FinalizationRegistryType | undefined | ||
|
||
export const REGISTRY_FINALIZE_AFTER = 10_000 | ||
export const REGISTRY_SWEEP_INTERVAL = 10_000 | ||
|
||
export class TimerBasedFinalizationRegistry<T> implements FinalizationRegistryType<T> { | ||
private registrations: Map<unknown, { value: T; registeredAt: number }> = new Map() | ||
private sweepTimeout: ReturnType<typeof setTimeout> | undefined | ||
|
||
constructor(private readonly finalize: (value: T) => void) {} | ||
|
||
// Token is actually required with this impl | ||
register(target: object, value: T, token?: object) { | ||
this.registrations.set(token, { | ||
value, | ||
registeredAt: Date.now() | ||
}) | ||
this.scheduleSweep() | ||
} | ||
|
||
unregister(token: unknown) { | ||
this.registrations.delete(token) | ||
} | ||
|
||
// Bound so it can be used directly as setTimeout callback. | ||
sweep = (maxAge = REGISTRY_FINALIZE_AFTER) => { | ||
// cancel timeout so we can force sweep anytime | ||
clearTimeout(this.sweepTimeout) | ||
this.sweepTimeout = undefined | ||
|
||
const now = Date.now() | ||
this.registrations.forEach((registration, token) => { | ||
if (now - registration.registeredAt >= maxAge) { | ||
this.finalize(registration.value) | ||
this.registrations.delete(token) | ||
} | ||
}) | ||
|
||
if (this.registrations.size > 0) { | ||
this.scheduleSweep() | ||
} | ||
} | ||
|
||
// Bound so it can be exported directly as clearTimers test utility. | ||
finalizeAllImmediately = () => { | ||
this.sweep(0) | ||
} | ||
|
||
private scheduleSweep() { | ||
if (this.sweepTimeout === undefined) { | ||
this.sweepTimeout = setTimeout(this.sweep, REGISTRY_SWEEP_INTERVAL) | ||
} | ||
} | ||
} | ||
|
||
export const UniversalFinalizationRegistry = FinalizationRegistry ?? TimerBasedFinalizationRegistry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change this assertion also to expect to match the other checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... But then the error message won't be the same think think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did want to preserve the message.