fix: objects with constructor property in deepExtend + test (#23)

* test: add deepFreeze helper

* test: add deepFreeze test

* fix: objects with constructor property in deepExtend

Before this fix object like { constructor: { … } } wouldn't be treated
as an ordinary object but would be copied by reference.

src/util.ts

@@ -358,20 +358,16 @@ export function selectiveNotDeepExtend(propsToExclude: string[], a: any, b: any,
 export function deepExtend(a: any, b: any, protoExtend: boolean = false, allowDeletion: boolean = false): any {
   for (const prop in b) {
     if (Object.prototype.hasOwnProperty.call(b, prop) || protoExtend === true) {
-      if (b[prop] && b[prop].constructor === Object) {
+      if (b[prop] && Object.getPrototypeOf(b[prop]) === Object.prototype) {
         if (a[prop] === undefined) {
-          a[prop] = {}
-        }
-        if (a[prop].constructor === Object) {
+          a[prop] = deepExtend({}, b[prop], protoExtend) // NOTE: allowDeletion not propagated!
+        } else if (a[prop] && Object.getPrototypeOf(a[prop]) === Object.prototype) {
           deepExtend(a[prop], b[prop], protoExtend) // NOTE: allowDeletion not propagated!
         } else {
           copyOrDelete(a, b, prop, allowDeletion)
         }
       } else if (Array.isArray(b[prop])) {
-        a[prop] = []
-        for (let i = 0; i < b[prop].length; i++) {
-          a[prop].push(b[prop][i])
-        }
+        a[prop] = b[prop].slice()
       } else {
         copyOrDelete(a, b, prop, allowDeletion)
       }

test/deep-extend.test.ts

@@ -0,0 +1,238 @@
+import { expect } from 'chai'
+import { deepFreeze } from './helpers'
+
+import { deepExtend } from '../src'
+
+describe('deepExtend', function(): void {
+  it('nested strings', function(): void {
+    const target = {
+      p1: 'p1 T',
+      p2: 'p2 T',
+      p4: {
+        p4p1: 'p4p1 T',
+        p4p2: 'p4p2 T',
+        p4p4: {
+          p4p4p1: 'p4p4p1 T',
+          p4p4p2: 'p4p4p2 T',
+        },
+      },
+    }
+    const source = deepFreeze({
+      p2: 'p2 S',
+      p3: 'p3 S',
+      p4: {
+        p4p2: 'p4p2 S',
+        p4p3: 'p4p3 S',
+        p4p4: {
+          p4p4p2: 'p4p4p2 S',
+          p4p4p3: 'p4p4p3 S',
+        },
+      },
+    })
+
+    const merged = deepExtend(target, source)
+
+    expect(merged, 'They should be the same instance.').to.equal(target)
+    expect(merged, 'All the properties should be deeply merged.').to.deep.equal({
+      p1: 'p1 T',
+      p2: 'p2 S',
+      p3: 'p3 S',
+      p4: {
+        p4p1: 'p4p1 T',
+        p4p2: 'p4p2 S',
+        p4p3: 'p4p3 S',
+        p4p4: {
+          p4p4p1: 'p4p4p1 T',
+          p4p4p2: 'p4p4p2 S',
+          p4p4p3: 'p4p4p3 S',
+        },
+      },
+    })
+  })
+
+  it('arrays', function(): void {
+    const target = {
+      arrays: {
+        p1: ['T', 1, true, 'T'],
+        p2: ['T', 1, true, 'T'],
+      },
+    }
+    const source = deepFreeze({
+      arrays: {
+        p2: ['S', false, 0, 'S'],
+        p3: ['S', false, 0, 'S'],
+      },
+    })
+
+    const merged = deepExtend(target, source)
+
+    expect(merged, 'They should be the same instance.').to.equal(target)
+    expect(merged, 'Objects inheriting directly from Object should be deeply merged, arrays replaced.').to.deep.equal({
+      arrays: {
+        p1: ['T', 1, true, 'T'],
+        p2: ['S', false, 0, 'S'],
+        p3: ['S', false, 0, 'S'],
+      },
+    })
+
+    expect(merged.arrays.p2, 'Array should not be copied by reference.').to.not.equal(source.arrays.p2)
+    expect(merged.arrays.p2, 'Array should not be copied by reference.').to.not.equal(source.arrays.p2)
+  })
+
+  it('objects with other than Object prototype', function(): void {
+    const objectLiteral = { p3: 'S' }
+    const objectFromNull = Object.create(null)
+    const objectFromObject = Object.create(Object)
+    const objectFromMap = new Map()
+
+    const target = {
+      objects: {
+        objectLiteral: { p1: 'T' },
+      },
+    }
+    const source = deepFreeze({
+      objects: {
+        objectLiteral,
+        objectFromNull,
+        objectFromObject,
+        objectFromMap,
+      },
+    })
+
+    const merged = deepExtend(target, source)
+
+    expect(merged, 'They should be the same instance.').to.equal(target)
+    expect(merged, 'Objects inheriting directly from Object should be deeply merged, other replaced.').to.deep.equal({
+      objects: {
+        objectLiteral: {
+          p1: 'T',
+          p3: 'S',
+        },
+        objectFromNull: {},
+        objectFromObject: {},
+        objectFromMap: new Map(),
+      },
+    })
+
+    expect(merged.objects.objectLiteral, 'Object literal should not be copied by reference.').to.not.equal(
+      source.objects.objectLiteral
+    )
+    expect(merged.objects.objectFromNull, 'Object created from null should be copied by reference.').to.equal(
+      source.objects.objectFromNull
+    )
+    expect(merged.objects.objectFromObject, 'Object created from null should be copied by reference.').to.equal(
+      source.objects.objectFromObject
+    )
+    expect(merged.objects.objectFromMap, 'Object created from null should be copied by reference.').to.equal(
+      source.objects.objectFromMap
+    )
+  })
+
+  describe('inherited properties', function(): void {
+    it('ignored by default', function(): void {
+      const target = {}
+      const source = deepFreeze(
+        Object.create(
+          deepFreeze({
+            inherited: 'S',
+          })
+        )
+      )
+
+      const merged = deepExtend(target, source)
+
+      expect(merged, 'They should be the same instance.').to.equal(target)
+      expect(merged, 'Inherited properties shouldn’t be inherited by default.').to.deep.equal({})
+    })
+
+    it('inherited if enabled', function(): void {
+      const target = {}
+      const source = deepFreeze(
+        Object.create(
+          deepFreeze({
+            inherited: 'S',
+          })
+        )
+      )
+
+      const merged = deepExtend(target, source, true)
+
+      expect(merged, 'They should be the same instance.').to.equal(target)
+      expect(merged, 'Inherited properties should be inherited when enabled.').to.deep.equal({
+        inherited: 'S',
+      })
+    })
+  })
+
+  describe('deletion', function(): void {
+    it('disabled', function(): void {
+      const target = {
+        p1: 'p1 T',
+        p2: 'p2 T',
+      }
+      const source = deepFreeze({
+        p2: null,
+        p3: null,
+      })
+
+      const merged = deepExtend(target, source)
+
+      expect(merged, 'They should be the same instance.').to.equal(target)
+      expect(merged, 'No properties should be deleted unless enabled.').to.deep.equal({
+        p1: 'p1 T',
+        p2: null,
+        p3: null,
+      })
+    })
+
+    it('enabled', function(): void {
+      const target = {
+        p1: 'p1 T',
+        p2: 'p2 T',
+      }
+      const source = deepFreeze({
+        p2: null,
+        p3: null,
+      })
+
+      const merged = deepExtend(target, source, false, true)
+
+      expect(merged, 'They should be the same instance.').to.equal(target)
+      expect(merged, 'Null properties from the source should delete matching properties in the target.').to.deep.equal({
+        p1: 'p1 T',
+        p3: null, // TODO: This seems wrong.
+      })
+    })
+  })
+
+  describe('edge cases', function(): void {
+    it('constructor property', function(): void {
+      const target = {
+        object: {
+          constructor: {
+            p1: 'T',
+          },
+        },
+      }
+      const source = deepFreeze({
+        object: {
+          constructor: {
+            p3: 'S',
+          },
+        },
+      })
+
+      const merged = deepExtend(target, source)
+
+      expect(merged, 'They should be the same instance.').to.equal(target)
+      expect(merged, 'All the properties should be deeply merged.').to.deep.equal({
+        object: {
+          constructor: {
+            p1: 'T',
+            p3: 'S',
+          },
+        },
+      })
+    })
+  })
+})

test/helpers/index.ts

@@ -0,0 +1,50 @@
+/**
+ * Prevents supplied object from being modified.
+ *
+ * @remarks
+ * Has no problem with cycles.
+ * Can receive frozen or parially frozen objects.
+ *
+ * @typeParam T - The type of the object.
+ *
+ * @param object - The object to be recursively frozen.
+ * @param freeze - Function that does the freezing. Object.seal or anything with the same API can be used instead.
+ *
+ * @returns The frozen object (the same instance as object param).
+ */
+export function deepFreeze<T extends object>(object: T, freeze: (object: any) => any = Object.freeze.bind(Object)): T {
+  const alreadyFrozen = new Set<any>()
+
+  /**
+   * Recursivelly freezes objects using alreadyFrozen to prevent infinite cycles.
+   *
+   * @param object - The object to be recursively frozen.
+   *
+   * @returns The frozen object (the same instance as object param).
+   */
+  function recursivelyFreeze(object: any): any {
+    // Prevent double freezing (could lead to stack overflow due to an infinite cycle)
+    // Object.isFrozen is not used here because frozen objects can have unfrozen objects in their properties.
+    if (alreadyFrozen.has(object)) {
+      return object
+    }
+    alreadyFrozen.add(object)
+
+    // Retrieve the property names defined on object
+    const names = Object.getOwnPropertyNames(object)
+
+    // Freeze properties before freezing the object
+    for (const name of names) {
+      const prop = object[name]
+      if (prop && typeof prop === 'object') {
+        recursivelyFreeze(prop)
+      } else {
+        freeze(prop)
+      }
+    }
+
+    return freeze(object)
+  }
+
+  return recursivelyFreeze(object)
+}