[pull] main from mobxjs:main

.changeset/fix-endbatch-unobservation-reentrancy.md

@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+Fix a stack overflow ("Maximum call stack size exceeded") that could occur when an `onBecomeUnobserved` handler disposes a `Reaction`. Disposing a `Reaction` re-enters `endBatch()`, which used to recurse into the same `pendingUnobservations` drain loop instead of letting the already-running outer loop pick up the newly queued items, causing unbounded stack depth for long enough chains.

.changeset/fix-observableset-receiver-order.md

@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+Fix ObservableSet union, intersection and symmetricDifference to return results in receiver order, matching native Set, when the argument is a plain Set.

.changeset/lazy-observers-allocation.md

@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+perf: lazily allocate the internal `observers_` Set. Atoms and computed values no longer allocate an empty `Set` upfront; it is created on first observer instead. Most atoms in large stores are never observed, so this saves roughly 160 bytes per unobserved atom (e.g. ~35% lower heap usage when hydrating 50k instances with 10 observable fields each).

.changeset/use-observer-first-render-retention.md

@@ -0,0 +1,5 @@
+---
+"mobx-react-lite": patch
+---
+
+fix: `useObserver` no longer retains the element tree returned by a component's first render for the component's whole mounted life. `subscribe`/`getSnapshot` were created inside `useObserver`'s first invocation and therefore shared that invocation's closure context with the `reaction.track` callback's captures (`render`, `renderResult`); since React's `useSyncExternalStore` holds `subscribe` while the component is mounted, the first render result (and every fiber and DOM node reachable from it) could never be garbage collected. The administration object is now created by a module-level factory whose scope contains nothing render-related.

packages/mobx-react-lite/__tests__/useObserverRetention.test.tsx

@@ -0,0 +1,50 @@
+import { render } from "@testing-library/react"
+import * as React from "react"
+import gc from "expose-gc/function"
+import { observer } from "../src"
+
+function nextFrame() {
+    return new Promise(accept => setTimeout(accept, 1))
+}
+
+async function gc_cycle() {
+    await nextFrame()
+    gc()
+    await nextFrame()
+}
+
+// If `subscribe` shares a closure context with the `reaction.track` callback,
+// the first render's result stays reachable for as long as the component is mounted.
+// See the comment on `createObserverAdministration` in src/useObserver.ts.
+test("a mounted observer does not retain the element tree it first returned", async () => {
+    let capturedFirstTree: object | null = null
+
+    const TestComponent = observer(function TestComponent({ label }: { label: string }) {
+        const tree = <div data-label={label} />
+        if (!capturedFirstTree) {
+            capturedFirstTree = tree
+        }
+        return tree
+    })
+
+    const rendering = render(<TestComponent label="first" />)
+
+    const weakFirstTree = new WeakRef(capturedFirstTree!)
+    capturedFirstTree = null
+
+    // Several re-renders rather than one: React double-buffers fibers, so the
+    // `alternate` fiber legitimately keeps the immediately-previous render's
+    // tree alive for one extra commit. Distinct props are required because
+    // `observer` wraps the component in `React.memo`.
+    rendering.rerender(<TestComponent label="second" />)
+    rendering.rerender(<TestComponent label="third" />)
+    rendering.rerender(<TestComponent label="fourth" />)
+
+    await gc_cycle()
+
+    expect(weakFirstTree.deref()).toBeUndefined()
+})
+
+// There is deliberately no assertion that the first render's `props` become
+// collectable: a control component wrapped in plain `React.memo` (no MobX involved)
+// fails such an assertion too, so it cannot isolate MobX's contribution.

packages/mobx-react-lite/src/useObserver.ts

@@ -30,6 +30,49 @@ function createReaction(adm: ObserverAdministration) {
     })
 }
 
+// This must stay a module-level factory and must NOT be inlined into `useObserver`:
+// all closures created during the same function invocation share one context object.
+// If `subscribe`/`getSnapshot` were created inside `useObserver`, that shared context
+// would also hold the `reaction.track` callback's captures (`render`, `renderResult`),
+// and since `useSyncExternalStore` holds `subscribe` for as long as the component is
+// mounted, every observer would retain its first render's element tree, fibers and DOM.
+// Module scope also makes it impossible for these closures to capture `admRef`,
+// which would prevent its collection and break leaked-reaction disposal via the
+// FinalizationRegistry (see the comment on `ObserverAdministration`).
+function createObserverAdministration(baseComponentName: string): ObserverAdministration {
+    const adm: ObserverAdministration = {
+        reaction: null,
+        onStoreChange: null,
+        stateVersion: Symbol(),
+        name: baseComponentName,
+        subscribe(onStoreChange: () => void) {
+            observerFinalizationRegistry.unregister(adm)
+            adm.onStoreChange = onStoreChange
+            if (!adm.reaction) {
+                // We've lost our reaction and therefore all subscriptions, occurs when:
+                // 1. Timer based finalization registry disposed reaction before component mounted.
+                // 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
+                // We have to recreate reaction and schedule re-render to recreate subscriptions,
+                // even if state did not change.
+                createReaction(adm)
+                // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value.
+                // So we make sure that is not the case
+                adm.stateVersion = Symbol()
+            }
+
+            return () => {
+                adm.onStoreChange = null
+                adm.reaction?.dispose()
+                adm.reaction = null
+            }
+        },
+        getSnapshot() {
+            return adm.stateVersion
+        }
+    }
+    return adm
+}
+
 export function useObserver<T>(render: () => T, baseComponentName: string = "observed"): T {
     if (isUsingStaticRendering()) {
         return render()
@@ -39,41 +82,7 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
 
     if (!admRef.current) {
         // First render
-        const adm: ObserverAdministration = {
-            reaction: null,
-            onStoreChange: null,
-            stateVersion: Symbol(),
-            name: baseComponentName,
-            subscribe(onStoreChange: () => void) {
-                // Do NOT access admRef here!
-                observerFinalizationRegistry.unregister(adm)
-                adm.onStoreChange = onStoreChange
-                if (!adm.reaction) {
-                    // We've lost our reaction and therefore all subscriptions, occurs when:
-                    // 1. Timer based finalization registry disposed reaction before component mounted.
-                    // 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
-                    // We have to recreate reaction and schedule re-render to recreate subscriptions,
-                    // even if state did not change.
-                    createReaction(adm)
-                    // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value.
-                    // So we make sure that is not the case
-                    adm.stateVersion = Symbol()
-                }
-
-                return () => {
-                    // Do NOT access admRef here!
-                    adm.onStoreChange = null
-                    adm.reaction?.dispose()
-                    adm.reaction = null
-                }
-            },
-            getSnapshot() {
-                // Do NOT access admRef here!
-                return adm.stateVersion
-            }
-        }
-
-        admRef.current = adm
+        admRef.current = createObserverAdministration(baseComponentName)
     }
 
     const adm = admRef.current!

packages/mobx/__tests__/base/become-observed.ts

@@ -502,6 +502,58 @@ test("#2667", () => {
     ])
 })
 
+test("#3954 - disposing a chain of reactions from onBecomeUnobserved doesn't overflow the stack", () => {
+    // Each box's onBecomeUnobserved handler disposes the next reaction in the
+    // chain. Disposing reaction[0] unobserves box[0], whose handler disposes
+    // reaction[1], which unobserves box[1], and so on. Each of those disposals
+    // re-enters endBatch() while the previous one is still draining
+    // pendingUnobservations, so this used to recurse N deep instead of
+    // looping, overflowing the stack for a large enough chain.
+    const N = 10000
+    const boxes = Array.from({ length: N }, () => observable.box(0))
+    const disposers = boxes.map(box => autorun(() => box.get()))
+    let unobservedCount = 0
+
+    boxes.forEach((box, i) => {
+        onBecomeUnobserved(box, () => {
+            unobservedCount++
+            if (i + 1 < N) {
+                disposers[i + 1]()
+            }
+        })
+    })
+
+    expect(() => disposers[0]()).not.toThrow()
+
+    // the whole chain should have unwound, not just the first link
+    expect(unobservedCount).toBe(N)
+})
+
+test("#3954 followup - isRunningUnobservations is released even if an onBecomeUnobserved handler throws", () => {
+    const boxA = observable.box(0)
+    const disposeA = autorun(() => boxA.get())
+    onBecomeUnobserved(boxA, () => {
+        throw new Error("boom")
+    })
+
+    // the handler's exception should still surface to the caller, not be swallowed
+    expect(() => disposeA()).toThrow("boom")
+
+    // if the internal guard were left stuck true after that exception, every
+    // future endBatch() would silently stop draining pendingUnobservations,
+    // so this completely unrelated disposal would never fire its own handler
+    const boxB = observable.box(0)
+    const disposeB = autorun(() => boxB.get())
+    let unobservedB = false
+    onBecomeUnobserved(boxB, () => {
+        unobservedB = true
+    })
+
+    disposeB()
+
+    expect(unobservedB).toBe(true)
+})
+
 test("works with ObservableSet #3595", () => {
     const onSetObserved = jest.fn()
     const onSetUnobserved = jest.fn()

packages/mobx/__tests__/base/errorhandling.js

@@ -484,7 +484,7 @@ test("peeking inside erroring computed value doesn't bork (global) state", () =>
     }).toThrow(/chocolademelk/)
 
     expect(a.isPendingUnobservation).toBe(false)
-    expect(a.observers_.size).toBe(0)
+    expect(a.observers_?.size ?? 0).toBe(0)
     expect(a.diffValue).toBe(0)
     expect(a.lowestObserverState_).toBe(-1)
     expect(a.hasUnreportedChange_).toBe(false)
@@ -494,7 +494,7 @@ test("peeking inside erroring computed value doesn't bork (global) state", () =>
     expect(b.observing_.length).toBe(0)
     expect(b.newObserving_).toBe(null)
     expect(b.isPendingUnobservation).toBe(false)
-    expect(b.observers_.size).toBe(0)
+    expect(b.observers_?.size ?? 0).toBe(0)
     expect(b.diffValue).toBe(0)
     expect(b.lowestObserverState_).toBe(0)
     expect(b.unboundDepsCount_).toBe(0)

packages/mobx/__tests__/base/observables.js

@@ -1031,7 +1031,7 @@ test("prematurely end autorun", function () {
             x.get()
         })
 
-        expect(x.observers_.size).toBe(0)
+        expect(x.observers_?.size ?? 0).toBe(0)
         expect(dis1[$mobx].observing_.length).toBe(0)
         expect(dis2[$mobx].observing_.length).toBe(0)
 
@@ -1282,15 +1282,15 @@ test("prematurely ended autoruns are cleaned up properly", () => {
 
     expect(called).toBe(1)
     expect(a.observers_.size).toBe(1)
-    expect(b.observers_.size).toBe(0)
+    expect(b.observers_?.size ?? 0).toBe(0)
     expect(c.observers_.size).toBe(1)
     expect(d[$mobx].observing_.length).toBe(2)
 
     a.set(2)
 
     expect(called).toBe(2)
     expect(a.observers_.size).toBe(0)
-    expect(b.observers_.size).toBe(0)
+    expect(b.observers_?.size ?? 0).toBe(0)
     expect(c.observers_.size).toBe(0)
     expect(d[$mobx].observing_.length).toBe(0)
 })

packages/mobx/__tests__/base/set.js

@@ -339,6 +339,17 @@ describe("The Set object methods do what they are supposed to do", () => {
         expect(isDisjointFromObservableResult).toBeTruthy()
     })
 
+    test("set methods preserve receiver order (matches native Set)", () => {
+        const nativeCopy = new Set(reactiveSet)
+        const other = new Set([6, 2, 1])
+
+        expect([...reactiveSet.union(other)]).toEqual([...nativeCopy.union(other)])
+        expect([...reactiveSet.intersection(other)]).toEqual([...nativeCopy.intersection(other)])
+        expect([...reactiveSet.symmetricDifference(other)]).toEqual([
+            ...nativeCopy.symmetricDifference(other)
+        ])
+    })
+
     test("with ObservableSet #3919", () => {
         const intersectionObservableResult = reactiveSet.intersection(set([1, 2, 6]))
         const unionObservableResult = reactiveSet.union(set([1, 2, 6]))

packages/mobx/src/core/atom.ts

@@ -30,7 +30,8 @@ const enum AtomFlags {
 export class Atom implements IAtom {
     private flags_ = 0b000
 
-    observers_ = new Set<IDerivation>()
+    // Allocated lazily on first observer to save memory.
+    observers_: Set<IDerivation> | null = null
 
     lastAccessedBy_ = 0
     lowestObserverState_ = IDerivationState_.NOT_TRACKING_

packages/mobx/src/core/computedvalue.ts

@@ -84,7 +84,8 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
     dependenciesState_ = IDerivationState_.NOT_TRACKING_
     observing_: IObservable[] = [] // nodes we are looking at. Our value depends on these nodes
     newObserving_ = null // during tracking it's an array with new observed observers
-    observers_ = new Set<IDerivation>()
+    // Lazily allocated on first observer - see Atom.observers_.
+    observers_: Set<IDerivation> | null = null
     runId_ = 0
     lastAccessedBy_ = 0
     lowestObserverState_ = IDerivationState_.UP_TO_DATE_
@@ -200,7 +201,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
         if (
             globalState.inBatch === 0 &&
             // !globalState.trackingDerivatpion &&
-            this.observers_.size === 0 &&
+            (!this.observers_ || this.observers_.size === 0) &&
             !this.keepAlive_
         ) {
             if (shouldCompute(this)) {

packages/mobx/src/core/derivation.ts

@@ -134,7 +134,7 @@ export function checkIfStateModificationsAreAllowed(atom: IAtom) {
     if (!__DEV__) {
         return
     }
-    const hasObservers = atom.observers_.size > 0
+    const hasObservers = !!atom.observers_ && atom.observers_.size > 0
     // Should not be possible to change observed state outside strict mode, except during initialization, see #563
     if (
         !globalState.allowStateChanges &&

packages/mobx/src/core/globalstate.ts

@@ -82,6 +82,14 @@ export class MobXGlobals {
      */
     isRunningReactions = false
 
+    /**
+     * Are we currently draining pendingUnobservations in endBatch?
+     * An onBecomeUnobserved handler can dispose a Reaction, which calls
+     * startBatch/endBatch again; this guards against re-entering the same
+     * drain loop recursively (see endBatch in observable.ts).
+     */
+    isRunningUnobservations = false
+
     /**
      * Is it allowed to change observables at this point?
      * In general, MobX doesn't allow that when running computations and React.render.