Merge e52f02f into 211f5cf

__tests__/base.js

@@ -240,6 +240,76 @@ function runBaseTest(name, useProxies, freeze, useListener) {
             expect(nextState.anArray).toEqual([3, 2, {c: 3}])
         })
 
+        it("can delete array items - 2", () => {
+            const nextState = produce(
+                baseState,
+                s => {
+                    delete s.anArray[1]
+                },
+                listener
+            )
+            expect(nextState).not.toBe(baseState)
+            expect(nextState.anArray).not.toBe(baseState.anArray)
+            expect(Object.keys(nextState.anArray)).toEqual(["0", "2", "3"])
+            expect(nextState.anArray.length).toBe(4)
+            expect(nextState.anArray).toEqual([3, undefined, {c: 3}, 1])
+        })
+
+        it("can add non-index key on array", () => {
+            let state = {
+                anArray: [1, 2, 3]
+            }
+            const nextState = produce(
+                state,
+                s => {
+                    s.anArray.name = "name"
+                },
+                listener
+            )
+            expect(nextState).not.toBe(state)
+            expect(nextState.anArray).not.toBe(state.anArray)
+            expect(Object.keys(nextState.anArray)).toEqual([
+                "0",
+                "1",
+                "2",
+                "name"
+            ])
+        })
+
+        it("can delete array items - 3", () => {
+            let state = {
+                anArray: [1, 2, 3]
+            }
+            const nextState = produce(
+                state,
+                s => {
+                    s.anArray.name = "name"
+                    delete s.anArray[1]
+                },
+                listener
+            )
+            expect(nextState).not.toBe(state)
+            expect(nextState.anArray).not.toBe(state.anArray)
+            expect(Object.keys(nextState.anArray)).toEqual(["0", "2", "name"])
+        })
+
+        it("can delete array items - 4", () => {
+            let state = {
+                anArray: [1, 2, 3]
+            }
+            state.anArray.name = "anArray"
+            const nextState = produce(
+                state,
+                s => {
+                    delete s.anArray.name
+                },
+                listener
+            )
+            expect(nextState).not.toBe(state)
+            expect(nextState.anArray).not.toBe(state.anArray)
+            expect(Object.keys(nextState.anArray)).toEqual(["0", "1", "2"])
+        })
+
         it("should support sorting arrays", () => {
             const nextState = produce(
                 baseState,
@@ -509,7 +579,7 @@ function runBaseTest(name, useProxies, freeze, useListener) {
                     expect(1 in draft).toBe(false)
                     expect("0" in draft).toBe(true)
                     expect("1" in draft).toBe(false)
-                    expect(length in draft).toBe(true)
+                    expect("length" in draft).toBe(true)
                     expect(
                         Reflect.ownKeys(draft).filter(
                             x => typeof x === "string"

__tests__/patch.js

@@ -200,6 +200,90 @@ describe("arrays - 6", () => {
     )
 })
 
+describe("arrays - 7", () => {
+    runPatchTest(
+        {x: [1, 2, 3]},
+        d => {
+            delete d.x[1]
+            d.x.push(100)
+        },
+        [
+            {op: "remove", path: ["x", 1]},
+            {op: "add", path: ["x", 3], value: 100}
+        ],
+        [
+            {op: "add", path: ["x", 1], value: 2},
+            {op: "replace", path: ["x", "length"], value: 3}
+        ]
+    )
+})
+
+describe("arrays - 8", () => {
+    runPatchTest(
+        {x: [1, 2, 3]},
+        d => {
+            delete d.x[1]
+            d.x.name = "anArray"
+        },
+        [
+            {op: "remove", path: ["x", 1]},
+            {op: "add", path: ["x", "name"], value: "anArray"}
+        ],
+        [
+            {op: "add", path: ["x", 1], value: 2},
+            {op: "remove", path: ["x", "name"]}
+        ]
+    )
+})
+
+describe("arrays - 9", () => {
+    runPatchTest(
+        {x: [1, 2, {name: "jim"}]},
+        d => {
+            delete d.x[2].name
+        },
+        [{op: "remove", path: ["x", 2, "name"]}],
+        [{op: "add", path: ["x", 2, "name"], value: "jim"}]
+    )
+})
+
+describe("arrays - 10", () => {
+    runPatchTest(
+        {x: [1, 2, {name: "jim"}]},
+        d => {
+            d.x[2].age = 30
+        },
+        [{op: "add", path: ["x", 2, "age"], value: 30}],
+        [{op: "remove", path: ["x", 2, "age"]}]
+    )
+})
+
+describe("arrays - 11", () => {
+    let state = new Array(3)
+    state[2] = 3
+    runPatchTest(
+        state,
+        d => {
+            d[1] = 4
+        },
+        [{op: "add", path: [1], value: 4}],
+        [{op: "remove", path: [1]}]
+    )
+})
+
+describe("arrays - 12", () => {
+    let state = [1, 2, 3]
+    state.name = "anArray"
+    runPatchTest(
+        state,
+        d => {
+            delete d.name
+        },
+        [{op: "remove", path: ["name"]}],
+        [{op: "add", path: ["name"], value: "anArray"}]
+    )
+})
+
 describe("simple replacement", () => {
     runPatchTest({x: 3}, _d => 4, [{op: "replace", path: [], value: 4}])
 })

package.json

@@ -86,6 +86,7 @@
       "^.+\\.jsx?$": "babel-jest",
       "^.+\\.tsx?$": "ts-jest"
     },
+    "testEnvironment": "node",
     "testRegex": "/__tests__/[^/]*[jt]sx?$",
     "globals": {
       "ts-jest": {

src/common.js

@@ -68,16 +68,27 @@ const assign =
     }
 
 export function shallowCopy(value) {
-    if (Array.isArray(value)) return value.slice()
+    if (Array.isArray(value)) {
+        return assign([], value)
+    }
     const target = value.__proto__ === undefined ? Object.create(null) : {}
     return assign(target, value)
 }
 
 export function each(value, cb) {
-    if (Array.isArray(value)) {
-        for (let i = 0; i < value.length; i++) cb(i, value[i])
-    } else {
-        for (let key in value) cb(key, value[key])
+    for (let key in value) {
+        if (Array.isArray(value) && isNonNegativeInteger(key)) key = Number(key)
+        cb(key, value[key])
+    }
+}
+
+export function diffKeys(from, to) {
+    // TODO: optimize
+    const a = Object.keys(from)
+    const b = Object.keys(to)
+    return {
+        added: b.filter(key => a.indexOf(key) === -1),
+        removed: a.filter(key => b.indexOf(key) === -1)
     }
 }
 
@@ -166,3 +177,8 @@ export function is(x, y) {
         return x !== x && y !== y
     }
 }
+
+export function isNonNegativeInteger(v) {
+    const reg = /^\d+$/
+    return reg.test(v)
+}

src/es5.js

@@ -9,6 +9,7 @@ import {
     shallowCopy,
     RETURNED_AND_MODIFIED_ERROR,
     each,
+    diffKeys,
     finalize
 } from "./common"
 
@@ -128,43 +129,20 @@ function markChangesRecursively(object) {
     const state = object[PROXY_STATE]
     if (!state) return
     const {proxy, base} = state
-    if (Array.isArray(object)) {
-        if (hasArrayChanges(state)) {
-            markChanged(state)
-            state.assigned.length = true
-            if (proxy.length < base.length)
-                for (let i = proxy.length; i < base.length; i++)
-                    state.assigned[i] = false
-            else
-                for (let i = base.length; i < proxy.length; i++)
-                    state.assigned[i] = true
-            each(proxy, (index, child) => {
-                if (!state.assigned[index]) markChangesRecursively(child)
-            })
-        }
-    } else {
-        const {added, removed} = diffKeys(base, proxy)
-        if (added.length > 0 || removed.length > 0) markChanged(state)
-        each(added, (_, key) => {
-            state.assigned[key] = true
-        })
-        each(removed, (_, key) => {
-            state.assigned[key] = false
-        })
-        each(proxy, (key, child) => {
-            if (!state.assigned[key]) markChangesRecursively(child)
-        })
-    }
-}
 
-function diffKeys(from, to) {
-    // TODO: optimize
-    const a = Object.keys(from)
-    const b = Object.keys(to)
-    return {
-        added: b.filter(key => a.indexOf(key) === -1),
-        removed: a.filter(key => b.indexOf(key) === -1)
-    }
+    // See test case patch.js "arrays - 9" "arrays - 10"
+    // Here, if we call Array.isArray and hasArrayChanges, will not detecte the change of object which is in array
+    const {added, removed} = diffKeys(base, proxy)
+    if (added.length > 0 || removed.length > 0) markChanged(state)
+    each(added, (_, key) => {
+        state.assigned[key] = true
+    })
+    each(removed, (_, key) => {
+        state.assigned[key] = false
+    })
+    each(proxy, (key, child) => {
+        if (!state.assigned[key]) markChangesRecursively(child)
+    })
 }
 
 function hasObjectChanges(state) {
@@ -176,6 +154,10 @@ function hasObjectChanges(state) {
 function hasArrayChanges(state) {
     const {proxy} = state
     if (proxy.length !== state.base.length) return true
+
+    // See test case base.js "can delete array items - 3"  Use Object.keys().length equal is not effective
+    if (!shallowEqual(Object.keys(proxy), Object.keys(state.base))) return true
+
     // See #116
     // If we first shorten the length, our array interceptors will be removed.
     // If after that new items are added, result in the same original length,

src/patches.js

@@ -1,4 +1,4 @@
-import {each} from "./common"
+import {each, has, diffKeys, isNonNegativeInteger} from "./common"
 
 export function generatePatches(
     state,
@@ -39,6 +39,20 @@ export function generateArrayPatches(
 ) {
     const shared = Math.min(baseValue.length, resultValue.length)
     for (let i = 0; i < shared; i++) {
+        if (!has(resultValue, i) && has(baseValue, i)) {
+            var path = basepath.concat(i)
+            patches.push({op: "remove", path: path})
+            inversePatches.push({op: "add", path: path, value: baseValue[i]})
+            continue
+        }
+
+        if (has(resultValue, i) && !has(baseValue, i)) {
+            var path = basepath.concat(i)
+            patches.push({op: "add", path: path, value: resultValue[i]})
+            inversePatches.push({op: "remove", path: path})
+            continue
+        }
+
         if (state.assigned[i] && baseValue[i] !== resultValue[i]) {
             const path = basepath.concat(i)
             patches.push({op: "replace", path, value: resultValue[i]})
@@ -68,6 +82,21 @@ export function generateArrayPatches(
             inversePatches.push({op: "add", path, value: baseValue[i]})
         }
     }
+
+    //handle the non-index keys
+    let {added, removed} = diffKeys(baseValue, resultValue)
+    added = added.filter(v => !isNonNegativeInteger(v))
+    removed = removed.filter(v => !isNonNegativeInteger(v))
+    each(added, (_, key) => {
+        const path = basepath.concat(key)
+        patches.push({op: "add", path: path, value: resultValue[key]})
+        inversePatches.push({op: "remove", path: path})
+    })
+    each(removed, (_, key) => {
+        const path = basepath.concat(key)
+        patches.push({op: "remove", path: path})
+        inversePatches.push({op: "add", path: path, value: baseValue[key]})
+    })
 }
 
 function generateObjectPatches(
@@ -83,16 +112,18 @@ function generateObjectPatches(
         const value = resultValue[key]
         const op = !assignedValue
             ? "remove"
-            : key in baseValue ? "replace" : "add"
+            : key in baseValue
+                ? "replace"
+                : "add"
         if (origValue === baseValue && op === "replace") return
         const path = basepath.concat(key)
         patches.push(op === "remove" ? {op, path} : {op, path, value})
         inversePatches.push(
             op === "add"
                 ? {op: "remove", path}
                 : op === "remove"
-                  ? {op: "add", path, value: origValue}
-                  : {op: "replace", path, value: origValue}
+                    ? {op: "add", path, value: origValue}
+                    : {op: "replace", path, value: origValue}
         )
     })
 }
@@ -125,15 +156,7 @@ export function applyPatches(draft, patches) {
                     base[key] = patch.value
                     break
                 case "remove":
-                    if (Array.isArray(base)) {
-                        if (key === base.length - 1) base.length -= 1
-                        else
-                            throw new Error(
-                                `Remove can only remove the last key of an array, index: ${key}, length: ${
-                                    base.length
-                                }`
-                            )
-                    } else delete base[key]
+                    delete base[key]
                     break
                 default:
                     throw new Error("Unsupported patch operation: " + patch.op)