fix: ObservableSet.replace only emits events for actual changes

.changeset/observable-set-replace-events.md

@@ -0,0 +1,7 @@
+---
+"mobx": patch
+---
+
+Fix `ObservableSet.replace` emitting spurious `delete`/`add` events (and triggering reactions) for values that are unchanged. It now only fires `delete` for removed values and `add` for newly added ones, mirroring `ObservableMap.replace`.
+
+Note: because `replace` no longer clears and re-adds every value, the iteration order after `replace` changes in a (subtle but observable) way. Surviving values now keep their original relative position and newly added values are appended, instead of the whole set being reordered to match the argument. For example, `set(["a", "b", "c"]).replace(["d", "b", "a"])` previously iterated as `d, b, a`, and now iterates as `a, b, d`. This is arguably the more correct behavior (unchanged values are genuinely unchanged), but if you relied on `replace` reordering the set to match its argument, you may need to adjust.

packages/mobx/__tests__/base/set.js

@@ -512,3 +512,109 @@ describe("Observable Set interceptors", () => {
         expect([...s]).toStrictEqual([1, 10])
     })
 })
+
+describe("#3761 replace only fires events for actual changes", () => {
+    test("replace only emits delete/add for removed/added values", () => {
+        const s = set(["a", "b", "c"])
+        const events = []
+        mobx.observe(s, change => {
+            delete change.observableKind
+            delete change.debugObjectName
+            events.push(change)
+        })
+
+        // The replacement is intentionally ordered differently from the original
+        // ("c", "a", "d" vs "a", "b", "c"): "b" is removed, "d" is added, "a"/"c" survive.
+        s.replace(["c", "a", "d"])
+
+        expect(events).toEqual([
+            { object: s, oldValue: "b", type: "delete" },
+            { object: s, newValue: "d", type: "add" }
+        ])
+        // Surviving values keep their original relative order ("a" before "c") and the
+        // added value is appended, so the result iterates as ["a", "c", "d"]. See the
+        // iteration-order note in the changeset / #3761 discussion.
+        expect(mobx.values(s)).toEqual(["a", "c", "d"])
+    })
+
+    test("replace with identical content emits no events", () => {
+        const s = set(["x", "y"])
+        const events = []
+        mobx.observe(s, change => events.push(change))
+
+        s.replace(["x", "y"])
+
+        expect(events).toEqual([])
+        expect(mobx.values(s)).toEqual(["x", "y"])
+    })
+
+    test("replace with an ES6 Set only emits events for actual changes", () => {
+        const s = set([1, 2, 3])
+        const events = []
+        mobx.observe(s, change => {
+            delete change.observableKind
+            delete change.debugObjectName
+            events.push(change)
+        })
+
+        // Reordered replacement (3, 1, 4 vs 1, 2, 3): 2 is removed, 4 is added, 1/3 survive.
+        s.replace(new Set([3, 1, 4]))
+
+        expect(events).toEqual([
+            { object: s, oldValue: 2, type: "delete" },
+            { object: s, newValue: 4, type: "add" }
+        ])
+        // Survivors keep their original relative order (1 before 3), 4 is appended.
+        expect(mobx.values(s)).toEqual([1, 3, 4])
+    })
+
+    test("replace with an observable Set only emits events for actual changes", () => {
+        const s = set([1, 2, 3])
+        const other = set([2, 3, 4])
+        const events = []
+        mobx.observe(s, change => {
+            delete change.observableKind
+            delete change.debugObjectName
+            events.push(change)
+        })
+
+        s.replace(other)
+
+        expect(events).toEqual([
+            { object: s, oldValue: 1, type: "delete" },
+            { object: s, newValue: 4, type: "add" }
+        ])
+        expect(mobx.values(s)).toEqual([2, 3, 4])
+    })
+
+    test("replace with identical content does not report a change", () => {
+        const s = set([1, 2, 3])
+        let runCount = 0
+        const dispose = mobx.autorun(() => {
+            mobx.values(s)
+            runCount++
+        })
+        expect(runCount).toBe(1)
+
+        // Nothing actually changes, so observers must not be notified.
+        s.replace([1, 2, 3])
+
+        expect(runCount).toBe(1)
+        dispose()
+    })
+
+    test("replace still honors interceptors", () => {
+        const s = set([1, 2])
+        mobx.intercept(s, change => {
+            // Prevent adding 4.
+            if (change.type === "add" && change.newValue === 4) {
+                return undefined
+            }
+            return change
+        })
+
+        s.replace([2, 3, 4])
+
+        expect(mobx.values(s)).toEqual([2, 3])
+    })
+})

packages/mobx/src/types/observableset.ts

@@ -55,16 +55,14 @@ export type ISetWillDeleteChange<T = any> = {
     type: "delete"
     object: ObservableSet<T>
     oldValue: T
-};
+}
 export type ISetWillAddChange<T = any> = {
     type: "add"
     object: ObservableSet<T>
     newValue: T
-};
+}
 
-export type ISetWillChange<T = any> =
-    | ISetWillDeleteChange<T>
-    | ISetWillAddChange<T>
+export type ISetWillChange<T = any> = ISetWillDeleteChange<T> | ISetWillAddChange<T>
 
 export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillChange>, IListenable {
     [$mobx] = ObservableSetMarker
@@ -133,8 +131,7 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
             }
 
             // implemented reassignment same as it's done for ObservableMap
-            value = change.newValue!;
-
+            value = change.newValue!
         }
         if (!this.has(value)) {
             transaction(() => {
@@ -296,17 +293,42 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
             other = new Set(other)
         }
 
-        transaction(() => {
-            if (Array.isArray(other)) {
-                this.clear()
-                other.forEach(value => this.add(value))
-            } else if (isES6Set(other)) {
-                this.clear()
-                other.forEach(value => this.add(value))
-            } else if (other !== null && other !== undefined) {
-                die("Cannot initialize set from " + other)
-            }
-        })
+        if (Array.isArray(other) || isES6Set(other)) {
+            // Only emit `delete`/`add` events (and `reportChanged`) for values that
+            // actually change, instead of clearing and re-adding everything. `add` and
+            // `delete` are already no-ops for values that are respectively already
+            // present or already absent, so we just need to avoid deleting values that
+            // are part of the replacement. See #3761.
+            transaction(() => {
+                // Collect the desired values for quick lookup. `other` is already a Set
+                // here when it was passed (or snapshotted from an observable set) as one,
+                // so reuse it rather than allocating another; arrays are wrapped (which
+                // also dedupes them).
+                const replacementValues: Set<T> = isES6Set(other)
+                    ? other
+                    : new Set<T>(other as Iterable<T>)
+                // Short-circuit the trivial cases: an empty replacement is just a clear,
+                // and replacing into an empty set only needs the adds.
+                if (replacementValues.size === 0) {
+                    this.clear()
+                    return
+                }
+                if (this.data_.size === 0) {
+                    replacementValues.forEach(value => this.add(value))
+                    return
+                }
+                // Delete values that are not part of the replacement.
+                for (const value of this.data_.values()) {
+                    if (!replacementValues.has(this.dehanceValue_(value))) {
+                        this.delete(value)
+                    }
+                }
+                // Add new values; values that are already present are a no-op.
+                replacementValues.forEach(value => this.add(value))
+            })
+        } else if (other !== null && other !== undefined) {
+            die("Cannot initialize set from " + other)
+        }
 
         return this
     }