fix: Fix memory leak fast-copying nested instance

Ok, it’s not a real memory leak, but we don’t want extra instances in memory.  This makes sure that fastCopy is never used on the first level of a model instance.  (It works fine on nested model instances, because it always calls new prop.constructor on nested objects)

It includes lots of tests for cloning and commiting.

Author: marshallswain

src/utils.ts

@@ -419,10 +419,14 @@ export function mergeWithAccessors(
       return
     }
 
-    // If we're dealing with a Vue Observable, just assign the values.
+    // Handle Vue observable objects
     if (destIsVueObservable || sourceIsVueObservable) {
-      if (_isObject(source[key])) {
+      const isObject = _isObject(source[key])
+      const isFeathersVuexInstance = isObject && !!(source[key].constructor.modelName || source[key].constructor.namespace)
+      // Do not use fastCopy directly on a feathers-vuex BaseModel instance to keep from breaking reactivity.
+      if (isObject && !isFeathersVuexInstance) {
         try {
+          const sourceObject = source[key]
           dest[key] = fastCopy(source[key])
         } catch (err) {
           if(!err.message.includes('getter')) {

test/service-module/model-relationships.test.ts

@@ -152,6 +152,9 @@ function makeContext() {
       super(data, options)
     }
   }
+  /**
+   * This Model demonstrates how to use a dynamic set of instanceDefaults based on incoming data.
+   */
   class Todo extends BaseModel {
     public static modelName = 'Todo'
     public static instanceDefaults(data) {
@@ -529,4 +532,71 @@ describe('Models - Relationships', function () {
 
     assert.equal(todo.task.isComplete, false, 'preserved after clone')
   })
+
+  it('returns the same object when an instance is cloned twice', function () {
+    const { Todo } = makeContext()
+
+    const todo = new Todo({
+      task: {
+        id: 1,
+        description: 'test',
+        isComplete: true
+      }
+    })
+
+    const clone1 = todo.clone()
+    const clone2 = todo.clone()
+
+    assert(clone1 === clone2, 'there should only ever be one clone in memory for an instance with the same id')
+  })
+
+  it('on clone, nested instances do not get cloned', function () {
+    const { Todo } = makeContext()
+
+    const todo = new Todo({
+      task: {
+        id: 1,
+        description: 'test',
+        isComplete: true
+      }
+    })
+
+    const todoClone = todo.clone()
+
+    assert(todoClone.task.__isClone === undefined, 'todo.task should still be the original item and not the clone')
+  })
+
+  it('on nested commit in instance, original nested instances get updated', function () {
+    const { Todo } = makeContext()
+
+    const todo = new Todo({
+      task: {
+        id: 1,
+        description: 'test',
+        isComplete: true
+      }
+    })
+
+    const taskClone = todo.task.clone()
+
+    taskClone.description = 'changed'
+    taskClone.commit()
+
+    assert(todo.task.description === 'changed', 'the nested task should have been updated')
+  })
+
+  it('nested instances get updated in clones and original records', function () {
+    const { Todo } = makeContext()
+
+    const todo = new Todo({
+      task: {
+        id: 1,
+        description: 'test',
+        isComplete: true
+      }
+    })
+    const todoClone = todo.clone()
+
+    assert(todo.task === todoClone.task, 'the same task instance should be in both the original and clone')
+  })
 })

test/service-module/service-module.test.ts

@@ -49,6 +49,11 @@ function makeContext() {
     public constructor(data, options?) {
       super(data, options)
     }
+    public static instanceDefaults(data) {
+      return {
+        description: ''
+      }
+    }
   }
   class HotspotMedia extends BaseModel {
     public static modelName = 'HotspotMedia'
@@ -304,7 +309,42 @@ describe('Service Module', function () {
       )
     })
 
-    it(`no longer changes original if you don't use the return value of commit()`, function () {
+    it(`the object returned from clone is not the same as the original`, function () {
+      const { serviceTodo, owners } = this
+      const serviceTodoClone = serviceTodo.clone()
+
+      assert(serviceTodo !== serviceTodoClone, 'the objects are distinct')
+    })
+
+    it(`the object returned from commit is not the same as the clone`, function () {
+      const { serviceTodo, owners } = this
+      const serviceTodoClone = serviceTodo.clone()
+      const committedTodo = serviceTodoClone.commit()
+
+      assert(committedTodo !== serviceTodoClone, 'the objects are distinct')
+    })
+
+    it(`the object returned from commit is the same as the original`, function () {
+      const { serviceTodo, owners } = this
+      const serviceTodoClone = serviceTodo.clone()
+      const committedTodo = serviceTodoClone.commit()
+
+      assert(serviceTodo === committedTodo, 'the objects are the same')
+    })
+
+    it(`nested arrays are distinct after clone`, function () {
+      const { ServiceTodo } = this
+
+      const todo = new ServiceTodo({
+        description: 'test',
+        owners: ['Marshall', 'Mariah']
+      })
+      const clone = todo.clone()
+
+      assert(todo.owners !== clone.owners, 'the arrays are not the same in memory')
+    })
+
+    it.skip(`modifying a clone after calling commit() does not change the original `, function () {
       const { serviceTodo, owners } = this
       const serviceTodoClone = serviceTodo.clone()