Skip to content

Commit

Permalink
fix: Preserve insertion order of Sets, fixes #819 (#976)
Browse files Browse the repository at this point in the history
* Proposed fix for #819 to preserve Set insertion order

* Proposed fix for #819 (B)

We are already cloning and iterating over the Set. If we go a step further and clear it before iteration, and re-add all children during iteration, we can preserve insertion order.
  • Loading branch information
jdip authored Jan 15, 2023
1 parent b9eae1d commit b3eeb69
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 8 deletions.
34 changes: 34 additions & 0 deletions __tests__/map-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,38 @@ function runBaseTest(name, useProxies, autoFreeze, useListener) {
expect(mapType1).toBe(mapType2)
})
})
describe("set issues " + name, () => {
test("#819.A - maintains order when adding", () => {
const objs = [
"a",
{
id: "b"
}
]

const set = new Set([objs[0]])
const newSet = produce(set, draft => {
draft.add(objs[1])
})

// passes
expect(Array.from(newSet)).toEqual([objs[0], objs[1]])
})

test("#819.B - maintains order when adding", () => {
const objs = [
{
id: "a"
},
"b"
]

const set = new Set([objs[0]])
const newSet = produce(set, draft => {
draft.add(objs[1])
})

expect(Array.from(newSet)).toEqual([objs[0], objs[1]])
})
})
}
22 changes: 15 additions & 7 deletions src/core/finalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,17 @@ function finalize(rootScope: ImmerScope, value: any, path?: PatchPath) {
: state.copy_
// Finalize all children of the copy
// For sets we clone before iterating, otherwise we can get in endless loop due to modifying during iteration, see #628
// Although the original test case doesn't seem valid anyway, so if this in the way we can turn the next line
// back to each(result, ....)
each(
state.type_ === ProxyType.Set ? new Set(result) : result,
(key, childValue) =>
finalizeProperty(rootScope, state, result, key, childValue, path)
// To preserve insertion order in all cases we then clear the set
// And we let finalizeProperty know it needs to re-add non-draft children back to the target
let resultEach = result
let isSet = false
if (state.type_ === ProxyType.Set) {
resultEach = new Set(result)
result.clear()
isSet = true
}
each(resultEach, (key, childValue) =>
finalizeProperty(rootScope, state, result, key, childValue, path, isSet)
)
// everything inside is frozen, we can freeze here
maybeFreeze(rootScope, result, false)
Expand All @@ -115,7 +120,8 @@ function finalizeProperty(
targetObject: any,
prop: string | number,
childValue: any,
rootPath?: PatchPath
rootPath?: PatchPath,
targetIsSet?: boolean
) {
if (__DEV__ && childValue === targetObject) die(5)
if (isDraft(childValue)) {
Expand All @@ -134,6 +140,8 @@ function finalizeProperty(
if (isDraft(res)) {
rootScope.canAutoFreeze_ = false
} else return
} else if (targetIsSet) {
targetObject.add(childValue)
}
// Search new objects for unfinalized drafts. Frozen objects should never contain drafts.
if (isDraftable(childValue) && !isFrozen(childValue)) {
Expand Down
1 change: 0 additions & 1 deletion src/utils/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ export function set(thing: any, propOrOldValue: PropertyKey, value: any) {
const t = getArchtype(thing)
if (t === Archtype.Map) thing.set(propOrOldValue, value)
else if (t === Archtype.Set) {
thing.delete(propOrOldValue)
thing.add(value)
} else thing[propOrOldValue] = value
}
Expand Down

0 comments on commit b3eeb69

Please sign in to comment.