From 6b57ffda813a65a0763206edcf7305e8cb90d655 Mon Sep 17 00:00:00 2001 From: Alexander Kireev Date: Sat, 13 Jun 2026 22:36:21 +0700 Subject: [PATCH 1/2] fix: ObservableSet.replace only emits events for actual changes ObservableSet.replace previously cleared the set and re-added every value, emitting a delete event for every existing element followed by an add event for every replacement element - even when the contents were unchanged. This also retriggered reactions unnecessarily. It now deletes only the values that are not part of the replacement and adds only the new ones (add/delete are already no-ops for unchanged values), mirroring the behavior of ObservableMap.replace. Closes #3761 --- .changeset/observable-set-replace-events.md | 5 ++ packages/mobx/__tests__/base/set.js | 99 +++++++++++++++++++++ packages/mobx/src/types/observableset.ts | 43 +++++---- 3 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 .changeset/observable-set-replace-events.md diff --git a/.changeset/observable-set-replace-events.md b/.changeset/observable-set-replace-events.md new file mode 100644 index 0000000000..f755ba0521 --- /dev/null +++ b/.changeset/observable-set-replace-events.md @@ -0,0 +1,5 @@ +--- +"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`. diff --git a/packages/mobx/__tests__/base/set.js b/packages/mobx/__tests__/base/set.js index 45d893547b..95c83b73e7 100644 --- a/packages/mobx/__tests__/base/set.js +++ b/packages/mobx/__tests__/base/set.js @@ -512,3 +512,102 @@ 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) + }) + + s.replace(["b", "c", "d"]) + + expect(events).toEqual([ + { object: s, oldValue: "a", type: "delete" }, + { object: s, newValue: "d", type: "add" } + ]) + expect(mobx.values(s)).toEqual(["b", "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) + }) + + s.replace(new Set([2, 3, 4])) + + 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 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]) + }) +}) diff --git a/packages/mobx/src/types/observableset.ts b/packages/mobx/src/types/observableset.ts index ba9cdc2f43..16e4f1c4c8 100644 --- a/packages/mobx/src/types/observableset.ts +++ b/packages/mobx/src/types/observableset.ts @@ -55,16 +55,14 @@ export type ISetWillDeleteChange = { type: "delete" object: ObservableSet oldValue: T -}; +} export type ISetWillAddChange = { type: "add" object: ObservableSet newValue: T -}; +} -export type ISetWillChange = - | ISetWillDeleteChange - | ISetWillAddChange +export type ISetWillChange = ISetWillDeleteChange | ISetWillAddChange export class ObservableSet implements Set, IInterceptable, IListenable { [$mobx] = ObservableSetMarker @@ -133,8 +131,7 @@ export class ObservableSet implements Set, IInterceptable { @@ -296,17 +293,27 @@ export class ObservableSet implements Set, IInterceptable { - 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 (dedupes the source). + const replacementValues = new Set(other as Iterable) + // 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 } From 57c170dd2132ddb3a00f8fd46275425e5993f829 Mon Sep 17 00:00:00 2001 From: Alexander Kireev Date: Sun, 21 Jun 2026 20:07:11 +0700 Subject: [PATCH 2/2] fix(set): address review feedback on ObservableSet.replace Per review on #3761: - Reuse `other` directly when it is already a Set instead of allocating a second one (observable sets are already snapshotted earlier and the Set is only read, never mutated). - Short-circuit the trivial cases: an empty replacement is a plain `clear()`, and replacing into an empty set only needs the adds. - Document the (observable) iteration-order change in the changeset: surviving values now keep their original position and new values are appended rather than the set being reordered to match the argument. - Reorder the replacement arrays in the tests so they assert the resulting iteration order and cover the documented behavior change. --- .changeset/observable-set-replace-events.md | 2 ++ packages/mobx/__tests__/base/set.js | 19 +++++++++++++------ packages/mobx/src/types/observableset.ts | 19 +++++++++++++++++-- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/.changeset/observable-set-replace-events.md b/.changeset/observable-set-replace-events.md index f755ba0521..159df9c6db 100644 --- a/.changeset/observable-set-replace-events.md +++ b/.changeset/observable-set-replace-events.md @@ -3,3 +3,5 @@ --- 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. diff --git a/packages/mobx/__tests__/base/set.js b/packages/mobx/__tests__/base/set.js index 95c83b73e7..1145651c8d 100644 --- a/packages/mobx/__tests__/base/set.js +++ b/packages/mobx/__tests__/base/set.js @@ -523,13 +523,18 @@ describe("#3761 replace only fires events for actual changes", () => { events.push(change) }) - s.replace(["b", "c", "d"]) + // 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: "a", type: "delete" }, + { object: s, oldValue: "b", type: "delete" }, { object: s, newValue: "d", type: "add" } ]) - expect(mobx.values(s)).toEqual(["b", "c", "d"]) + // 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", () => { @@ -552,13 +557,15 @@ describe("#3761 replace only fires events for actual changes", () => { events.push(change) }) - s.replace(new Set([2, 3, 4])) + // 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: 1, type: "delete" }, + { object: s, oldValue: 2, type: "delete" }, { object: s, newValue: 4, type: "add" } ]) - expect(mobx.values(s)).toEqual([2, 3, 4]) + // 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", () => { diff --git a/packages/mobx/src/types/observableset.ts b/packages/mobx/src/types/observableset.ts index 16e4f1c4c8..b5fd92dc03 100644 --- a/packages/mobx/src/types/observableset.ts +++ b/packages/mobx/src/types/observableset.ts @@ -300,8 +300,23 @@ export class ObservableSet implements Set, IInterceptable { - // Collect the desired values for quick lookup (dedupes the source). - const replacementValues = new Set(other as Iterable) + // 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 = isES6Set(other) + ? other + : new Set(other as Iterable) + // 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))) {