From 223ce6be2f85dd1ae66a519aea305512e1e641f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 26 Nov 2022 23:40:59 +0100 Subject: [PATCH 01/38] wip --- packages/mobx-react-lite/src/observer.ts | 12 +++ packages/mobx-react-lite/src/useObserver.ts | 100 +++++++++++++++++- .../utils/UniversalFinalizationRegistry.ts | 59 +++++++++++ .../src/utils/observerFinalizationRegistry.ts | 9 ++ .../mobx/__tests__/v5/base/observables.js | 27 ++++- packages/mobx/src/core/atom.ts | 6 +- packages/mobx/src/core/globalstate.ts | 5 + 7 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts create mode 100644 packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index dee0bb2e5..3117eb3e2 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/packages/mobx-react-lite/src/observer.ts @@ -1,3 +1,4 @@ +import { Reaction } from "mobx" import { forwardRef, memo } from "react" import { isUsingStaticRendering } from "./staticRendering" @@ -164,3 +165,14 @@ function copyStaticProperties(base: any, target: any) { } }) } + +export type ObserverInstance = { + reaction: Reaction | null // also works as disposed flag + forceUpdate: Function | null // also works as mounted flag + // BC: we will use local state version if global isn't available. + // It should behave as previous implementation - tearing is still present, + // because there is no cross component synchronization, + // but we can use `useExternalSyncStore` API. + stateVersion: any + componentName: string +} diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index a5d889f7a..55adfa3ea 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -1,4 +1,4 @@ -import { Reaction } from "mobx" +import { Reaction, _getGlobalState } from "mobx" import React from "react" import { printDebugValue } from "./utils/printDebugValue" import { @@ -7,11 +7,18 @@ import { recordReactionAsCommitted } from "./utils/reactionCleanupTracking" import { isUsingStaticRendering } from "./staticRendering" +import { ObserverInstance } from "./observer" +import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" function observerComponentNameFor(baseComponentName: string) { return `observer${baseComponentName}` } +const mobxGlobalState = _getGlobalState() + +// BC +const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined" + /** * We use class to make it easier to detect in heap snapshots by name */ @@ -21,7 +28,7 @@ function objectToBeRetainedByReactFactory() { return new ObjectToBeRetainedByReact() } -export function useObserver(fn: () => T, baseComponentName: string = "observed"): T { +export function useLegacyObserver(fn: () => T, baseComponentName: string = "observed"): T { if (isUsingStaticRendering()) { return fn() } @@ -124,3 +131,92 @@ export function useObserver(fn: () => T, baseComponentName: string = "observe return rendering } + +function createReaction(instance: ObserverInstance) { + instance.reaction = new Reaction(observerComponentNameFor(instance.componentName), () => { + if (!globalStateVersionIsAvailable) { + // BC + instance.stateVersion = Symbol() + } + instance.forceUpdate?.() + }) +} + +function scheduleReactionDisposal(instance: ObserverInstance) {} + +function cancelReactionDisposal(instance: ObserverInstance) {} + +function disposeReaction(instance: ObserverInstance) { + instance.reaction?.dispose() + instance.reaction = null +} + +// reset/tearDown/suspend/detach +function dispose(instance: ObserverInstance) { + instance.forceUpdate = null + disposeReaction(instance) +} + +export function useObserver(render: () => T, baseComponentName: string = "observed"): T { + if (isUsingStaticRendering()) { + return render() + } + + // StrictMode/ConcurrentMode/Suspense may mean that our component is + // rendered and abandoned multiple times, so we need to track leaked + // Reactions. + const instanceRef = React.useRef(null) + + if (!instanceRef.current) { + const instance: ObserverInstance = { + reaction: null, + forceUpdate: null, + stateVersion: Symbol(), + componentName: baseComponentName + } + + createReaction(instance) + + instanceRef.current = instance + observerFinalizationRegistry.register(instanceRef, instance, instanceRef) + } + + const { reaction } = instanceRef.current! + React.useDebugValue(reaction!, printDebugValue) + + React.useSyncExternalStore( + onStoreChange => { + observerFinalizationRegistry.unregister(instanceRef) + const instance = instanceRef.current! + instance.forceUpdate = onStoreChange + if (!instance.reaction) { + createReaction(instance) + } + + return () => dispose(instanceRef.current!) + }, + () => + globalStateVersionIsAvailable + ? mobxGlobalState.stateVersion + : instanceRef.current?.stateVersion + ) + + // render the original component, but have the + // reaction track the observables, so that rendering + // can be invalidated (see above) once a dependency changes + let renderResult!: T + let exception + reaction!.track(() => { + try { + renderResult = render() + } catch (e) { + exception = e + } + }) + + if (exception) { + throw exception // re-throw any exceptions caught during rendering + } + + return renderResult +} diff --git a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts new file mode 100644 index 000000000..9dfe2380e --- /dev/null +++ b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts @@ -0,0 +1,59 @@ +declare class FinalizationRegistryType { + 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 implements FinalizationRegistryType { + private registrations: Map = new Map() + private sweepTimeout: ReturnType | undefined + + constructor(private readonly finalize: (value: T) => void) {} + + register(target, value, token) { + this.registrations.set(token, { + value, + registeredAt: Date.now() + }) + this.scheduleSweep() + } + + unregister(token: unknown) { + this.registrations.delete(token) + } + + 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() + } + } + + public finalizeAllImmediately() { + this.sweep(0) + } + + private scheduleSweep() { + if (this.sweepTimeout === undefined) { + this.sweepTimeout = setTimeout(this.sweep, REGISTRY_SWEEP_INTERVAL) + } + } +} + +export const UniversalFinalizationRegistry = FinalizationRegistry ?? TimerBasedFinalizationRegistry diff --git a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts new file mode 100644 index 000000000..6b2adad99 --- /dev/null +++ b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts @@ -0,0 +1,9 @@ +import { ObserverInstance } from "../observer" +import { UniversalFinalizationRegistry } from "./UniversalFinalizationRegistry" + +export const observerFinalizationRegistry = new UniversalFinalizationRegistry( + (instance: ObserverInstance) => { + instance.reaction?.dispose() + instance.reaction = null + } +) diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index 460553cc3..cc8230c61 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -15,7 +15,7 @@ const { isObservableProp } = mobx const utils = require("../../v5/utils/test-utils") -const { MAX_SPLICE_SIZE } = require("../../../src/internal") +const { MAX_SPLICE_SIZE, getGlobalState } = require("../../../src/internal") const voidObserver = function () {} @@ -2360,3 +2360,28 @@ describe("`requiresObservable` takes precedence over global `reactionRequiresObs expect(consoleWarnSpy).not.toHaveBeenCalled() }) }) + +test("state version updates correctly", () => { + // The current implementation updates state version at the end of batch. + // This test demonstrates that the version is correctly updated with each state mutations: + // 1. Even without wrapping mutation in batch explicitely. + // 2. Even in self-invoking recursive derivation. + const o = mobx.observable({ x: 0 }) + let prevStateVersion + + const disposeAutorun = mobx.autorun(() => { + if (o.x === 5) { + disposeAutorun() + return + } + const currentStateVersion = getGlobalState().stateVersion + expect(prevStateVersion).not.toBe(currentStateVersion) + prevStateVersion = currentStateVersion + o.x++ + }) + + prevStateVersion = getGlobalState().stateVersion + o.x++ + expect(o.x).toBe(5) + expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) +}) diff --git a/packages/mobx/src/core/atom.ts b/packages/mobx/src/core/atom.ts index c62cbd703..b51f4887e 100644 --- a/packages/mobx/src/core/atom.ts +++ b/packages/mobx/src/core/atom.ts @@ -11,7 +11,8 @@ import { propagateChanged, reportObserved, startBatch, - Lambda + Lambda, + globalState } from "../internal" export const $mobx = Symbol("mobx administration") @@ -66,6 +67,9 @@ export class Atom implements IAtom { public reportChanged() { startBatch() propagateChanged(this) + // We could update state version only at the end of batch, + // but we would still have to switch some global flag here to signal a change. + globalState.stateVersion = Symbol() endBatch() } diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index 35de9dfea..bd01cfe7d 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -150,6 +150,11 @@ export class MobXGlobals { * configurable: true */ safeDescriptors = true + + /** + * Changes with each state update, used by useMutableSyncStore + */ + stateVersion = Symbol() } let canMergeGlobalState = true From fbe7604d04eb812a7114276b0f8aa86e1dc4d693 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 27 Nov 2022 23:51:00 +0100 Subject: [PATCH 02/38] wip --- .../__tests__/observer.test.tsx | 83 ++++++++ ...trictAndConcurrentModeUsingTimers.test.tsx | 190 +++++++++++++++++- packages/mobx-react-lite/src/index.ts | 6 +- packages/mobx-react-lite/src/useObserver.ts | 97 +++++++-- .../utils/UniversalFinalizationRegistry.ts | 8 +- .../src/utils/observerFinalizationRegistry.ts | 7 +- yarn.lock | 21 +- 7 files changed, 377 insertions(+), 35 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx index f0d636700..94a315bd9 100644 --- a/packages/mobx-react-lite/__tests__/observer.test.tsx +++ b/packages/mobx-react-lite/__tests__/observer.test.tsx @@ -1084,3 +1084,86 @@ test("Anonymous component displayName #3192", () => { expect(observerError.message).toEqual(memoError.message) consoleErrorSpy.mockRestore() }) + +describe.skip("simple TODO del", () => { + let state = {} + let onStoreUpdate = () => {} + + function updateStore(newState) { + state = newState + onStoreUpdate() + } + + const subscribe = _onStoreUpdate => { + console.log("SUBSCRIBE") + onStoreUpdate = _onStoreUpdate + return () => { + console.log("UNSUBSCRIBE") + onStoreUpdate = () => {} + } + } + + const getSnapshot = () => state + + function TestCmp() { + console.log("RENDER") + + React.useEffect(() => { + console.log("MOUNT") + return () => console.log("UNMOUNT") + }, []) + + React.useSyncExternalStore(subscribe, getSnapshot) + + return null + } + + test("", async () => { + render() + + act(() => { + //console.log('before update') + updateStore({}) + //console.log('after update') + }) + // console.log('before await'); + // await (new Promise(resolve =>setTimeout(resolve, 1000))); + // console.log('after await'); + // act(()=> { + // //console.log('before update') + // updateStore({}); + // //console.log('after update') + // }) + + console.log("after act") + }) + + const execute = () => { + const renderings = { + child: 0, + parent: 0 + } + const data = { x: 1 } + const odata = mobx.observable({ y: 1 }) + + const Child = observer((props: any) => { + renderings.child++ + return {props.data.x} + }) + const TestCmp = observer(() => { + renderings.parent++ + return {odata.y} + }) + return { ...render(), renderings, odata } + } + + test("prd", () => { + const { container, renderings, odata } = execute() + expect(renderings.parent).toBe(1) + act(() => { + odata.y++ + }) + expect(renderings.parent).toBe(2) + expect(container.querySelector("span")!.innerHTML).toBe("1") + }) +}) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx index 945a5d15a..4bc8701f2 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx @@ -12,9 +12,20 @@ import { CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS } from "../src/utils/reactionCleanupTrackingCommon" +import { + REGISTRY_FINALIZE_AFTER, + REGISTRY_SWEEP_INTERVAL +} from "../src/utils/UniversalFinalizationRegistry" -afterEach(cleanup) +import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" +import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry" + +expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegistry) +const registry = observerFinalizationRegistry as TimerBasedFinalizationRegistry + +afterEach(cleanup) +/* test("uncommitted components should not leak observations", async () => { resetCleanupScheduleForTests() @@ -190,3 +201,180 @@ test.skip("component should recreate reaction if necessary", () => { // wasn't even looking. expect(rendering.baseElement.textContent).toContain("42") }) +*/ + +test("uncommitted components should not leak observations", async () => { + registry.finalizeAllImmediately() + + // Unfortunately, Jest fake timers don't mock out Date.now, so we fake + // that out in parallel to Jest useFakeTimers + let fakeNow = Date.now() + jest.useFakeTimers() + jest.spyOn(Date, "now").mockImplementation(() => fakeNow) + + const store = mobx.observable({ count1: 0, count2: 0 }) + + // Track whether counts are observed + let count1IsObserved = false + let count2IsObserved = false + mobx.onBecomeObserved(store, "count1", () => (count1IsObserved = true)) + mobx.onBecomeUnobserved(store, "count1", () => (count1IsObserved = false)) + mobx.onBecomeObserved(store, "count2", () => (count2IsObserved = true)) + mobx.onBecomeUnobserved(store, "count2", () => (count2IsObserved = false)) + + const TestComponent1 = () => useObserver(() =>
{store.count1}
) + const TestComponent2 = () => useObserver(() =>
{store.count2}
) + + // Render, then remove only #2 + const rendering = render( + + + + + ) + rendering.rerender( + + + + ) + + // Allow any reaction-disposal cleanup timers to run + const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) + fakeNow += skip + jest.advanceTimersByTime(skip) + + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(count1IsObserved).toBeTruthy() + expect(count2IsObserved).toBeFalsy() +}) + +test("cleanup timer should not clean up recently-pended reactions", () => { + // If we're not careful with timings, it's possible to get the + // following scenario: + // 1. Component instance A is being created; it renders, we put its reaction R1 into the cleanup list + // 2. Strict/Concurrent mode causes that render to be thrown away + // 3. Component instance A is being created; it renders, we put its reaction R2 into the cleanup list + // 4. The MobX reaction timer from 5 seconds ago kicks in and cleans up all reactions from uncommitted + // components, including R1 and R2 + // 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: + registry.finalizeAllImmediately() + + // Unfortunately, Jest fake timers don't mock out Date.now, so we fake + // that out in parallel to Jest useFakeTimers + const fakeNow = Date.now() + jest.useFakeTimers() + jest.spyOn(Date, "now").mockImplementation(() => fakeNow) + + const store = mobx.observable({ count: 0 }) + + // Track whether the count is observed + let countIsObserved = false + mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) + mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) + + const TestComponent1 = () => useObserver(() =>
{store.count}
) + + const rendering = render( + // We use StrictMode here, but it would be helpful to switch this to use real + // concurrent mode: we don't have a true async render right now so this test + // isn't as thorough as it could be. + + + + ) + + // We need to trigger our cleanup timer to run. We can't do this simply + // by running all jest's faked timers as that would allow the scheduled + // `useEffect` calls to run, and we want to simulate our cleanup timer + // getting in between those stages. + + // 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. + registry.sweep() + + // Advance time enough to allow any timer-queued effects to run + jest.advanceTimersByTime(500) + + // Now allow the useEffect calls to run to completion. + act(() => { + // no-op, but triggers effect flushing + }) + + // count should still be observed + expect(countIsObserved).toBeTruthy() +}) + +// TODO: MWE: disabled during React 18 migration, not sure how to express it icmw with testing-lib, +// and using new React renderRoot will fail icmw JSDOM +test.skip("component should recreate reaction if necessary", () => { + // There _may_ be very strange cases where the reaction gets tidied up + // but is actually still needed. This _really_ shouldn't happen. + // e.g. if we're using Suspense and the component starts to render, + // but then gets paused for 60 seconds, and then comes back to life. + // With the implementation of React at the time of writing this, React + // will actually fully re-render that component (discarding previous + // hook slots) before going ahead with a commit, but it's unwise + // to depend on such an implementation detail. So we must cope with + // the component having had its reaction tidied and still going on to + // be committed. In that case we recreate the reaction and force + // an update. + + // This unit test attempts to replicate that scenario: + + registry.finalizeAllImmediately() + + // Unfortunately, Jest fake timers don't mock out Date.now, so we fake + // that out in parallel to Jest useFakeTimers + let fakeNow = Date.now() + jest.useFakeTimers() + jest.spyOn(Date, "now").mockImplementation(() => fakeNow) + + const store = mobx.observable({ count: 0 }) + + // Track whether the count is observed + let countIsObserved = false + mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) + mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) + + const TestComponent1 = () => useObserver(() =>
{store.count}
) + + const rendering = render( + + + + ) + + // We need to trigger our cleanup timer to run. We don't want + // to allow Jest's effects to run, however: we want to simulate the + // case where the component is rendered, then the reaction gets cleaned up, + // and _then_ the component commits. + + // Force everything to be disposed. + const skip = Math.max(REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL) + fakeNow += skip + registry.sweep() + + // The reaction should have been cleaned up. + expect(countIsObserved).toBeFalsy() + + // Whilst nobody's looking, change the observable value + store.count = 42 + + // Now allow the useEffect calls to run to completion, + // re-awakening the component. + jest.advanceTimersByTime(500) + act(() => { + // no-op, but triggers effect flushing + }) + + // count should be observed once more. + expect(countIsObserved).toBeTruthy() + // and the component should have rendered enough to + // show the latest value, which was set whilst it + // wasn't even looking. + expect(rendering.baseElement.textContent).toContain("42") +}) diff --git a/packages/mobx-react-lite/src/index.ts b/packages/mobx-react-lite/src/index.ts index b493ee1e5..6834b416d 100644 --- a/packages/mobx-react-lite/src/index.ts +++ b/packages/mobx-react-lite/src/index.ts @@ -5,7 +5,7 @@ import { observerBatching } from "./utils/observerBatching" import { useDeprecated } from "./utils/utils" import { useObserver as useObserverOriginal } from "./useObserver" import { enableStaticRendering } from "./staticRendering" - +import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" observerBatching(batch) export { isUsingStaticRendering, enableStaticRendering } from "./staticRendering" @@ -14,7 +14,9 @@ export { Observer } from "./ObserverComponent" export { useLocalObservable } from "./useLocalObservable" export { useLocalStore } from "./useLocalStore" export { useAsObservableSource } from "./useAsObservableSource" -export { resetCleanupScheduleForTests as clearTimers } from "./utils/reactionCleanupTracking" +//export { resetCleanupScheduleForTests as clearTimers } from "./utils/reactionCleanupTracking" + +export const clearTimes = observerFinalizationRegistry["finalizeAllImmediately"] ?? (() => {}) export function useObserver(fn: () => T, baseComponentName: string = "observed"): T { if ("production" !== process.env.NODE_ENV) { diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 55adfa3ea..091c58d66 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -17,7 +17,8 @@ function observerComponentNameFor(baseComponentName: string) { const mobxGlobalState = _getGlobalState() // BC -const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined" +const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined" // TODO +//const globalStateVersionIsAvailable = false; // TODO /** * We use class to make it easier to detect in heap snapshots by name @@ -138,14 +139,18 @@ function createReaction(instance: ObserverInstance) { // BC instance.stateVersion = Symbol() } + // Force update won't be avaliable until the component "mounts". + // If state changes in between initial render and mount, + // `useExternalSyncStore` should handle that by checking the state version and issuing update. instance.forceUpdate?.() }) } +/* function scheduleReactionDisposal(instance: ObserverInstance) {} function cancelReactionDisposal(instance: ObserverInstance) {} - +*/ function disposeReaction(instance: ObserverInstance) { instance.reaction?.dispose() instance.reaction = null @@ -162,10 +167,15 @@ export function useObserver(render: () => T, baseComponentName: string = "obs return render() } + // Force update, see #2982 + const [, setState] = React.useState() + const forceUpdate = () => setState([] as any) + // StrictMode/ConcurrentMode/Suspense may mean that our component is // rendered and abandoned multiple times, so we need to track leaked // Reactions. const instanceRef = React.useRef(null) + const [finalizationRegistryTarget] = React.useState(objectToBeRetainedByReactFactory) if (!instanceRef.current) { const instance: ObserverInstance = { @@ -174,31 +184,78 @@ export function useObserver(render: () => T, baseComponentName: string = "obs stateVersion: Symbol(), componentName: baseComponentName } + // Opt: instead of useMemo we keep these on instance + // @ts-ignore + ;(instance.subscribe = (onStoreChange: () => void) => { + // Do NOT access instanceRef here! + //console.log('SUBSCRIBE'); + observerFinalizationRegistry.unregister(instance) + //const instance = instanceRef.current! + instance.forceUpdate = onStoreChange + if (!instance.reaction) { + createReaction(instance) + // We've lost our reaction and therefore all the subscriptions. + // We have to schedule re-render to recreate subscriptions, + // even if state did not change. + // TODO or we could jus transfer dependencies + //console.log('REACTION LOST, FORCING UPDATE'); + instance.forceUpdate() + } + + return () => { + // Do NOT access instanceRef here! + //console.log('UNSUBSCRIBE'); + //dispose(instanceRef.current!) + dispose(instance) + } + }), + // @ts-ignore + (instance.getSnapshot = () => + globalStateVersionIsAvailable + ? mobxGlobalState.stateVersion + : //: instanceRef.current?.stateVersion + instance.stateVersion) createReaction(instance) instanceRef.current = instance - observerFinalizationRegistry.register(instanceRef, instance, instanceRef) + observerFinalizationRegistry.register(instanceRef, instance, instance) } - const { reaction } = instanceRef.current! - React.useDebugValue(reaction!, printDebugValue) + const instance = instanceRef.current! + React.useDebugValue(instance.reaction!, printDebugValue) + + // const subscribe = React.useCallback((onStoreChange: () => void) => { + // console.log('SUBSCRIBE'); + // observerFinalizationRegistry.unregister(instanceRef) + // const instance = instanceRef.current! + // instance.forceUpdate = onStoreChange + // if (!instance.reaction) { + // createReaction(instance) + // // We've lost our reaction and therefore all the subscriptions. + // // We have to schedule re-render to recreate subscriptions, + // // even if state did not change. + // // TODO our we could jus transfer dependencies + // console.log('REACTION LOST, FORCING UPDATE'); + // instance.forceUpdate(); + // } + + // return () => { + // console.log('UNSUBSCRIBE'); + // dispose(instanceRef.current!) + // } + // }, []); React.useSyncExternalStore( - onStoreChange => { - observerFinalizationRegistry.unregister(instanceRef) - const instance = instanceRef.current! - instance.forceUpdate = onStoreChange - if (!instance.reaction) { - createReaction(instance) - } - - return () => dispose(instanceRef.current!) - }, - () => - globalStateVersionIsAvailable - ? mobxGlobalState.stateVersion - : instanceRef.current?.stateVersion + // Both of these must be stable, otherwise it would keep resubscribing every render. + // @ts-ignore + instance.subscribe, + // @ts-ignore + instance.getSnapshot + // () => + // globalStateVersionIsAvailable + // ? mobxGlobalState.stateVersion + // : instanceRef.current?.stateVersion ) // render the original component, but have the @@ -206,7 +263,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs // can be invalidated (see above) once a dependency changes let renderResult!: T let exception - reaction!.track(() => { + instance.reaction!.track(() => { try { renderResult = render() } catch (e) { diff --git a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts index 9dfe2380e..d361ccc53 100644 --- a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts @@ -1,4 +1,4 @@ -declare class FinalizationRegistryType { +export declare class FinalizationRegistryType { constructor(finalize: (value: T) => void) register(target: object, value: T, token?: object): void unregister(token: object): void @@ -15,7 +15,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy constructor(private readonly finalize: (value: T) => void) {} - register(target, value, token) { + register(target: object, value: T, token?: object) { this.registrations.set(token, { value, registeredAt: Date.now() @@ -34,7 +34,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy const now = Date.now() this.registrations.forEach((registration, token) => { - if (now - registration.registeredAt > maxAge) { + if (now - registration.registeredAt >= maxAge) { this.finalize(registration.value) this.registrations.delete(token) } @@ -45,7 +45,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy } } - public finalizeAllImmediately() { + finalizeAllImmediately = () => { this.sweep(0) } diff --git a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts index 6b2adad99..2be730e53 100644 --- a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts @@ -1,8 +1,13 @@ import { ObserverInstance } from "../observer" -import { UniversalFinalizationRegistry } from "./UniversalFinalizationRegistry" +import { + UniversalFinalizationRegistry, + // @ts-ignore + FinalizationRegistryType +} from "./UniversalFinalizationRegistry" export const observerFinalizationRegistry = new UniversalFinalizationRegistry( (instance: ObserverInstance) => { + //console.log('FINALIZING'); // TODO instance.reaction?.dispose() instance.reaction = null } diff --git a/yarn.lock b/yarn.lock index e4f319f9b..ac32c3811 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11760,12 +11760,12 @@ randomfill@^1.0.3: safe-buffer "^5.1.0" react-dom@^18.0.0: - version "18.0.0" - resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-18.0.0.tgz#26b88534f8f1dbb80853e1eabe752f24100d8023" - integrity sha512-XqX7uzmFo0pUceWFCt7Gff6IyIMzFUn7QMZrbrQfGxtaxXZIcGQzoNpRLE3fQLnS4XzLLPMZX2T9TRcSrasicw== + version "18.2.0" + resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-18.2.0.tgz#22aaf38708db2674ed9ada224ca4aa708d821e3d" + integrity sha512-6IMTriUmvsjHUjNtEDudZfuDQUoWXVxKHhlEGSk81n4YFS+r/Kl99wXiwlVXtPBtJenozv2P+hxDsw9eA7Xo6g== dependencies: loose-envify "^1.1.0" - scheduler "^0.21.0" + scheduler "^0.23.0" react-error-boundary@^3.1.0: version "3.1.4" @@ -11807,9 +11807,9 @@ react-test-renderer@^18.0.0: scheduler "^0.21.0" react@^18.0.0: - version "18.0.0" - resolved "https://registry.yarnpkg.com/react/-/react-18.0.0.tgz#b468736d1f4a5891f38585ba8e8fb29f91c3cb96" - integrity sha512-x+VL6wbT4JRVPm7EGxXhZ8w8LTROaxPXOqhlGyVSrv0sB1jkyFGgXxJ8LVoPRLvPR6/CIZGFmfzqUa2NYeMr2A== + version "18.2.0" + resolved "https://registry.yarnpkg.com/react/-/react-18.2.0.tgz#555bd98592883255fa00de14f1151a917b5d77d5" + integrity sha512-/3IjMdb2L9QbBdWiW5e3P2/npwMBaU9mHCSCUzNln0ZCYbcfTsGbTJrU/kGemdH2IWmB2ioZ+zkxtmq6g09fGQ== dependencies: loose-envify "^1.1.0" @@ -12490,6 +12490,13 @@ scheduler@^0.21.0: dependencies: loose-envify "^1.1.0" +scheduler@^0.23.0: + version "0.23.0" + resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.23.0.tgz#ba8041afc3d30eb206a487b6b384002e4e61fdfe" + integrity sha512-CtuThmgHNg7zIZWAXi3AsyIzA3n4xx7aNyjwC2VJldO2LMVDhFK+63xGqq6CsJH4rTAt6/M+N4GhZiDYPx9eUw== + dependencies: + loose-envify "^1.1.0" + schema-utils@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-1.0.0.tgz#0b79a93204d7b600d4b2850d1f66c2a34951c770" From 6976dec5630c1a122164efa96ef5b5c5edf922f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 27 Nov 2022 23:53:32 +0100 Subject: [PATCH 03/38] wip --- packages/mobx-react-lite/src/useObserver.ts | 46 ++++----------------- 1 file changed, 8 insertions(+), 38 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 091c58d66..fc34f5817 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -167,15 +167,10 @@ export function useObserver(render: () => T, baseComponentName: string = "obs return render() } - // Force update, see #2982 - const [, setState] = React.useState() - const forceUpdate = () => setState([] as any) - // StrictMode/ConcurrentMode/Suspense may mean that our component is // rendered and abandoned multiple times, so we need to track leaked // Reactions. const instanceRef = React.useRef(null) - const [finalizationRegistryTarget] = React.useState(objectToBeRetainedByReactFactory) if (!instanceRef.current) { const instance: ObserverInstance = { @@ -186,7 +181,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs } // Opt: instead of useMemo we keep these on instance // @ts-ignore - ;(instance.subscribe = (onStoreChange: () => void) => { + instance.subscribe = (onStoreChange: () => void) => { // Do NOT access instanceRef here! //console.log('SUBSCRIBE'); observerFinalizationRegistry.unregister(instance) @@ -208,13 +203,13 @@ export function useObserver(render: () => T, baseComponentName: string = "obs //dispose(instanceRef.current!) dispose(instance) } - }), - // @ts-ignore - (instance.getSnapshot = () => - globalStateVersionIsAvailable - ? mobxGlobalState.stateVersion - : //: instanceRef.current?.stateVersion - instance.stateVersion) + } + // @ts-ignore + instance.getSnapshot = () => + globalStateVersionIsAvailable + ? mobxGlobalState.stateVersion + : //: instanceRef.current?.stateVersion + instance.stateVersion createReaction(instance) @@ -225,37 +220,12 @@ export function useObserver(render: () => T, baseComponentName: string = "obs const instance = instanceRef.current! React.useDebugValue(instance.reaction!, printDebugValue) - // const subscribe = React.useCallback((onStoreChange: () => void) => { - // console.log('SUBSCRIBE'); - // observerFinalizationRegistry.unregister(instanceRef) - // const instance = instanceRef.current! - // instance.forceUpdate = onStoreChange - // if (!instance.reaction) { - // createReaction(instance) - // // We've lost our reaction and therefore all the subscriptions. - // // We have to schedule re-render to recreate subscriptions, - // // even if state did not change. - // // TODO our we could jus transfer dependencies - // console.log('REACTION LOST, FORCING UPDATE'); - // instance.forceUpdate(); - // } - - // return () => { - // console.log('UNSUBSCRIBE'); - // dispose(instanceRef.current!) - // } - // }, []); - React.useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. // @ts-ignore instance.subscribe, // @ts-ignore instance.getSnapshot - // () => - // globalStateVersionIsAvailable - // ? mobxGlobalState.stateVersion - // : instanceRef.current?.stateVersion ) // render the original component, but have the From 08ab24db78e9f2684b921ed494058a2881533b4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 17 Dec 2022 22:55:18 +0100 Subject: [PATCH 04/38] wip --- .vscode/settings.json | 2 + packages/mobx-react-lite/src/index.ts | 2 +- packages/mobx-react-lite/src/observer.ts | 5 +- packages/mobx-react-lite/src/useObserver.ts | 6 +- .../src/utils/observerFinalizationRegistry.ts | 8 +- .../__snapshots__/observer.test.tsx.snap | 20 +- .../__tests__/finalizationRegistry.tsx | 88 +++++ packages/mobx-react/__tests__/inject.test.tsx | 5 +- .../mobx-react/__tests__/observer.test.tsx | 72 +++- packages/mobx-react/__tests__/symbol.test.tsx | 25 -- packages/mobx-react/package.json | 3 +- packages/mobx-react/src/disposeOnUnmount.ts | 33 +- packages/mobx-react/src/index.ts | 9 +- packages/mobx-react/src/observerClass.ts | 348 ++++++++++-------- packages/mobx-react/src/utils/utils.ts | 23 +- packages/mobx/src/core/globalstate.ts | 2 +- 16 files changed, 418 insertions(+), 233 deletions(-) create mode 100644 packages/mobx-react/__tests__/finalizationRegistry.tsx delete mode 100644 packages/mobx-react/__tests__/symbol.test.tsx diff --git a/.vscode/settings.json b/.vscode/settings.json index dd4e99e9b..618db460d 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,9 +1,11 @@ { "[typescript]": { + "editor.defaultFormatter": "esbenp.prettier-vscode", "editor.formatOnSave": true, "editor.formatOnPaste": false }, "[javascript]": { + "editor.defaultFormatter": "esbenp.prettier-vscode", "editor.formatOnSave": true, "editor.formatOnPaste": false }, diff --git a/packages/mobx-react-lite/src/index.ts b/packages/mobx-react-lite/src/index.ts index 6834b416d..b62334f18 100644 --- a/packages/mobx-react-lite/src/index.ts +++ b/packages/mobx-react-lite/src/index.ts @@ -6,6 +6,7 @@ import { useDeprecated } from "./utils/utils" import { useObserver as useObserverOriginal } from "./useObserver" import { enableStaticRendering } from "./staticRendering" import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" +export { observerFinalizationRegistry as _observerFinalizationRegistry } observerBatching(batch) export { isUsingStaticRendering, enableStaticRendering } from "./staticRendering" @@ -14,7 +15,6 @@ export { Observer } from "./ObserverComponent" export { useLocalObservable } from "./useLocalObservable" export { useLocalStore } from "./useLocalStore" export { useAsObservableSource } from "./useAsObservableSource" -//export { resetCleanupScheduleForTests as clearTimers } from "./utils/reactionCleanupTracking" export const clearTimes = observerFinalizationRegistry["finalizeAllImmediately"] ?? (() => {}) diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index 3117eb3e2..1f71abda5 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/packages/mobx-react-lite/src/observer.ts @@ -166,6 +166,9 @@ function copyStaticProperties(base: any, target: any) { }) } +// TODO rename to ObserverAdministration +// Make sure NOT to store `this` or `instanceRef` (not even as part of a closure!) on this object, +// otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry. export type ObserverInstance = { reaction: Reaction | null // also works as disposed flag forceUpdate: Function | null // also works as mounted flag @@ -174,5 +177,5 @@ export type ObserverInstance = { // because there is no cross component synchronization, // but we can use `useExternalSyncStore` API. stateVersion: any - componentName: string + componentName: string // TODO just name } diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index fc34f5817..fd7bfbc4b 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -10,6 +10,7 @@ import { isUsingStaticRendering } from "./staticRendering" import { ObserverInstance } from "./observer" import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" +// TODO rename, it's only used for reaction function observerComponentNameFor(baseComponentName: string) { return `observer${baseComponentName}` } @@ -179,7 +180,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs stateVersion: Symbol(), componentName: baseComponentName } - // Opt: instead of useMemo we keep these on instance + // Opt: instead of useMemo we keep subscribe/getSnapshot on instance // @ts-ignore instance.subscribe = (onStoreChange: () => void) => { // Do NOT access instanceRef here! @@ -188,11 +189,12 @@ export function useObserver(render: () => T, baseComponentName: string = "obs //const instance = instanceRef.current! instance.forceUpdate = onStoreChange if (!instance.reaction) { + // TODO probably we can create reaction lazily in render and therefore simply issue forceUpdate, + // but keep in mind we don't want to use finalization registry at this point (we are mounted). createReaction(instance) // We've lost our reaction and therefore all the subscriptions. // We have to schedule re-render to recreate subscriptions, // even if state did not change. - // TODO or we could jus transfer dependencies //console.log('REACTION LOST, FORCING UPDATE'); instance.forceUpdate() } diff --git a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts index 2be730e53..5b9a34763 100644 --- a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts @@ -1,4 +1,4 @@ -import { ObserverInstance } from "../observer" +import { Reaction } from "mobx" import { UniversalFinalizationRegistry, // @ts-ignore @@ -6,9 +6,9 @@ import { } from "./UniversalFinalizationRegistry" export const observerFinalizationRegistry = new UniversalFinalizationRegistry( - (instance: ObserverInstance) => { + (adm: { reaction: Reaction | null }) => { //console.log('FINALIZING'); // TODO - instance.reaction?.dispose() - instance.reaction = null + adm.reaction?.dispose() + adm.reaction = null } ) diff --git a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap b/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap index 0dd2351e4..1ec4711f0 100644 --- a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap +++ b/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap @@ -18,6 +18,8 @@ exports[`#797 - replacing this.render should trigger a warning 1`] = ` } `; +exports[`#3492 should not cause warning by calling forceUpdate on uncommited components 1`] = `[MockFunction]`; + exports[`Redeclaring an existing observer component as an observer should log a warning 1`] = ` [MockFunction] { "calls": Array [ @@ -35,23 +37,7 @@ exports[`Redeclaring an existing observer component as an observer should log a } `; -exports[`SSR works #3448 1`] = ` -[MockFunction] { - "calls": Array [ - Array [ - "The reactive render of an observer class component (TestCmp) - was overridden after MobX attached. This may result in a memory leak if the - overridden reactive render was not properly disposed.", - ], - ], - "results": Array [ - Object { - "type": "return", - "value": undefined, - }, - ], -} -`; +exports[`SSR works #3448 1`] = `[MockFunction]`; exports[`issue 12 1`] = `
diff --git a/packages/mobx-react/__tests__/finalizationRegistry.tsx b/packages/mobx-react/__tests__/finalizationRegistry.tsx new file mode 100644 index 000000000..c1875069e --- /dev/null +++ b/packages/mobx-react/__tests__/finalizationRegistry.tsx @@ -0,0 +1,88 @@ +import { cleanup, render, waitFor } from "@testing-library/react" +import * as mobx from "mobx" +import * as React from "react" + +// @ts-ignore +import gc from "expose-gc/function" +import { observer } from "../src" + +afterEach(cleanup) + +function sleep(time: number) { + return new Promise(res => { + setTimeout(res, time) + }) +} + +// TODO dunno why it's not available +declare class WeakRef { + constructor(object: T) + deref(): T | undefined +} + +test("should not prevent GC of uncomitted components", async () => { + expect(typeof globalThis.FinalizationRegistry).toBe("function") + + // This specific setup causes first instance of A not being commited. + // This is checked by comparing constructor and componentDidMount invocation counts. + // There is no profound reason why that's the case, if you know a simpler or more robust setup + // feel free to change this. + + const o = mobx.observable({ x: 0 }) + let aConstructorCount = 0 + let aMountCount = 0 + + let firstARef: WeakRef + + @observer + class A extends React.Component { + constructor(props) { + super(props) + if (aConstructorCount === 0) { + firstARef = new WeakRef(this) + } + aConstructorCount++ + } + componentDidMount(): void { + aMountCount++ + } + render() { + return ( + + + {o.x} + + ) + } + } + + class B extends React.Component { + render() { + return "B" + } + } + + const LazyA = React.lazy(() => Promise.resolve({ default: A })) + const LazyB = React.lazy(() => Promise.resolve({ default: B })) + + function App() { + return ( + + + + ) + } + + const { unmount, container } = render() + + expect(container).toHaveTextContent("fallback") + await waitFor(() => expect(container).toHaveTextContent("B0")) + expect(aConstructorCount).toBe(2) + expect(aMountCount).toBe(1) + + gc() + await sleep(50) + expect(firstARef!.deref()).toBeUndefined() + + unmount() +}) diff --git a/packages/mobx-react/__tests__/inject.test.tsx b/packages/mobx-react/__tests__/inject.test.tsx index 3259b5bbb..b264a19b9 100644 --- a/packages/mobx-react/__tests__/inject.test.tsx +++ b/packages/mobx-react/__tests__/inject.test.tsx @@ -102,7 +102,7 @@ describe("inject based context", () => { expect(C.displayName).toBe("inject(ComponentC)") }) - test.only("shouldn't change original displayName of component that uses forwardRef", () => { + test("shouldn't change original displayName of component that uses forwardRef", () => { const FancyComp = React.forwardRef((_: any, ref: React.Ref) => { return
}) @@ -403,7 +403,8 @@ describe("inject based context", () => { expect(container).toHaveTextContent("Veria") }) - test("using a custom injector is not too reactive", () => { + // TODO + test.skip("using a custom injector is not too reactive", () => { let listRender = 0 let itemRender = 0 let injectRender = 0 diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index b4b0285d3..f6a64154e 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -1,6 +1,6 @@ -import React, { createContext, StrictMode } from "react" -import { inject, observer, Observer, enableStaticRendering, useStaticRendering } from "../src" -import { render, act } from "@testing-library/react" +import React, { createContext, Fragment, StrictMode, Suspense } from "react" +import { inject, observer, Observer, enableStaticRendering } from "../src" +import { render, act, waitFor } from "@testing-library/react" import { getObserverTree, _resetGlobalState, @@ -870,7 +870,8 @@ test.skip("#709 - applying observer on React.memo component", () => { render(, { wrapper: ErrorCatcher }) }) -test("#797 - replacing this.render should trigger a warning", () => { +// TODO explain skip +test.skip("#797 - replacing this.render should trigger a warning", () => { consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {}) @observer @@ -1032,8 +1033,69 @@ test("SSR works #3448", () => { enableStaticRendering(true) const { unmount, container } = render(app) expect(container).toHaveTextContent(":)") - enableStaticRendering(false) unmount() + enableStaticRendering(false) + + expect(consoleWarnMock).toMatchSnapshot() +}) + +test("#3492 should not cause warning by calling forceUpdate on uncommited components", async () => { + consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {}) + + const o = observable({ x: 0 }) + let aConstructorCount = 0 + let aMountCount = 0 + let aRenderCount = 0 + + @observer + class A extends React.Component { + constructor(props) { + super(props) + aConstructorCount++ + } + componentDidMount(): void { + aMountCount++ + } + render() { + aRenderCount++ + return ( + + + {o.x} + + ) + } + } + + class B extends React.Component { + render() { + return "B" + } + } + + const LazyA = React.lazy(() => Promise.resolve({ default: A })) + const LazyB = React.lazy(() => Promise.resolve({ default: B })) + function App() { + return ( + + + + ) + } + + const { unmount, container } = render() + + expect(container).toHaveTextContent("fallback") + await waitFor(() => expect(container).toHaveTextContent("B0")) + act(() => { + o.x++ + }) + expect(container).toHaveTextContent("B1") + // React throws away the first instance, therefore the mismatch + expect(aConstructorCount).toBe(2) + expect(aMountCount).toBe(1) + expect(aRenderCount).toBe(3) + unmount() expect(consoleWarnMock).toMatchSnapshot() }) diff --git a/packages/mobx-react/__tests__/symbol.test.tsx b/packages/mobx-react/__tests__/symbol.test.tsx deleted file mode 100644 index 4fe06ba45..000000000 --- a/packages/mobx-react/__tests__/symbol.test.tsx +++ /dev/null @@ -1,25 +0,0 @@ -import React from "react" -import { observer } from "../src" -import { render } from "@testing-library/react" -import { newSymbol } from "../src/utils/utils" - -// @ts-ignore -delete global.Symbol - -test("work without Symbol", () => { - const Component1 = observer( - class extends React.Component { - render() { - return null - } - } - ) - render() -}) - -test("cache newSymbol created Symbols", () => { - const symbol1 = newSymbol("name") - const symbol2 = newSymbol("name") - - expect(symbol1).toEqual(symbol2) -}) diff --git a/packages/mobx-react/package.json b/packages/mobx-react/package.json index 03f7de0d9..d797d6a8c 100644 --- a/packages/mobx-react/package.json +++ b/packages/mobx-react/package.json @@ -52,7 +52,8 @@ }, "devDependencies": { "mobx": "^6.7.0", - "mobx-react-lite": "^3.4.0" + "mobx-react-lite": "^3.4.0", + "expose-gc": "^1.0.0" }, "keywords": [ "mobx", diff --git a/packages/mobx-react/src/disposeOnUnmount.ts b/packages/mobx-react/src/disposeOnUnmount.ts index 409273d6f..16b673137 100644 --- a/packages/mobx-react/src/disposeOnUnmount.ts +++ b/packages/mobx-react/src/disposeOnUnmount.ts @@ -1,10 +1,13 @@ import React from "react" -import { patch, newSymbol } from "./utils/utils" +import { patch } from "./utils/utils" + +const reactMajorVersion = Number.parseInt(React.version.split(".")[0]) +let warnedAboutDisposeOnUnmountDeprecated = false type Disposer = () => void -const protoStoreKey = newSymbol("disposeOnUnmountProto") -const instStoreKey = newSymbol("disposeOnUnmountInst") +const protoStoreKey = Symbol("disposeOnUnmountProto") +const instStoreKey = Symbol("disposeOnUnmountInst") function runDisposersOnWillUnmount() { ;[...(this[protoStoreKey] || []), ...(this[instStoreKey] || [])].forEach(propKeyOrFunction => { @@ -17,12 +20,22 @@ function runDisposersOnWillUnmount() { }) } +/** + * @deprecated + */ export function disposeOnUnmount(target: React.Component, propertyKey: PropertyKey): void + +/** + * @deprecated + */ export function disposeOnUnmount>( target: React.Component, fn: TF ): TF +/** + * @deprecated + */ export function disposeOnUnmount( target: React.Component, propertyKeyOrFunction: PropertyKey | Disposer | Array @@ -31,6 +44,20 @@ export function disposeOnUnmount( return propertyKeyOrFunction.map(fn => disposeOnUnmount(target, fn)) } + if (!warnedAboutDisposeOnUnmountDeprecated) { + if (reactMajorVersion >= 18) { + console.error( + "[mobx-react] disposeOnUnmount is not compatible with React 18. Don't use it." + ) + } else { + console.warn( + "[mobx-react] disposeOnUnmount is deprecated. It won't work correctly with React 18 and higher." + ) + } + } + + warnedAboutDisposeOnUnmountDeprecated = true + const c = Object.getPrototypeOf(target).constructor const c2 = Object.getPrototypeOf(target.constructor) // Special case for react-hot-loader diff --git a/packages/mobx-react/src/index.ts b/packages/mobx-react/src/index.ts index 6bdd97929..65abc7ed7 100644 --- a/packages/mobx-react/src/index.ts +++ b/packages/mobx-react/src/index.ts @@ -1,8 +1,13 @@ import { observable } from "mobx" import { Component } from "react" -if (!Component) throw new Error("mobx-react requires React to be available") -if (!observable) throw new Error("mobx-react requires mobx to be available") +if (!Component) { + throw new Error("mobx-react requires React to be available") +} + +if (!observable) { + throw new Error("mobx-react requires mobx to be available") +} export { Observer, diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index 7a2c6be1a..c51a011bb 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -1,44 +1,84 @@ -import { PureComponent, Component } from "react" +import { PureComponent, Component, ComponentClass, ClassAttributes } from "react" import { createAtom, _allowStateChanges, Reaction, - $mobx, + //$mobx, // TODO _allowStateReadsStart, - _allowStateReadsEnd + _allowStateReadsEnd, + _getGlobalState, + IAtom } from "mobx" -import { isUsingStaticRendering } from "mobx-react-lite" +import { + isUsingStaticRendering, + _observerFinalizationRegistry as observerFinalizationRegistry +} from "mobx-react-lite" +import { shallowEqual, patch } from "./utils/utils" + +// TODO symbols + +const administrationSymbol = Symbol("ObserverAdministration") +//const mobxAdminProperty = $mobx // TODO +const isMobXReactObserverSymbol = Symbol("isMobXReactObserver") -import { newSymbol, shallowEqual, setHiddenProp, patch } from "./utils/utils" +type ObserverAdministration = { + reaction: Reaction | null // also serves as disposed flag + forceUpdate: Function | null + mounted: boolean // we could use forceUpdate as mounted flag + name: string + propsAtom: IAtom + stateAtom: IAtom + contextAtom: IAtom + props: any + state: any + context: any + // Setting this.props causes forceUpdate, because this.props is observable. + // forceUpdate sets this.props. + // This flag is used to avoid the loop. + isUpdating: boolean +} -const mobxAdminProperty = $mobx || "$mobx" // BC -const mobxObserverProperty = newSymbol("isMobXReactObserver") -const mobxIsUnmounted = newSymbol("isUnmounted") -const skipRenderKey = newSymbol("skipRender") -const isForcingUpdateKey = newSymbol("isForcingUpdate") +function getAdministration(component: Component): ObserverAdministration { + // We create administration lazily, because we can't patch constructor + // and the exact moment of initialization partially depends on React internals. + // At the time of writing this, the first thing invoked is one of the observable getter/setter (state/props/context). + return (component[administrationSymbol] ??= { + reaction: null, + mounted: false, + forceUpdate: null, + name: getDisplayName(component.constructor as ComponentClass), + state: undefined, + props: undefined, + context: undefined, + propsAtom: createAtom("props"), + stateAtom: createAtom("state"), + contextAtom: createAtom("context"), + isUpdating: false + }) +} export function makeClassComponentObserver( - componentClass: React.ComponentClass -): React.ComponentClass { - const target = componentClass.prototype + componentClass: ComponentClass +): ComponentClass { + const { prototype } = componentClass - if (componentClass[mobxObserverProperty]) { - const displayName = getDisplayName(target) + if (componentClass[isMobXReactObserverSymbol]) { + const displayName = getDisplayName(componentClass) console.warn( `The provided component class (${displayName}) has already been declared as an observer component.` ) } else { - componentClass[mobxObserverProperty] = true + componentClass[isMobXReactObserverSymbol] = true } - if (target.componentWillReact) { + if (prototype.componentWillReact) { throw new Error("The componentWillReact life-cycle event is no longer supported") } if (componentClass["__proto__"] !== PureComponent) { - if (!target.shouldComponentUpdate) { - target.shouldComponentUpdate = observerSCU - } else if (target.shouldComponentUpdate !== observerSCU) { + if (!prototype.shouldComponentUpdate) { + prototype.shouldComponentUpdate = observerSCU + } else if (prototype.shouldComponentUpdate !== observerSCU) { // n.b. unequal check, instead of existence check, as @observer might be on superclass as well throw new Error( "It is not allowed to use shouldComponentUpdate in observer based components." @@ -50,142 +90,156 @@ export function makeClassComponentObserver( // are defined inside the component, and which rely on state or props, re-compute if state or props change // (otherwise the computed wouldn't update and become stale on props change, since props are not observable) // However, this solution is not without it's own problems: https://github.com/mobxjs/mobx-react/issues?utf8=%E2%9C%93&q=is%3Aissue+label%3Aobservable-props-or-not+ - makeObservableProp(target, "props") - makeObservableProp(target, "state") - if (componentClass.contextType) { - makeObservableProp(target, "context") - } + Object.defineProperties(prototype, { + props: observablePropsDescriptor, + state: observableStateDescriptor, + context: observableContextDescriptor + }) - const originalRender = target.render + const originalRender = prototype.render if (typeof originalRender !== "function") { - const displayName = getDisplayName(target) + const displayName = getDisplayName(componentClass) throw new Error( `[mobx-react] class component (${displayName}) is missing \`render\` method.` + `\n\`observer\` requires \`render\` being a function defined on prototype.` + `\n\`render = () => {}\` or \`render = function() {}\` is not supported.` ) } - target.render = function () { - this.render = isUsingStaticRendering() - ? originalRender - : createReactiveRender.call(this, originalRender) + + prototype.render = function () { + Object.defineProperty(this, "render", { + // There is no safe way to replace render, therefore it's forbidden. + configurable: false, + writable: false, + value: isUsingStaticRendering() + ? originalRender + : createReactiveRender.call(this, originalRender) + }) return this.render() } - patch(target, "componentDidMount", function () { - this[mobxIsUnmounted] = false - if (!this.render[mobxAdminProperty]) { - // Reaction is re-created automatically during render, but a component can re-mount and skip render #3395. - // To re-create the reaction and re-subscribe to relevant observables we have to force an update. - Component.prototype.forceUpdate.call(this) + + patch(prototype, "componentDidMount", function () { + // `componentDidMount` may not be called at all. React can abandon the instance after `render`. + // That's why we use finalization registry to dispose reaction created during render. + // Happens with `` see #3492 + // + // `componentDidMount` can be called immediately after `componentWillUnmount` without calling `render` in between. + // Happens with ``see #3395. + // + // If `componentDidMount` is called, it's guaranteed to run synchronously with render (similary to `useLayoutEffect`). + // Therefore we don't have to worry about external (observable) state being updated before mount (no state version checking). + // + // Things may change: "In the future, React will provide a feature that lets components preserve state between unmounts" + + const admin = getAdministration(this) + + admin.mounted = true + + // Component instance committed, prevent reaction disposal. + observerFinalizationRegistry.unregister(admin) + + // We don't set forceUpdate before mount because it requires a reference to `this`, + // therefore `this` could NOT be garbage collected before mount, + // preventing reaction disposal by FinalizationRegistry and leading to memory leak. + // As an alternative we could have `admin.instanceRef = new WeakRef(this)`, but lets avoid it if possible. + admin.forceUpdate = () => this.forceUpdate() + + if (!admin.reaction) { + // 1. Instance was unmounted (reaction disposed) and immediately remounted without running render #3395. + // 2. Reaction was disposed by finalization registry before mount. Shouldn't ever happen for class components: + // `componentDidMount` runs synchronously after render, but our registry are deferred (can't run in between). + // In any case we lost subscriptions to observables, so we have to create new reaction and re-render to resubscribe. + // The reaction will be created lazily by following render. + admin.forceUpdate() } }) - patch(target, "componentWillUnmount", function () { + + patch(prototype, "componentWillUnmount", function () { if (isUsingStaticRendering()) { return } - - const reaction = this.render[mobxAdminProperty] - if (reaction) { - reaction.dispose() - // Forces reaction to be re-created on next render - this.render[mobxAdminProperty] = null - } else { - // Render may have been hot-swapped and/or overridden by a subclass. - const displayName = getDisplayName(this) - console.warn( - `The reactive render of an observer class component (${displayName}) - was overridden after MobX attached. This may result in a memory leak if the - overridden reactive render was not properly disposed.` - ) - } - - this[mobxIsUnmounted] = true + const admin = getAdministration(this) + admin.reaction?.dispose() + admin.reaction = null + admin.forceUpdate = null + admin.mounted = false }) + return componentClass } // Generates a friendly name for debugging -function getDisplayName(comp: any) { - return ( - comp.displayName || - comp.name || - (comp.constructor && (comp.constructor.displayName || comp.constructor.name)) || - "" - ) +function getDisplayName(componentClass: ComponentClass) { + return componentClass.displayName || componentClass.name || "" } function createReactiveRender(originalRender: any) { - /** - * If props are shallowly modified, react will render anyway, - * so atom.reportChanged() should not result in yet another re-render - */ - setHiddenProp(this, skipRenderKey, false) - /** - * forceUpdate will re-assign this.props. We don't want that to cause a loop, - * so detect these changes - */ - setHiddenProp(this, isForcingUpdateKey, false) - - const initialName = getDisplayName(this) const boundOriginalRender = originalRender.bind(this) - let isRenderingPending = false - - const createReaction = () => { - const reaction = new Reaction(`${initialName}.render()`, () => { - if (!isRenderingPending) { - // N.B. Getting here *before mounting* means that a component constructor has side effects (see the relevant test in misc.test.tsx) - // This unidiomatic React usage but React will correctly warn about this so we continue as usual - // See #85 / Pull #44 - isRenderingPending = true - if (this[mobxIsUnmounted] !== true) { - let hasError = true - try { - setHiddenProp(this, isForcingUpdateKey, true) - if (!this[skipRenderKey]) { - Component.prototype.forceUpdate.call(this) - } - hasError = false - } finally { - setHiddenProp(this, isForcingUpdateKey, false) - if (hasError) { - reaction.dispose() - // Forces reaction to be re-created on next render - this.render[mobxAdminProperty] = null - } - } - } - } - }) - reaction["reactComponent"] = this - return reaction - } + const admin = getAdministration(this) function reactiveRender() { - isRenderingPending = false - // Create reaction lazily to support re-mounting #3395 - const reaction = (reactiveRender[mobxAdminProperty] ??= createReaction()) - let exception: unknown = undefined - let rendering = undefined - reaction.track(() => { + if (!admin.reaction) { + // Create reaction lazily to support re-mounting #3395 + admin.reaction = createReaction(admin) + if (!admin.mounted) { + // React can abandon this instance and never call `componentDidMount`/`componentWillUnmount`, + // we have to make sure reaction will be disposed. + observerFinalizationRegistry.register(this, admin, this) + } + } + + let error: unknown = undefined + let renderResult = undefined + admin.reaction.track(() => { try { // TODO@major // Optimization: replace with _allowStateChangesStart/End (not available in mobx@6.0.0) - rendering = _allowStateChanges(false, boundOriginalRender) + renderResult = _allowStateChanges(false, boundOriginalRender) } catch (e) { - exception = e + error = e } }) - if (exception) { - throw exception + if (error) { + throw error } - return rendering + return renderResult } return reactiveRender } -function observerSCU(nextProps: React.ClassAttributes, nextState: any): boolean { +function createReaction(admin: ObserverAdministration) { + return new Reaction(`${admin.name}.render()`, () => { + if (admin.isUpdating) { + // Reaction is suppressed when setting new state/props/context, + // this is when component is already being updated. + return + } + + if (!admin.mounted) { + // This is neccessary to avoid react warning about calling forceUpdate on component that isn't mounted yet. + // This happens when component is abandoned after render - our reaction is already created and reacts to changes. + // Due to the synchronous nature of `componenDidMount`, we don't have to worry that component could eventually mount and require update. + return + } + + try { + // forceUpdate sets new `props`, since we made it observable, they would `reportChanged`, causing a loop. + //admin.observablesSuppressed = true + admin.isUpdating = true + admin.forceUpdate?.() + } catch (error) { + admin.reaction?.dispose() + admin.reaction = null + } finally { + admin.isUpdating = false + //admin.observablesSuppressed = false + } + }) +} + +function observerSCU(nextProps: ClassAttributes, nextState: any): boolean { if (isUsingStaticRendering()) { console.warn( "[mobx-react] It seems that a re-rendering of a React component is triggered while in static (server-side) mode. Please make sure components are rendered only once server-side." @@ -202,45 +256,41 @@ function observerSCU(nextProps: React.ClassAttributes, nextState: any): boo return !shallowEqual(this.props, nextProps) } -function makeObservableProp(target: any, propName: string): void { - const valueHolderKey = newSymbol(`reactProp_${propName}_valueHolder`) - const atomHolderKey = newSymbol(`reactProp_${propName}_atomHolder`) - function getAtom() { - if (!this[atomHolderKey]) { - setHiddenProp(this, atomHolderKey, createAtom("reactive " + propName)) - } - return this[atomHolderKey] - } - Object.defineProperty(target, propName, { +function createObservablePropDescriptor(key: "props" | "state" | "context") { + const atomKey = `${key}Atom` + return { configurable: true, enumerable: true, - get: function () { - let prevReadState = false + get() { + const admin = getAdministration(this) - // Why this check? BC? - // @ts-expect-error - if (_allowStateReadsStart && _allowStateReadsEnd) { - prevReadState = _allowStateReadsStart(true) - } - getAtom.call(this).reportObserved() + let prevReadState = _allowStateReadsStart(true) - // Why this check? BC? - // @ts-expect-error - if (_allowStateReadsStart && _allowStateReadsEnd) { - _allowStateReadsEnd(prevReadState) - } + admin[atomKey].reportObserved() + + _allowStateReadsEnd(prevReadState) - return this[valueHolderKey] + return admin[key] }, - set: function set(v) { - if (!this[isForcingUpdateKey] && !shallowEqual(this[valueHolderKey], v)) { - setHiddenProp(this, valueHolderKey, v) - setHiddenProp(this, skipRenderKey, true) - getAtom.call(this).reportChanged() - setHiddenProp(this, skipRenderKey, false) + set(value) { + const admin = getAdministration(this) + // Observables are suppressed if forceUpdate was issues by reaction + // TODO !admin.observablesSuppressed + if (!admin.isUpdating && !shallowEqual(admin[key], value)) { + admin[key] = value + // This notifies all observers including our component, + // but we don't want to cause `forceUpdate`, because component is already updating, + // therefore supress component reaction. + admin.isUpdating = true + admin[atomKey].reportChanged() + admin.isUpdating = false } else { - setHiddenProp(this, valueHolderKey, v) + admin[key] = value } } - }) + } } + +const observablePropsDescriptor = createObservablePropDescriptor("props") +const observableStateDescriptor = createObservablePropDescriptor("state") +const observableContextDescriptor = createObservablePropDescriptor("context") diff --git a/packages/mobx-react/src/utils/utils.ts b/packages/mobx-react/src/utils/utils.ts index c79efa456..9571d3b6f 100644 --- a/packages/mobx-react/src/utils/utils.ts +++ b/packages/mobx-react/src/utils/utils.ts @@ -1,21 +1,3 @@ -let symbolId = 0 -function createSymbol(name: string): symbol | string { - if (typeof Symbol === "function") { - return Symbol(name) - } - const symbol = `__$mobx-react ${name} (${symbolId})` - symbolId++ - return symbol -} - -const createdSymbols = {} -export function newSymbol(name: string): symbol | string { - if (!createdSymbols[name]) { - createdSymbols[name] = createSymbol(name) - } - return createdSymbols[name] -} - export function shallowEqual(objA: any, objB: any): boolean { //From: https://github.com/facebook/fbjs/blob/c69904a511b900266935168223063dd8772dfc40/packages/fbjs/src/core/shallowEqual.js if (is(objA, objB)) { @@ -96,8 +78,8 @@ export function setHiddenProp(target: object, prop: any, value: any): void { * Utilities for patching componentWillUnmount, to make sure @disposeOnUnmount works correctly icm with user defined hooks * and the handler provided by mobx-react */ -const mobxMixins = newSymbol("patchMixins") -const mobxPatchedDefinition = newSymbol("patchedDefinition") +const mobxMixins = Symbol("patchMixins") +const mobxPatchedDefinition = Symbol("patchedDefinition") export interface Mixins extends Record { locks: number @@ -175,6 +157,7 @@ function createDefinition( let wrappedFunc = wrapFunction(originalMethod, mixins) return { + // @ts-ignore [mobxPatchedDefinition]: true, get: function () { return wrappedFunc diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index bd01cfe7d..f27979b7d 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -152,7 +152,7 @@ export class MobXGlobals { safeDescriptors = true /** - * Changes with each state update, used by useMutableSyncStore + * Changes with each state update, used by useSyncExternalStore */ stateVersion = Symbol() } From 951b7e855ab8e05d6afe443adb3a4a55503ad39f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 11:01:01 +0100 Subject: [PATCH 05/38] fix test --- packages/mobx-react/__tests__/inject.test.tsx | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/mobx-react/__tests__/inject.test.tsx b/packages/mobx-react/__tests__/inject.test.tsx index b264a19b9..c2fdeaf85 100644 --- a/packages/mobx-react/__tests__/inject.test.tsx +++ b/packages/mobx-react/__tests__/inject.test.tsx @@ -403,8 +403,7 @@ describe("inject based context", () => { expect(container).toHaveTextContent("Veria") }) - // TODO - test.skip("using a custom injector is not too reactive", () => { + test("using a custom injector is not too reactive", () => { let listRender = 0 let itemRender = 0 let injectRender = 0 @@ -499,12 +498,21 @@ describe("inject based context", () => { expect(injectRender).toBe(6) expect(itemRender).toBe(6) - container.querySelectorAll(".hl_ItemB").forEach((e: Element) => (e as HTMLElement).click()) + act(() => { + container + .querySelectorAll(".hl_ItemB") + .forEach((e: Element) => (e as HTMLElement).click()) + }) + expect(listRender).toBe(1) expect(injectRender).toBe(12) // ideally, 7 expect(itemRender).toBe(7) + act(() => { + container + .querySelectorAll(".hl_ItemF") + .forEach((e: Element) => (e as HTMLElement).click()) + }) - container.querySelectorAll(".hl_ItemF").forEach((e: Element) => (e as HTMLElement).click()) expect(listRender).toBe(1) expect(injectRender).toBe(18) // ideally, 9 expect(itemRender).toBe(9) From abfaa7ee91a4d46f0298f5099bef6946cbd90aab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 11:18:32 +0100 Subject: [PATCH 06/38] wip --- packages/mobx-react/__tests__/observer.test.tsx | 2 +- packages/mobx-react/src/observerClass.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index f6a64154e..619b1ff6e 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -870,7 +870,7 @@ test.skip("#709 - applying observer on React.memo component", () => { render(, { wrapper: ErrorCatcher }) }) -// TODO explain skip +// this.render is made non-configurable, therefore can't be replaced test.skip("#797 - replacing this.render should trigger a warning", () => { consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {}) diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index c51a011bb..826f9793b 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -274,8 +274,8 @@ function createObservablePropDescriptor(key: "props" | "state" | "context") { }, set(value) { const admin = getAdministration(this) - // Observables are suppressed if forceUpdate was issues by reaction - // TODO !admin.observablesSuppressed + // forceUpdate issued by reaction sets new props. + // It sets isUpdating to true to prevent loop. if (!admin.isUpdating && !shallowEqual(admin[key], value)) { admin[key] = value // This notifies all observers including our component, From b5c26cae72bf902becfc4949a0fc8842f181005b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 12:47:55 +0100 Subject: [PATCH 07/38] refactor --- packages/mobx-react-lite/src/observer.ts | 14 -- packages/mobx-react-lite/src/useObserver.ts | 251 +++++--------------- 2 files changed, 59 insertions(+), 206 deletions(-) diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index 1f71abda5..cd019c492 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/packages/mobx-react-lite/src/observer.ts @@ -165,17 +165,3 @@ function copyStaticProperties(base: any, target: any) { } }) } - -// TODO rename to ObserverAdministration -// Make sure NOT to store `this` or `instanceRef` (not even as part of a closure!) on this object, -// otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry. -export type ObserverInstance = { - reaction: Reaction | null // also works as disposed flag - forceUpdate: Function | null // also works as mounted flag - // BC: we will use local state version if global isn't available. - // It should behave as previous implementation - tearing is still present, - // because there is no cross component synchronization, - // but we can use `useExternalSyncStore` API. - stateVersion: any - componentName: string // TODO just name -} diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index fd7bfbc4b..b31c64f8d 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -1,233 +1,100 @@ import { Reaction, _getGlobalState } from "mobx" import React from "react" import { printDebugValue } from "./utils/printDebugValue" -import { - addReactionToTrack, - IReactionTracking, - recordReactionAsCommitted -} from "./utils/reactionCleanupTracking" import { isUsingStaticRendering } from "./staticRendering" -import { ObserverInstance } from "./observer" import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" -// TODO rename, it's only used for reaction -function observerComponentNameFor(baseComponentName: string) { - return `observer${baseComponentName}` +// Do not store `admRef` (even as part of a closure!) on this object, +// otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry. +type ObserverAdministration = { + reaction: Reaction | null // also serves as disposed flag + forceUpdate: Function | null // also serves as mounted flag + // BC: we will use local state version if global isn't available. + // It should behave as previous implementation - tearing is still present, + // because there is no cross component synchronization, + // but we can use `useExternalSyncStore` API. + stateVersion: any + name: string + // These don't depend on state/props, therefore we can keep them here instead of `useCallback` + subscribe: Parameters[0] + getSnapshot: Parameters[1] } const mobxGlobalState = _getGlobalState() // BC -const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined" // TODO -//const globalStateVersionIsAvailable = false; // TODO +const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined" -/** - * We use class to make it easier to detect in heap snapshots by name - */ -class ObjectToBeRetainedByReact {} - -function objectToBeRetainedByReactFactory() { - return new ObjectToBeRetainedByReact() -} - -export function useLegacyObserver(fn: () => T, baseComponentName: string = "observed"): T { - if (isUsingStaticRendering()) { - return fn() - } - - const [objectRetainedByReact] = React.useState(objectToBeRetainedByReactFactory) - // Force update, see #2982 - const [, setState] = React.useState() - const forceUpdate = () => setState([] as any) - - // StrictMode/ConcurrentMode/Suspense may mean that our component is - // rendered and abandoned multiple times, so we need to track leaked - // Reactions. - const reactionTrackingRef = React.useRef(null) - - if (!reactionTrackingRef.current) { - // First render for this component (or first time since a previous - // reaction from an abandoned render was disposed). - - const newReaction = new Reaction(observerComponentNameFor(baseComponentName), () => { - // Observable has changed, meaning we want to re-render - // BUT if we're a component that hasn't yet got to the useEffect() - // stage, we might be a component that _started_ to render, but - // got dropped, and we don't want to make state changes then. - // (It triggers warnings in StrictMode, for a start.) - if (trackingData.mounted) { - // We have reached useEffect(), so we're mounted, and can trigger an update - forceUpdate() - } else { - // We haven't yet reached useEffect(), so we'll need to trigger a re-render - // when (and if) useEffect() arrives. - trackingData.changedBeforeMount = true - } - }) - - const trackingData = addReactionToTrack( - reactionTrackingRef, - newReaction, - objectRetainedByReact - ) - } - - const { reaction } = reactionTrackingRef.current! - React.useDebugValue(reaction, printDebugValue) - - React.useEffect(() => { - // Called on first mount only - recordReactionAsCommitted(reactionTrackingRef) - - if (reactionTrackingRef.current) { - // Great. We've already got our reaction from our render; - // all we need to do is to record that it's now mounted, - // to allow future observable changes to trigger re-renders - reactionTrackingRef.current.mounted = true - // Got a change before first mount, force an update - if (reactionTrackingRef.current.changedBeforeMount) { - reactionTrackingRef.current.changedBeforeMount = false - forceUpdate() - } - } else { - // The reaction we set up in our render has been disposed. - // This can be due to bad timings of renderings, e.g. our - // component was paused for a _very_ long time, and our - // reaction got cleaned up - - // Re-create the reaction - reactionTrackingRef.current = { - reaction: new Reaction(observerComponentNameFor(baseComponentName), () => { - // We've definitely already been mounted at this point - forceUpdate() - }), - mounted: true, - changedBeforeMount: false, - cleanAt: Infinity - } - forceUpdate() - } - - return () => { - reactionTrackingRef.current!.reaction.dispose() - reactionTrackingRef.current = null - } - }, []) - - // render the original component, but have the - // reaction track the observables, so that rendering - // can be invalidated (see above) once a dependency changes - let rendering!: T - let exception - reaction.track(() => { - try { - rendering = fn() - } catch (e) { - exception = e - } - }) - - if (exception) { - throw exception // re-throw any exceptions caught during rendering - } - - return rendering -} - -function createReaction(instance: ObserverInstance) { - instance.reaction = new Reaction(observerComponentNameFor(instance.componentName), () => { +function createReaction(adm: ObserverAdministration) { + adm.reaction = new Reaction(`observer${adm.name}`, () => { if (!globalStateVersionIsAvailable) { // BC - instance.stateVersion = Symbol() + adm.stateVersion = Symbol() } // Force update won't be avaliable until the component "mounts". // If state changes in between initial render and mount, // `useExternalSyncStore` should handle that by checking the state version and issuing update. - instance.forceUpdate?.() + adm.forceUpdate?.() }) } -/* -function scheduleReactionDisposal(instance: ObserverInstance) {} - -function cancelReactionDisposal(instance: ObserverInstance) {} -*/ -function disposeReaction(instance: ObserverInstance) { - instance.reaction?.dispose() - instance.reaction = null -} - -// reset/tearDown/suspend/detach -function dispose(instance: ObserverInstance) { - instance.forceUpdate = null - disposeReaction(instance) -} - export function useObserver(render: () => T, baseComponentName: string = "observed"): T { if (isUsingStaticRendering()) { return render() } - // StrictMode/ConcurrentMode/Suspense may mean that our component is - // rendered and abandoned multiple times, so we need to track leaked - // Reactions. - const instanceRef = React.useRef(null) + const admRef = React.useRef(null) - if (!instanceRef.current) { - const instance: ObserverInstance = { + if (!admRef.current) { + const adm: ObserverAdministration = { reaction: null, forceUpdate: null, stateVersion: Symbol(), - componentName: baseComponentName - } - // Opt: instead of useMemo we keep subscribe/getSnapshot on instance - // @ts-ignore - instance.subscribe = (onStoreChange: () => void) => { - // Do NOT access instanceRef here! - //console.log('SUBSCRIBE'); - observerFinalizationRegistry.unregister(instance) - //const instance = instanceRef.current! - instance.forceUpdate = onStoreChange - if (!instance.reaction) { - // TODO probably we can create reaction lazily in render and therefore simply issue forceUpdate, - // but keep in mind we don't want to use finalization registry at this point (we are mounted). - createReaction(instance) - // We've lost our reaction and therefore all the subscriptions. - // We have to schedule re-render to recreate subscriptions, - // even if state did not change. - //console.log('REACTION LOST, FORCING UPDATE'); - instance.forceUpdate() - } - - return () => { - // Do NOT access instanceRef here! - //console.log('UNSUBSCRIBE'); - //dispose(instanceRef.current!) - dispose(instance) + name: baseComponentName, + subscribe(onStoreChange: () => void) { + // Do NOT access admRef here! + observerFinalizationRegistry.unregister(adm) + adm.forceUpdate = onStoreChange + if (!adm.reaction) { + // We've lost our reaction and therefore all subscriptions. + // We have to recreate reaction and schedule re-render to recreate subscriptions, + // even if state did not change. + createReaction(adm) + adm.forceUpdate() + } + + return () => { + // Do NOT access admRef here! + adm.forceUpdate = null + adm.reaction?.dispose() + adm.reaction = null + } + }, + getSnapshot() { + // Do NOT access admRef here! + return globalStateVersionIsAvailable + ? mobxGlobalState.stateVersion + : adm.stateVersion } } - // @ts-ignore - instance.getSnapshot = () => - globalStateVersionIsAvailable - ? mobxGlobalState.stateVersion - : //: instanceRef.current?.stateVersion - instance.stateVersion - createReaction(instance) + createReaction(adm) + + admRef.current = adm - instanceRef.current = instance - observerFinalizationRegistry.register(instanceRef, instance, instance) + // StrictMode/ConcurrentMode/Suspense may mean that our component is + // rendered and abandoned multiple times, so we need to track leaked + // Reactions. + observerFinalizationRegistry.register(admRef, adm, adm) } - const instance = instanceRef.current! - React.useDebugValue(instance.reaction!, printDebugValue) + const adm = admRef.current! + React.useDebugValue(adm.reaction!, printDebugValue) React.useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. - // @ts-ignore - instance.subscribe, - // @ts-ignore - instance.getSnapshot + adm.subscribe, + adm.getSnapshot ) // render the original component, but have the @@ -235,7 +102,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs // can be invalidated (see above) once a dependency changes let renderResult!: T let exception - instance.reaction!.track(() => { + adm.reaction!.track(() => { try { renderResult = render() } catch (e) { From dfb75fdd0d7c59d8a15f042a1613deb1ce6cc960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 12:54:49 +0100 Subject: [PATCH 08/38] refactor --- packages/mobx-react-lite/src/observer.ts | 1 - .../src/utils/observerFinalizationRegistry.ts | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index cd019c492..dee0bb2e5 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/packages/mobx-react-lite/src/observer.ts @@ -1,4 +1,3 @@ -import { Reaction } from "mobx" import { forwardRef, memo } from "react" import { isUsingStaticRendering } from "./staticRendering" diff --git a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts index 5b9a34763..e139ac210 100644 --- a/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts @@ -1,13 +1,8 @@ import { Reaction } from "mobx" -import { - UniversalFinalizationRegistry, - // @ts-ignore - FinalizationRegistryType -} from "./UniversalFinalizationRegistry" +import { UniversalFinalizationRegistry } from "./UniversalFinalizationRegistry" export const observerFinalizationRegistry = new UniversalFinalizationRegistry( (adm: { reaction: Reaction | null }) => { - //console.log('FINALIZING'); // TODO adm.reaction?.dispose() adm.reaction = null } From 50062639b8a31a950769848da33b96a239741bf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 13:09:14 +0100 Subject: [PATCH 09/38] refactors --- ...rentModeUsingFinalizationRegistry.test.tsx | 15 +- ...trictAndConcurrentModeUsingTimers.test.tsx | 187 ------------------ 2 files changed, 7 insertions(+), 195 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index 584685550..cd9a35fcc 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -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) 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 diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx index 4bc8701f2..d7250ccb4 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingTimers.test.tsx @@ -2,21 +2,11 @@ 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" import { REGISTRY_FINALIZE_AFTER, REGISTRY_SWEEP_INTERVAL } from "../src/utils/UniversalFinalizationRegistry" - import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry" @@ -25,183 +15,6 @@ expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegist const registry = observerFinalizationRegistry as TimerBasedFinalizationRegistry afterEach(cleanup) -/* -test("uncommitted components should not leak observations", async () => { - resetCleanupScheduleForTests() - - // Unfortunately, Jest fake timers don't mock out Date.now, so we fake - // that out in parallel to Jest useFakeTimers - let fakeNow = Date.now() - jest.useFakeTimers() - jest.spyOn(Date, "now").mockImplementation(() => fakeNow) - - const store = mobx.observable({ count1: 0, count2: 0 }) - - // Track whether counts are observed - let count1IsObserved = false - let count2IsObserved = false - mobx.onBecomeObserved(store, "count1", () => (count1IsObserved = true)) - mobx.onBecomeUnobserved(store, "count1", () => (count1IsObserved = false)) - mobx.onBecomeObserved(store, "count2", () => (count2IsObserved = true)) - mobx.onBecomeUnobserved(store, "count2", () => (count2IsObserved = false)) - - const TestComponent1 = () => useObserver(() =>
{store.count1}
) - const TestComponent2 = () => useObserver(() =>
{store.count2}
) - - // Render, then remove only #2 - const rendering = render( - - - - - ) - rendering.rerender( - - - - ) - - // Allow any reaction-disposal cleanup timers to run - const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS) - fakeNow += skip - jest.advanceTimersByTime(skip) - - // count1 should still be being observed by Component1, - // but count2 should have had its reaction cleaned up. - expect(count1IsObserved).toBeTruthy() - expect(count2IsObserved).toBeFalsy() -}) - -test("cleanup timer should not clean up recently-pended reactions", () => { - // If we're not careful with timings, it's possible to get the - // following scenario: - // 1. Component instance A is being created; it renders, we put its reaction R1 into the cleanup list - // 2. Strict/Concurrent mode causes that render to be thrown away - // 3. Component instance A is being created; it renders, we put its reaction R2 into the cleanup list - // 4. The MobX reaction timer from 5 seconds ago kicks in and cleans up all reactions from uncommitted - // components, including R1 and R2 - // 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() - - // Unfortunately, Jest fake timers don't mock out Date.now, so we fake - // that out in parallel to Jest useFakeTimers - const fakeNow = Date.now() - jest.useFakeTimers() - jest.spyOn(Date, "now").mockImplementation(() => fakeNow) - - const store = mobx.observable({ count: 0 }) - - // Track whether the count is observed - let countIsObserved = false - mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) - mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) - - const TestComponent1 = () => useObserver(() =>
{store.count}
) - - const rendering = render( - // We use StrictMode here, but it would be helpful to switch this to use real - // concurrent mode: we don't have a true async render right now so this test - // isn't as thorough as it could be. - - - - ) - - // We need to trigger our cleanup timer to run. We can't do this simply - // by running all jest's faked timers as that would allow the scheduled - // `useEffect` calls to run, and we want to simulate our cleanup timer - // getting in between those stages. - - // 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() - - // Advance time enough to allow any timer-queued effects to run - jest.advanceTimersByTime(500) - - // Now allow the useEffect calls to run to completion. - act(() => { - // no-op, but triggers effect flushing - }) - - // count should still be observed - expect(countIsObserved).toBeTruthy() -}) - -// TODO: MWE: disabled during React 18 migration, not sure how to express it icmw with testing-lib, -// and using new React renderRoot will fail icmw JSDOM -test.skip("component should recreate reaction if necessary", () => { - // There _may_ be very strange cases where the reaction gets tidied up - // but is actually still needed. This _really_ shouldn't happen. - // e.g. if we're using Suspense and the component starts to render, - // but then gets paused for 60 seconds, and then comes back to life. - // With the implementation of React at the time of writing this, React - // will actually fully re-render that component (discarding previous - // hook slots) before going ahead with a commit, but it's unwise - // to depend on such an implementation detail. So we must cope with - // the component having had its reaction tidied and still going on to - // be committed. In that case we recreate the reaction and force - // an update. - - // This unit test attempts to replicate that scenario: - - resetCleanupScheduleForTests() - - // Unfortunately, Jest fake timers don't mock out Date.now, so we fake - // that out in parallel to Jest useFakeTimers - let fakeNow = Date.now() - jest.useFakeTimers() - jest.spyOn(Date, "now").mockImplementation(() => fakeNow) - - const store = mobx.observable({ count: 0 }) - - // Track whether the count is observed - let countIsObserved = false - mobx.onBecomeObserved(store, "count", () => (countIsObserved = true)) - mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false)) - - const TestComponent1 = () => useObserver(() =>
{store.count}
) - - const rendering = render( - - - - ) - - // We need to trigger our cleanup timer to run. We don't want - // to allow Jest's effects to run, however: we want to simulate the - // case where the component is rendered, then the reaction gets cleaned up, - // and _then_ the component commits. - - // Force everything to be disposed. - const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS) - fakeNow += skip - forceCleanupTimerToRunNowForTests() - - // The reaction should have been cleaned up. - expect(countIsObserved).toBeFalsy() - - // Whilst nobody's looking, change the observable value - store.count = 42 - - // Now allow the useEffect calls to run to completion, - // re-awakening the component. - jest.advanceTimersByTime(500) - act(() => { - // no-op, but triggers effect flushing - }) - - // count should be observed once more. - expect(countIsObserved).toBeTruthy() - // and the component should have rendered enough to - // show the latest value, which was set whilst it - // wasn't even looking. - expect(rendering.baseElement.textContent).toContain("42") -}) -*/ test("uncommitted components should not leak observations", async () => { registry.finalizeAllImmediately() From 229fc05a22d14c60ec2d507224375b92cc815fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 13:17:36 +0100 Subject: [PATCH 10/38] refactors --- packages/mobx-react-lite/src/index.ts | 3 +- .../src/utils/FinalizationRegistryWrapper.ts | 12 -- .../utils/UniversalFinalizationRegistry.ts | 1 + ...leanupTrackingUsingFinalizationRegister.ts | 57 -------- ...createTimerBasedReactionCleanupTracking.ts | 122 ------------------ .../src/utils/reactionCleanupTracking.ts | 20 --- .../utils/reactionCleanupTrackingCommon.ts | 72 ----------- 7 files changed, 3 insertions(+), 284 deletions(-) delete mode 100644 packages/mobx-react-lite/src/utils/FinalizationRegistryWrapper.ts delete mode 100644 packages/mobx-react-lite/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts delete mode 100644 packages/mobx-react-lite/src/utils/createTimerBasedReactionCleanupTracking.ts delete mode 100644 packages/mobx-react-lite/src/utils/reactionCleanupTracking.ts delete mode 100644 packages/mobx-react-lite/src/utils/reactionCleanupTrackingCommon.ts diff --git a/packages/mobx-react-lite/src/index.ts b/packages/mobx-react-lite/src/index.ts index b62334f18..761f92239 100644 --- a/packages/mobx-react-lite/src/index.ts +++ b/packages/mobx-react-lite/src/index.ts @@ -6,7 +6,7 @@ import { useDeprecated } from "./utils/utils" import { useObserver as useObserverOriginal } from "./useObserver" import { enableStaticRendering } from "./staticRendering" import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" -export { observerFinalizationRegistry as _observerFinalizationRegistry } + observerBatching(batch) export { isUsingStaticRendering, enableStaticRendering } from "./staticRendering" @@ -16,6 +16,7 @@ export { useLocalObservable } from "./useLocalObservable" export { useLocalStore } from "./useLocalStore" export { useAsObservableSource } from "./useAsObservableSource" +export { observerFinalizationRegistry as _observerFinalizationRegistry } export const clearTimes = observerFinalizationRegistry["finalizeAllImmediately"] ?? (() => {}) export function useObserver(fn: () => T, baseComponentName: string = "observed"): T { diff --git a/packages/mobx-react-lite/src/utils/FinalizationRegistryWrapper.ts b/packages/mobx-react-lite/src/utils/FinalizationRegistryWrapper.ts deleted file mode 100644 index d11006303..000000000 --- a/packages/mobx-react-lite/src/utils/FinalizationRegistryWrapper.ts +++ /dev/null @@ -1,12 +0,0 @@ -declare class FinalizationRegistryType { - constructor(cleanup: (cleanupToken: T) => void) - register(object: object, cleanupToken: T, unregisterToken?: object): void - unregister(unregisterToken: object): void -} - -declare const FinalizationRegistry: typeof FinalizationRegistryType | undefined - -const FinalizationRegistryLocal = - typeof FinalizationRegistry === "undefined" ? undefined : FinalizationRegistry - -export { FinalizationRegistryLocal as FinalizationRegistry } diff --git a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts index d361ccc53..12d8032a8 100644 --- a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts @@ -15,6 +15,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy 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, diff --git a/packages/mobx-react-lite/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts b/packages/mobx-react-lite/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts deleted file mode 100644 index 44f20382c..000000000 --- a/packages/mobx-react-lite/src/utils/createReactionCleanupTrackingUsingFinalizationRegister.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { FinalizationRegistry as FinalizationRegistryMaybeUndefined } from "./FinalizationRegistryWrapper" -import { Reaction } from "mobx" -import { - ReactionCleanupTracking, - IReactionTracking, - createTrackingData -} from "./reactionCleanupTrackingCommon" - -/** - * FinalizationRegistry-based uncommitted reaction cleanup - */ -export function createReactionCleanupTrackingUsingFinalizationRegister( - FinalizationRegistry: NonNullable -): ReactionCleanupTracking { - const cleanupTokenToReactionTrackingMap = new Map() - let globalCleanupTokensCounter = 1 - - const registry = new FinalizationRegistry(function cleanupFunction(token: number) { - const trackedReaction = cleanupTokenToReactionTrackingMap.get(token) - if (trackedReaction) { - trackedReaction.reaction.dispose() - cleanupTokenToReactionTrackingMap.delete(token) - } - }) - - return { - addReactionToTrack( - reactionTrackingRef: React.MutableRefObject, - reaction: Reaction, - objectRetainedByReact: object - ) { - const token = globalCleanupTokensCounter++ - - registry.register(objectRetainedByReact, token, reactionTrackingRef) - reactionTrackingRef.current = createTrackingData(reaction) - reactionTrackingRef.current.finalizationRegistryCleanupToken = token - cleanupTokenToReactionTrackingMap.set(token, reactionTrackingRef.current) - - return reactionTrackingRef.current - }, - recordReactionAsCommitted(reactionRef: React.MutableRefObject) { - registry.unregister(reactionRef) - - if (reactionRef.current && reactionRef.current.finalizationRegistryCleanupToken) { - cleanupTokenToReactionTrackingMap.delete( - reactionRef.current.finalizationRegistryCleanupToken - ) - } - }, - forceCleanupTimerToRunNowForTests() { - // When FinalizationRegistry in use, this this is no-op - }, - resetCleanupScheduleForTests() { - // When FinalizationRegistry in use, this this is no-op - } - } -} diff --git a/packages/mobx-react-lite/src/utils/createTimerBasedReactionCleanupTracking.ts b/packages/mobx-react-lite/src/utils/createTimerBasedReactionCleanupTracking.ts deleted file mode 100644 index cb00c7b2c..000000000 --- a/packages/mobx-react-lite/src/utils/createTimerBasedReactionCleanupTracking.ts +++ /dev/null @@ -1,122 +0,0 @@ -import { Reaction } from "mobx" -import { - ReactionCleanupTracking, - IReactionTracking, - CLEANUP_TIMER_LOOP_MILLIS, - createTrackingData -} from "./reactionCleanupTrackingCommon" - -/** - * timers, gc-style, uncommitted reaction cleanup - */ -export function createTimerBasedReactionCleanupTracking(): ReactionCleanupTracking { - /** - * Reactions created by components that have yet to be fully mounted. - */ - const uncommittedReactionRefs: Set> = new Set() - - /** - * Latest 'uncommitted reactions' cleanup timer handle. - */ - let reactionCleanupHandle: ReturnType | undefined - - /* istanbul ignore next */ - /** - * Only to be used by test functions; do not export outside of mobx-react-lite - */ - function forceCleanupTimerToRunNowForTests() { - // This allows us to control the execution of the cleanup timer - // to force it to run at awkward times in unit tests. - if (reactionCleanupHandle) { - clearTimeout(reactionCleanupHandle) - cleanUncommittedReactions() - } - } - - /* istanbul ignore next */ - function resetCleanupScheduleForTests() { - if (uncommittedReactionRefs.size > 0) { - for (const ref of uncommittedReactionRefs) { - const tracking = ref.current - if (tracking) { - tracking.reaction.dispose() - ref.current = null - } - } - uncommittedReactionRefs.clear() - } - - if (reactionCleanupHandle) { - clearTimeout(reactionCleanupHandle) - reactionCleanupHandle = undefined - } - } - - function ensureCleanupTimerRunning() { - if (reactionCleanupHandle === undefined) { - reactionCleanupHandle = setTimeout(cleanUncommittedReactions, CLEANUP_TIMER_LOOP_MILLIS) - } - } - - function scheduleCleanupOfReactionIfLeaked( - ref: React.MutableRefObject - ) { - uncommittedReactionRefs.add(ref) - - ensureCleanupTimerRunning() - } - - function recordReactionAsCommitted( - reactionRef: React.MutableRefObject - ) { - uncommittedReactionRefs.delete(reactionRef) - } - - /** - * Run by the cleanup timer to dispose any outstanding reactions - */ - function cleanUncommittedReactions() { - reactionCleanupHandle = undefined - - // Loop through all the candidate leaked reactions; those older - // than CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS get tidied. - - const now = Date.now() - uncommittedReactionRefs.forEach(ref => { - const tracking = ref.current - if (tracking) { - if (now >= tracking.cleanAt) { - // It's time to tidy up this leaked reaction. - tracking.reaction.dispose() - ref.current = null - uncommittedReactionRefs.delete(ref) - } - } - }) - - if (uncommittedReactionRefs.size > 0) { - // We've just finished a round of cleanups but there are still - // some leak candidates outstanding. - ensureCleanupTimerRunning() - } - } - - return { - addReactionToTrack( - reactionTrackingRef: React.MutableRefObject, - reaction: Reaction, - /** - * On timer based implementation we don't really need this object, - * but we keep the same api - */ - objectRetainedByReact: unknown - ) { - reactionTrackingRef.current = createTrackingData(reaction) - scheduleCleanupOfReactionIfLeaked(reactionTrackingRef) - return reactionTrackingRef.current - }, - recordReactionAsCommitted, - forceCleanupTimerToRunNowForTests, - resetCleanupScheduleForTests - } -} diff --git a/packages/mobx-react-lite/src/utils/reactionCleanupTracking.ts b/packages/mobx-react-lite/src/utils/reactionCleanupTracking.ts deleted file mode 100644 index 1aeffbd33..000000000 --- a/packages/mobx-react-lite/src/utils/reactionCleanupTracking.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { FinalizationRegistry as FinalizationRegistryMaybeUndefined } from "./FinalizationRegistryWrapper" -import { createReactionCleanupTrackingUsingFinalizationRegister } from "./createReactionCleanupTrackingUsingFinalizationRegister" -import { createTimerBasedReactionCleanupTracking } from "./createTimerBasedReactionCleanupTracking" -export { IReactionTracking } from "./reactionCleanupTrackingCommon" - -const { - addReactionToTrack, - recordReactionAsCommitted, - resetCleanupScheduleForTests, - forceCleanupTimerToRunNowForTests -} = FinalizationRegistryMaybeUndefined - ? createReactionCleanupTrackingUsingFinalizationRegister(FinalizationRegistryMaybeUndefined) - : createTimerBasedReactionCleanupTracking() - -export { - addReactionToTrack, - recordReactionAsCommitted, - resetCleanupScheduleForTests, - forceCleanupTimerToRunNowForTests -} diff --git a/packages/mobx-react-lite/src/utils/reactionCleanupTrackingCommon.ts b/packages/mobx-react-lite/src/utils/reactionCleanupTrackingCommon.ts deleted file mode 100644 index e0aa149ff..000000000 --- a/packages/mobx-react-lite/src/utils/reactionCleanupTrackingCommon.ts +++ /dev/null @@ -1,72 +0,0 @@ -import { Reaction } from "mobx" - -export function createTrackingData(reaction: Reaction) { - const trackingData: IReactionTracking = { - reaction, - mounted: false, - changedBeforeMount: false, - cleanAt: Date.now() + CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS - } - return trackingData -} - -/** - * Unified api for timers/Finalization registry cleanups - * This abstraction make useObserver much simpler - */ -export interface ReactionCleanupTracking { - /** - * - * @param reaction The reaction to cleanup - * @param objectRetainedByReact This will be in actual use only when FinalizationRegister is in use - */ - addReactionToTrack( - reactionTrackingRef: React.MutableRefObject, - reaction: Reaction, - objectRetainedByReact: object - ): IReactionTracking - recordReactionAsCommitted(reactionRef: React.MutableRefObject): void - forceCleanupTimerToRunNowForTests(): void - resetCleanupScheduleForTests(): void -} - -export interface IReactionTracking { - /** The Reaction created during first render, which may be leaked */ - reaction: Reaction - /** - * The time (in ticks) at which point we should dispose of the reaction - * if this component hasn't yet been fully mounted. - */ - cleanAt: number - - /** - * Whether the component has yet completed mounting (for us, whether - * its useEffect has run) - */ - mounted: boolean - - /** - * Whether the observables that the component is tracking changed between - * the first render and the first useEffect. - */ - changedBeforeMount: boolean - - /** - * In case we are using finalization registry based cleanup, - * this will hold the cleanup token associated with this reaction - */ - finalizationRegistryCleanupToken?: number -} - -/** - * The minimum time before we'll clean up a Reaction created in a render - * for a component that hasn't managed to run its effects. This needs to - * be big enough to ensure that a component won't turn up and have its - * effects run without being re-rendered. - */ -export const CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS = 10_000 - -/** - * The frequency with which we'll check for leaked reactions. - */ -export const CLEANUP_TIMER_LOOP_MILLIS = 10_000 From 5ca1cfe404870e27be8ce5cd34709adfa4503094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 17:16:27 +0100 Subject: [PATCH 11/38] refactor --- .../src/utils/UniversalFinalizationRegistry.ts | 2 ++ packages/mobx-react/src/observerClass.ts | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts index 12d8032a8..fedb31c5b 100644 --- a/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts +++ b/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts @@ -28,6 +28,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy 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) @@ -46,6 +47,7 @@ export class TimerBasedFinalizationRegistry implements FinalizationRegistryTy } } + // Bound so it can be exported directly as clearTimers test utility. finalizeAllImmediately = () => { this.sweep(0) } diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index 826f9793b..796c3c439 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -225,8 +225,7 @@ function createReaction(admin: ObserverAdministration) { } try { - // forceUpdate sets new `props`, since we made it observable, they would `reportChanged`, causing a loop. - //admin.observablesSuppressed = true + // forceUpdate sets new `props`, since we made it observable, it would `reportChanged`, causing a loop. admin.isUpdating = true admin.forceUpdate?.() } catch (error) { @@ -234,7 +233,6 @@ function createReaction(admin: ObserverAdministration) { admin.reaction = null } finally { admin.isUpdating = false - //admin.observablesSuppressed = false } }) } From fa0583691d536343313f15d1868fbd1690239ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 17:42:04 +0100 Subject: [PATCH 12/38] use useSyncExternalStore shim --- packages/mobx-react-lite/package.json | 4 +++- packages/mobx-react-lite/src/useObserver.ts | 3 ++- yarn.lock | 5 +++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/mobx-react-lite/package.json b/packages/mobx-react-lite/package.json index d1f60a336..9cbc396d5 100644 --- a/packages/mobx-react-lite/package.json +++ b/packages/mobx-react-lite/package.json @@ -36,7 +36,9 @@ "url": "https://github.com/mobxjs/mobx/issues" }, "homepage": "https://mobx.js.org", - "dependencies": {}, + "dependencies": { + "use-sync-external-store": "^1.2.0" + }, "peerDependencies": { "mobx": "^6.1.0", "react": "^16.8.0 || ^17 || ^18" diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index b31c64f8d..b0e8b849d 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -3,6 +3,7 @@ import React from "react" import { printDebugValue } from "./utils/printDebugValue" import { isUsingStaticRendering } from "./staticRendering" import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry" +import { useSyncExternalStore } from "use-sync-external-store/shim" // Do not store `admRef` (even as part of a closure!) on this object, // otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry. @@ -91,7 +92,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs const adm = admRef.current! React.useDebugValue(adm.reaction!, printDebugValue) - React.useSyncExternalStore( + useSyncExternalStore( // Both of these must be stable, otherwise it would keep resubscribing every render. adm.subscribe, adm.getSnapshot diff --git a/yarn.lock b/yarn.lock index ac32c3811..f20d4178a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -13996,6 +13996,11 @@ url@^0.11.0: punycode "1.3.2" querystring "0.2.0" +use-sync-external-store@^1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/use-sync-external-store/-/use-sync-external-store-1.2.0.tgz#7dbefd6ef3fe4e767a0cf5d7287aacfb5846928a" + integrity sha512-eEgnFxGQ1Ife9bzYs6VLi8/4X6CObHMw9Qr9tPY43iKwsPw8xE8+EFsf/2cFZ5S3esXgpWgtSCtLNS41F+sKPA== + use@^3.1.0: version "3.1.1" resolved "https://registry.yarnpkg.com/use/-/use-3.1.1.tgz#d50c8cac79a19fbc20f2911f56eb973f4e10070f" From 8b8d9483f5b45ed09ea4d53ae3ad85b13982f7c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 17:42:46 +0100 Subject: [PATCH 13/38] typo --- packages/mobx-react-lite/src/useObserver.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index b0e8b849d..642e4581d 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -13,7 +13,7 @@ type ObserverAdministration = { // BC: we will use local state version if global isn't available. // It should behave as previous implementation - tearing is still present, // because there is no cross component synchronization, - // but we can use `useExternalSyncStore` API. + // but we can use `useSyncExternalStore` API. stateVersion: any name: string // These don't depend on state/props, therefore we can keep them here instead of `useCallback` @@ -34,7 +34,7 @@ function createReaction(adm: ObserverAdministration) { } // Force update won't be avaliable until the component "mounts". // If state changes in between initial render and mount, - // `useExternalSyncStore` should handle that by checking the state version and issuing update. + // `useSyncExternalStore` should handle that by checking the state version and issuing update. adm.forceUpdate?.() }) } From 27560d49204bf6bd19bd58cb42f6b833a55878cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 18 Dec 2022 22:22:28 +0100 Subject: [PATCH 14/38] refactor --- .changeset/early-terms-bow.md | 7 ++ .../__tests__/observer.test.tsx | 83 ------------------- .../mobx/__tests__/v5/base/observables.js | 4 +- 3 files changed, 10 insertions(+), 84 deletions(-) create mode 100644 .changeset/early-terms-bow.md diff --git a/.changeset/early-terms-bow.md b/.changeset/early-terms-bow.md new file mode 100644 index 000000000..dfa6a37d8 --- /dev/null +++ b/.changeset/early-terms-bow.md @@ -0,0 +1,7 @@ +--- +"mobx-react": minor +"mobx-react-lite": minor +"mobx": patch +--- + +TODO diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx index 94a315bd9..f0d636700 100644 --- a/packages/mobx-react-lite/__tests__/observer.test.tsx +++ b/packages/mobx-react-lite/__tests__/observer.test.tsx @@ -1084,86 +1084,3 @@ test("Anonymous component displayName #3192", () => { expect(observerError.message).toEqual(memoError.message) consoleErrorSpy.mockRestore() }) - -describe.skip("simple TODO del", () => { - let state = {} - let onStoreUpdate = () => {} - - function updateStore(newState) { - state = newState - onStoreUpdate() - } - - const subscribe = _onStoreUpdate => { - console.log("SUBSCRIBE") - onStoreUpdate = _onStoreUpdate - return () => { - console.log("UNSUBSCRIBE") - onStoreUpdate = () => {} - } - } - - const getSnapshot = () => state - - function TestCmp() { - console.log("RENDER") - - React.useEffect(() => { - console.log("MOUNT") - return () => console.log("UNMOUNT") - }, []) - - React.useSyncExternalStore(subscribe, getSnapshot) - - return null - } - - test("", async () => { - render() - - act(() => { - //console.log('before update') - updateStore({}) - //console.log('after update') - }) - // console.log('before await'); - // await (new Promise(resolve =>setTimeout(resolve, 1000))); - // console.log('after await'); - // act(()=> { - // //console.log('before update') - // updateStore({}); - // //console.log('after update') - // }) - - console.log("after act") - }) - - const execute = () => { - const renderings = { - child: 0, - parent: 0 - } - const data = { x: 1 } - const odata = mobx.observable({ y: 1 }) - - const Child = observer((props: any) => { - renderings.child++ - return {props.data.x} - }) - const TestCmp = observer(() => { - renderings.parent++ - return {odata.y} - }) - return { ...render(), renderings, odata } - } - - test("prd", () => { - const { container, renderings, odata } = execute() - expect(renderings.parent).toBe(1) - act(() => { - odata.y++ - }) - expect(renderings.parent).toBe(2) - expect(container.querySelector("span")!.innerHTML).toBe("1") - }) -}) diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index cc8230c61..041cd2e96 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2362,7 +2362,9 @@ describe("`requiresObservable` takes precedence over global `reactionRequiresObs }) test("state version updates correctly", () => { - // The current implementation updates state version at the end of batch. + // This test was designed around the idea of updating version only at the end of batch, + // which is NOT an implementation we've settled on, but the test is still valid. + // This test demonstrates that the version is correctly updated with each state mutations: // 1. Even without wrapping mutation in batch explicitely. // 2. Even in self-invoking recursive derivation. From dba0d23dd7b61a6f87c64802f04cac1c9b5cef7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 31 Dec 2022 14:52:10 +0100 Subject: [PATCH 15/38] useObserver: handle re-render without reaction --- packages/mobx-react-lite/src/useObserver.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 642e4581d..52c9b6206 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -47,6 +47,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs const admRef = React.useRef(null) if (!admRef.current) { + // First render const adm: ObserverAdministration = { reaction: null, forceUpdate: null, @@ -79,17 +80,20 @@ export function useObserver(render: () => T, baseComponentName: string = "obs } } - createReaction(adm) - admRef.current = adm + } + const adm = admRef.current! + + if (!adm.reaction) { + // First render or reaction was disposed by registry before subscribe + createReaction(adm) // StrictMode/ConcurrentMode/Suspense may mean that our component is // rendered and abandoned multiple times, so we need to track leaked // Reactions. observerFinalizationRegistry.register(admRef, adm, adm) } - const adm = admRef.current! React.useDebugValue(adm.reaction!, printDebugValue) useSyncExternalStore( From 2316e0405273cf4fb70fbc25fa9f901d9af60528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 31 Dec 2022 14:58:17 +0100 Subject: [PATCH 16/38] disposeOnUnmount jsdoc decprecation msg --- packages/mobx-react/src/disposeOnUnmount.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/mobx-react/src/disposeOnUnmount.ts b/packages/mobx-react/src/disposeOnUnmount.ts index 16b673137..d823930d7 100644 --- a/packages/mobx-react/src/disposeOnUnmount.ts +++ b/packages/mobx-react/src/disposeOnUnmount.ts @@ -21,12 +21,12 @@ function runDisposersOnWillUnmount() { } /** - * @deprecated + * @deprecated `disposeOnUnmount` is not compatible with React 18 and higher. */ export function disposeOnUnmount(target: React.Component, propertyKey: PropertyKey): void /** - * @deprecated + * @deprecated `disposeOnUnmount` is not compatible with React 18 and higher. */ export function disposeOnUnmount>( target: React.Component, @@ -34,7 +34,7 @@ export function disposeOnUnmount>( ): TF /** - * @deprecated + * @deprecated `disposeOnUnmount` is not compatible with React 18 and higher. */ export function disposeOnUnmount( target: React.Component, @@ -47,7 +47,7 @@ export function disposeOnUnmount( if (!warnedAboutDisposeOnUnmountDeprecated) { if (reactMajorVersion >= 18) { console.error( - "[mobx-react] disposeOnUnmount is not compatible with React 18. Don't use it." + "[mobx-react] disposeOnUnmount is not compatible with React 18 and higher. Don't use it." ) } else { console.warn( From dd52381cb85d521a95f019e9d3752ec626774478 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 31 Dec 2022 14:59:36 +0100 Subject: [PATCH 17/38] disposeOnUnmount refactor --- packages/mobx-react/src/disposeOnUnmount.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/mobx-react/src/disposeOnUnmount.ts b/packages/mobx-react/src/disposeOnUnmount.ts index d823930d7..fb5d7e251 100644 --- a/packages/mobx-react/src/disposeOnUnmount.ts +++ b/packages/mobx-react/src/disposeOnUnmount.ts @@ -54,10 +54,9 @@ export function disposeOnUnmount( "[mobx-react] disposeOnUnmount is deprecated. It won't work correctly with React 18 and higher." ) } + warnedAboutDisposeOnUnmountDeprecated = true } - warnedAboutDisposeOnUnmountDeprecated = true - const c = Object.getPrototypeOf(target).constructor const c2 = Object.getPrototypeOf(target.constructor) // Special case for react-hot-loader From 9e09fab9de7a40a0998e2db5086399b83416e427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 12:45:32 +0100 Subject: [PATCH 18/38] cleanup --- packages/mobx-react/src/observerClass.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index 796c3c439..dd91434a6 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -3,7 +3,6 @@ import { createAtom, _allowStateChanges, Reaction, - //$mobx, // TODO _allowStateReadsStart, _allowStateReadsEnd, _getGlobalState, @@ -15,10 +14,7 @@ import { } from "mobx-react-lite" import { shallowEqual, patch } from "./utils/utils" -// TODO symbols - const administrationSymbol = Symbol("ObserverAdministration") -//const mobxAdminProperty = $mobx // TODO const isMobXReactObserverSymbol = Symbol("isMobXReactObserver") type ObserverAdministration = { From f239cb4b3e210b9e4c966d360485e3a24841afc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 13:43:33 +0100 Subject: [PATCH 19/38] improve displayName/name handling --- .../mobx-react-lite/__tests__/observer.test.tsx | 11 +++++++---- packages/mobx-react-lite/src/observer.ts | 14 ++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx index f0d636700..a889a6f3a 100644 --- a/packages/mobx-react-lite/__tests__/observer.test.tsx +++ b/packages/mobx-react-lite/__tests__/observer.test.tsx @@ -618,12 +618,15 @@ it("should hoist known statics only", () => { expect(wrapped.render).toBe(undefined) }) -it("should have the correct displayName", () => { - const TestComponent = observer(function MyComponent() { +it("should inherit original name/displayName #3438", () => { + function Name() { return null - }) + } + Name.displayName = "DisplayName" + const TestComponent = observer(Name) - expect((TestComponent as any).type.displayName).toBe("MyComponent") + expect((TestComponent as any).type.name).toBe("Name") + expect((TestComponent as any).type.displayName).toBe("DisplayName") }) test("parent / childs render in the right order", done => { diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index dee0bb2e5..b625a742e 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/packages/mobx-react-lite/src/observer.ts @@ -104,11 +104,13 @@ export function observer

( return useObserver(() => render(props, ref), baseComponentName) } - // Don't set `displayName` for anonymous components, - // so the `displayName` can be customized by user, see #3192. - if (baseComponentName !== "") { - ;(observerComponent as React.FunctionComponent).displayName = baseComponentName - } + // Inherit original name and displayName, see #3438 + ;(observerComponent as React.FunctionComponent).displayName = baseComponent.displayName + Object.defineProperty(observerComponent, "name", { + value: baseComponent.name, + writable: true, + configurable: true + }) // Support legacy context: `contextTypes` must be applied before `memo` if ((baseComponent as any).contextTypes) { @@ -136,7 +138,7 @@ export function observer

( set() { throw new Error( `[mobx-react-lite] \`${ - this.displayName || this.type?.displayName || "Component" + this.displayName || this.type?.displayName || this.type?.name || "Component" }.contextTypes\` must be set before applying \`observer\`.` ) } From c1f375aa23ed1b90628b49aab1e318816c1cbeb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 13:58:05 +0100 Subject: [PATCH 20/38] fix mobx-react test --- packages/mobx-react/__tests__/observer.test.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index 619b1ff6e..d8c42f13b 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -353,7 +353,8 @@ test("correctly wraps display name of child component", () => { }) expect(A.name).toEqual("ObserverClass") - expect((B as any).type.displayName).toEqual("StatelessObserver") + expect((B as any).type.name).toEqual("StatelessObserver") + expect((B as any).type.displayName).toEqual(undefined) }) describe("124 - react to changes in this.props via computed", () => { From c0ececc9d2daa18c21944bd138bd9363959c5fc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 14:01:26 +0100 Subject: [PATCH 21/38] try provide GC bit more time --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index cd9a35fcc..74ff27a01 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(50) + await sleep(500) // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From 829dab9e11ec88b37ac576875216219921d19a99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 14:14:05 +0100 Subject: [PATCH 22/38] try lower GC time --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index 74ff27a01..ac2346d17 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(500) + await sleep(100) // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From 0ba9985efecc6e81124fe07eaf5f23b1a5583e80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 14:27:11 +0100 Subject: [PATCH 23/38] bit more perhpas? --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index ac2346d17..068870e0c 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(100) + await sleep(200) // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From 8c093177896fa4a43b1c917e2a0f95f568818660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 14:35:14 +0100 Subject: [PATCH 24/38] 300ms --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index 068870e0c..27b31ed27 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(200) + await sleep(300) // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From e5492e6c70d7abbff59b2b7321b625a340c3bbda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 11 Feb 2023 15:08:00 +0100 Subject: [PATCH 25/38] document magic number --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index 27b31ed27..e52a03313 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(300) + await sleep(300) // < 300 fails on CI // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From b250f5813da165ee91bf5660d1dacc0eb8b46b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 18 Feb 2023 20:51:23 +0100 Subject: [PATCH 26/38] use number instead of Symbol as stateVersion --- packages/mobx/src/core/atom.ts | 5 ++++- packages/mobx/src/core/globalstate.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/mobx/src/core/atom.ts b/packages/mobx/src/core/atom.ts index b51f4887e..beff74426 100644 --- a/packages/mobx/src/core/atom.ts +++ b/packages/mobx/src/core/atom.ts @@ -69,7 +69,10 @@ export class Atom implements IAtom { propagateChanged(this) // We could update state version only at the end of batch, // but we would still have to switch some global flag here to signal a change. - globalState.stateVersion = Symbol() + globalState.stateVersion = + globalState.stateVersion < Number.MAX_SAFE_INTEGER + ? globalState.stateVersion + 1 + : Number.MIN_SAFE_INTEGER endBatch() } diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index f27979b7d..e818775ee 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -154,7 +154,7 @@ export class MobXGlobals { /** * Changes with each state update, used by useSyncExternalStore */ - stateVersion = Symbol() + stateVersion = Number.MIN_SAFE_INTEGER } let canMergeGlobalState = true From e6c62467acdd3704ff093ebad2e7e747ac7329e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 18 Feb 2023 21:14:13 +0100 Subject: [PATCH 27/38] GC 300ms -> 500ms --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index e52a03313..4d404530c 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -43,7 +43,7 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(300) // < 300 fails on CI + await sleep(500) // < 500 randomly fails on CI // count1 should still be being observed by Component1, // but count2 should have had its reaction cleaned up. From b963f7a6f3102668afa86a74b4199a88a1c90ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 18:15:19 +0100 Subject: [PATCH 28/38] fix api test --- packages/mobx-react-lite/__tests__/api.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/api.test.ts b/packages/mobx-react-lite/__tests__/api.test.ts index ff2726fa2..a2f7cf57e 100644 --- a/packages/mobx-react-lite/__tests__/api.test.ts +++ b/packages/mobx-react-lite/__tests__/api.test.ts @@ -18,7 +18,8 @@ test("correct api should be exposed", function () { "useObserver", "isObserverBatched", "observerBatching", - "useStaticRendering" + "useStaticRendering", + "_observerFinalizationRegistry" ].sort() ) }) From 344c2cf2a9be085550c2e1496add56581ae26bd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 18:28:11 +0100 Subject: [PATCH 29/38] move use-sync-external-store to dev deps --- packages/mobx-react-lite/package.json | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/mobx-react-lite/package.json b/packages/mobx-react-lite/package.json index 70e172d84..05e187362 100644 --- a/packages/mobx-react-lite/package.json +++ b/packages/mobx-react-lite/package.json @@ -36,9 +36,7 @@ "url": "https://github.com/mobxjs/mobx/issues" }, "homepage": "https://mobx.js.org", - "dependencies": { - "use-sync-external-store": "^1.2.0" - }, + "dependencies": {}, "peerDependencies": { "mobx": "^6.1.0", "react": "^16.8.0 || ^17 || ^18" @@ -53,7 +51,8 @@ }, "devDependencies": { "mobx": "^6.8.0", - "expose-gc": "^1.0.0" + "expose-gc": "^1.0.0", + "use-sync-external-store": "^1.2.0" }, "keywords": [ "mobx", From 98940ad98977cbd19af5cd6c5fe21f242c1f27d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 19:00:11 +0100 Subject: [PATCH 30/38] changeset --- .changeset/brown-seals-worry.md | 5 +++++ .changeset/early-terms-bow.md | 7 +++---- .changeset/wise-waves-jam.md | 12 ++++++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 .changeset/brown-seals-worry.md create mode 100644 .changeset/wise-waves-jam.md diff --git a/.changeset/brown-seals-worry.md b/.changeset/brown-seals-worry.md new file mode 100644 index 000000000..b3156ee8f --- /dev/null +++ b/.changeset/brown-seals-worry.md @@ -0,0 +1,5 @@ +--- +"mobx": minor +--- + +Better support for React 18: Mobx now keeps track of a global state version, which updates with each mutation. diff --git a/.changeset/early-terms-bow.md b/.changeset/early-terms-bow.md index dfa6a37d8..381318017 100644 --- a/.changeset/early-terms-bow.md +++ b/.changeset/early-terms-bow.md @@ -1,7 +1,6 @@ --- -"mobx-react": minor -"mobx-react-lite": minor -"mobx": patch +"mobx-react-lite": major --- -TODO +Components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously. +Improved displayName/name handling as discussed in #3438. diff --git a/.changeset/wise-waves-jam.md b/.changeset/wise-waves-jam.md new file mode 100644 index 000000000..10c5d6145 --- /dev/null +++ b/.changeset/wise-waves-jam.md @@ -0,0 +1,12 @@ +--- +"mobx-react": major +--- + +Functional components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously. +Improved displayName/name handling of functional components as discussed in #3438. +Reactions of uncommited class components are now correctly disposed, fixes #3492. +Reactions don't notify uncommited class components, avoiding the warning, fixes #3492. +Removed symbol "polyfill" and replaced with actual Symbols. +Removed `this.render` replacement detection + warning. `this.render` is no longer configurable/writable (possibly BC). +Class component instance is no longer exposed as `component[$mobx]["reactcomponent"]` (possibly BC). +Deprecated `disposeOnUnmount`, it's not compatible with remounting. From 4ebb124b0da9e70b7471fb560794a81e69010e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 21:57:07 +0100 Subject: [PATCH 31/38] bump deps --- packages/mobx-react-lite/package.json | 4 ++-- packages/mobx-react/package.json | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/mobx-react-lite/package.json b/packages/mobx-react-lite/package.json index 05e187362..69ad36350 100644 --- a/packages/mobx-react-lite/package.json +++ b/packages/mobx-react-lite/package.json @@ -38,7 +38,7 @@ "homepage": "https://mobx.js.org", "dependencies": {}, "peerDependencies": { - "mobx": "^6.1.0", + "mobx": "^6.9.0", "react": "^16.8.0 || ^17 || ^18" }, "peerDependenciesMeta": { @@ -50,7 +50,7 @@ } }, "devDependencies": { - "mobx": "^6.8.0", + "mobx": "^6.9.0", "expose-gc": "^1.0.0", "use-sync-external-store": "^1.2.0" }, diff --git a/packages/mobx-react/package.json b/packages/mobx-react/package.json index d797d6a8c..27be74179 100644 --- a/packages/mobx-react/package.json +++ b/packages/mobx-react/package.json @@ -36,10 +36,10 @@ }, "homepage": "https://mobx.js.org", "dependencies": { - "mobx-react-lite": "^3.4.0" + "mobx-react-lite": "^4.0.0" }, "peerDependencies": { - "mobx": "^6.1.0", + "mobx": "^6.9.0", "react": "^16.8.0 || ^17 || ^18" }, "peerDependenciesMeta": { @@ -51,8 +51,8 @@ } }, "devDependencies": { - "mobx": "^6.7.0", - "mobx-react-lite": "^3.4.0", + "mobx": "^6.9.0", + "mobx-react-lite": "^4.0.0", "expose-gc": "^1.0.0" }, "keywords": [ From 1c7e16c58ad3621484e7224e641b49346d189e89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 22:13:07 +0100 Subject: [PATCH 32/38] deps? --- packages/mobx-react-lite/package.json | 2 +- packages/mobx-react/package.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mobx-react-lite/package.json b/packages/mobx-react-lite/package.json index 69ad36350..dd8470ee0 100644 --- a/packages/mobx-react-lite/package.json +++ b/packages/mobx-react-lite/package.json @@ -50,7 +50,7 @@ } }, "devDependencies": { - "mobx": "^6.9.0", + "mobx": "^6.8.0", "expose-gc": "^1.0.0", "use-sync-external-store": "^1.2.0" }, diff --git a/packages/mobx-react/package.json b/packages/mobx-react/package.json index 27be74179..82fffd4ec 100644 --- a/packages/mobx-react/package.json +++ b/packages/mobx-react/package.json @@ -51,8 +51,8 @@ } }, "devDependencies": { - "mobx": "^6.9.0", - "mobx-react-lite": "^4.0.0", + "mobx": "^6.8.0", + "mobx-react-lite": "^3.4.3", "expose-gc": "^1.0.0" }, "keywords": [ From 4b138b594ad27f3dc9305052d22fe195491e5e7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 12 Mar 2023 22:16:14 +0100 Subject: [PATCH 33/38] deps? --- packages/mobx-react/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react/package.json b/packages/mobx-react/package.json index 82fffd4ec..479fe6a7d 100644 --- a/packages/mobx-react/package.json +++ b/packages/mobx-react/package.json @@ -36,7 +36,7 @@ }, "homepage": "https://mobx.js.org", "dependencies": { - "mobx-react-lite": "^4.0.0" + "mobx-react-lite": "^3.4.3" }, "peerDependencies": { "mobx": "^6.9.0", From 21a7590e04e1d051f15b32d19b89a6ea717b62fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:08:33 +0100 Subject: [PATCH 34/38] add new lines to changeset --- .changeset/early-terms-bow.md | 4 ++-- .changeset/wise-waves-jam.md | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.changeset/early-terms-bow.md b/.changeset/early-terms-bow.md index 381318017..1ccce7473 100644 --- a/.changeset/early-terms-bow.md +++ b/.changeset/early-terms-bow.md @@ -2,5 +2,5 @@ "mobx-react-lite": major --- -Components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously. -Improved displayName/name handling as discussed in #3438. +Components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously.
+Improved displayName/name handling as discussed in #3438.
diff --git a/.changeset/wise-waves-jam.md b/.changeset/wise-waves-jam.md index 10c5d6145..2bb4557cc 100644 --- a/.changeset/wise-waves-jam.md +++ b/.changeset/wise-waves-jam.md @@ -2,11 +2,11 @@ "mobx-react": major --- -Functional components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously. -Improved displayName/name handling of functional components as discussed in #3438. -Reactions of uncommited class components are now correctly disposed, fixes #3492. -Reactions don't notify uncommited class components, avoiding the warning, fixes #3492. -Removed symbol "polyfill" and replaced with actual Symbols. -Removed `this.render` replacement detection + warning. `this.render` is no longer configurable/writable (possibly BC). -Class component instance is no longer exposed as `component[$mobx]["reactcomponent"]` (possibly BC). -Deprecated `disposeOnUnmount`, it's not compatible with remounting. +Functional components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously.
+Improved displayName/name handling of functional components as discussed in #3438.
+Reactions of uncommited class components are now correctly disposed, fixes #3492.
+Reactions don't notify uncommited class components, avoiding the warning, fixes #3492.
+Removed symbol "polyfill" and replaced with actual Symbols.
+Removed `this.render` replacement detection + warning. `this.render` is no longer configurable/writable (possibly BC).
+Class component instance is no longer exposed as `component[$mobx]["reactcomponent"]` (possibly BC).
+Deprecated `disposeOnUnmount`, it's not compatible with remounting.
From 91e6c1aeb9005d0c48f652b4f5fcf1d951605b72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:12:09 +0100 Subject: [PATCH 35/38] remove test --- .../__snapshots__/observer.test.tsx.snap | 18 -------------- .../mobx-react/__tests__/observer.test.tsx | 24 ------------------- 2 files changed, 42 deletions(-) diff --git a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap b/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap index 1ec4711f0..3c972cb79 100644 --- a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap +++ b/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap @@ -1,23 +1,5 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`#797 - replacing this.render should trigger a warning 1`] = ` -[MockFunction] { - "calls": Array [ - Array [ - "The reactive render of an observer class component (Component) - was overridden after MobX attached. This may result in a memory leak if the - overridden reactive render was not properly disposed.", - ], - ], - "results": Array [ - Object { - "type": "return", - "value": undefined, - }, - ], -} -`; - exports[`#3492 should not cause warning by calling forceUpdate on uncommited components 1`] = `[MockFunction]`; exports[`Redeclaring an existing observer component as an observer should log a warning 1`] = ` diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index d8c42f13b..cc5d818d8 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -871,30 +871,6 @@ test.skip("#709 - applying observer on React.memo component", () => { render(, { wrapper: ErrorCatcher }) }) -// this.render is made non-configurable, therefore can't be replaced -test.skip("#797 - replacing this.render should trigger a warning", () => { - consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {}) - - @observer - class Component extends React.Component { - render() { - return

- } - swapRenderFunc() { - this.render = () => { - return - } - } - } - - const compRef = React.createRef() - const { unmount } = render() - compRef.current?.swapRenderFunc() - unmount() - - expect(consoleWarnMock).toMatchSnapshot() -}) - test("Redeclaring an existing observer component as an observer should log a warning", () => { consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {}) From 12ef4cc73f85e771b80b19a5378992a57bfddaa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:47:58 +0100 Subject: [PATCH 36/38] improve comment --- packages/mobx-react/__tests__/finalizationRegistry.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react/__tests__/finalizationRegistry.tsx b/packages/mobx-react/__tests__/finalizationRegistry.tsx index c1875069e..76673a5b5 100644 --- a/packages/mobx-react/__tests__/finalizationRegistry.tsx +++ b/packages/mobx-react/__tests__/finalizationRegistry.tsx @@ -14,7 +14,7 @@ function sleep(time: number) { }) } -// TODO dunno why it's not available +// TODO remove once https://github.com/mobxjs/mobx/pull/3620 is merged. declare class WeakRef { constructor(object: T) deref(): T | undefined From 98cde8bc99e9a1d0ff36749ef54f1751d44a7338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:50:03 +0100 Subject: [PATCH 37/38] make finalization regsitry test bit more robust --- ...rentModeUsingFinalizationRegistry.test.tsx | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index 4d404530c..eb45ccfd7 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -1,4 +1,4 @@ -import { cleanup, render } from "@testing-library/react" +import { cleanup, render, waitFor } from "@testing-library/react" import * as mobx from "mobx" import * as React from "react" import { useObserver } from "../src/useObserver" @@ -43,10 +43,17 @@ test("uncommitted components should not leak observations", async () => { // Allow gc to kick in in case to let finalization registry cleanup gc() - await sleep(500) // < 500 randomly fails on CI - - // count1 should still be being observed by Component1, - // but count2 should have had its reaction cleaned up. - expect(count1IsObserved).toBeTruthy() - expect(count2IsObserved).toBeFalsy() + // Can take a while (especially on CI) before gc actually calls the registry + await waitFor( + () => { + // count1 should still be being observed by Component1, + // but count2 should have had its reaction cleaned up. + expect(count1IsObserved).toBeTruthy() + expect(count2IsObserved).toBeFalsy() + }, + { + timeout: 1500, + interval: 200 + } + ) }) From e55c51ccaa18b8f47b16dc70ea659b907a4e3da7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:57:24 +0100 Subject: [PATCH 38/38] increase timeout --- .../strictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index eb45ccfd7..bba6927e0 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -52,7 +52,7 @@ test("uncommitted components should not leak observations", async () => { expect(count2IsObserved).toBeFalsy() }, { - timeout: 1500, + timeout: 2000, interval: 200 } )