Skip to content

Commit

Permalink
fix: incorrect patches for delete on arrays. Fixes #879
Browse files Browse the repository at this point in the history
  • Loading branch information
mweststrate committed Jan 11, 2022
1 parent 0f96270 commit d91a659
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
31 changes: 30 additions & 1 deletion __tests__/patch.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jest.setTimeout(1000)

const isProd = process.env.NODE_ENV === "production"

function runPatchTest(base, producer, patches, inversePathes) {
function runPatchTest(base, producer, patches, inversePathes, expectedResult) {
let resultProxies, resultEs5

function runPatchTestHelper() {
Expand All @@ -26,6 +26,11 @@ function runPatchTest(base, producer, patches, inversePathes) {
recordedInversePatches = i
})

if (expectedResult !== undefined)
test("produced the correct result", () => {
expect(res).toEqual(expectedResult)
})

test("produces the correct patches", () => {
expect(recordedPatches).toEqual(patches)
if (inversePathes) expect(recordedInversePatches).toEqual(inversePathes)
Expand Down Expand Up @@ -1389,3 +1394,27 @@ test("#888 patch to a primitive produces the primitive", () => {
expect(patches).toEqual([{op: "replace", path: [], value: true}])
}
})

describe("#879 delete item from array", () => {
runPatchTest(
[1, 2, 3],
draft => {
delete draft[1]
},
[{op: "replace", path: [1], value: undefined}],
[{op: "replace", path: [1], value: 2}],
[1, undefined, 3]
)
})

describe("#879 delete item from array - 2", () => {
runPatchTest(
[1, 2, 3],
draft => {
delete draft[2]
},
[{op: "replace", path: [2], value: undefined}],
[{op: "replace", path: [2], value: 3}],
[1, 2, undefined]
)
})
3 changes: 2 additions & 1 deletion src/core/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ each(objectTraps, (key, fn) => {
})
arrayTraps.deleteProperty = function(state, prop) {
if (__DEV__ && isNaN(parseInt(prop as any))) die(13)
return objectTraps.deleteProperty!.call(this, state[0], prop)
// @ts-ignore
return arrayTraps.set!.call(this, state, prop, undefined)
}
arrayTraps.set = function(state, prop, value) {
if (__DEV__ && prop !== "length" && isNaN(parseInt(prop as any))) die(14)
Expand Down
8 changes: 8 additions & 0 deletions src/plugins/es5.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ export function enableES5() {

for (let i = 0; i < min; i++) {
// Only untouched indices trigger recursion.
if (!draft_.hasOwnProperty(i)) {
assigned_[i] = true
}
if (assigned_[i] === undefined) markChangesRecursively(draft_[i])
}
}
Expand Down Expand Up @@ -241,12 +244,17 @@ export function enableES5() {
// So if there is no own descriptor on the last position, we know that items were removed and added
// N.B.: splice, unshift, etc only shift values around, but not prop descriptors, so we only have to check
// the last one
// last descriptor can be not a trap, if the array was extended
const descriptor = Object.getOwnPropertyDescriptor(
draft_,
draft_.length - 1
)
// descriptor can be null, but only for newly created sparse arrays, eg. new Array(10)
if (descriptor && !descriptor.get) return true
// if we miss a property, it has been deleted, so array probobaly changed
for (let i = 0; i < draft_.length; i++) {
if (!draft_.hasOwnProperty(i)) return true
}
// For all other cases, we don't have to compare, as they would have been picked up by the index setters
return false
}
Expand Down

0 comments on commit d91a659

Please sign in to comment.