handle arrays creation in apply(). Ignore keys that have undefined va…

…lue in diff()

source/server/utils/merge/apply.test.ts

@@ -3,7 +3,7 @@ import {DELETE_KEY} from "./types.js";
 import apply from "./apply.js";
 
 describe("merge.apply()", function(){
-  it("merges a no-op", function(){
+  it("applies a no-op", function(){
     const a = {a:1};
     expect(apply(a, {})).to.deep.equal(a);
   });
@@ -15,11 +15,17 @@ describe("merge.apply()", function(){
   it("merges an updated key", function(){
     expect(apply({a:1, b:2}, {b:3})).to.deep.equal({a:1, b:3});
   });
+
   it("merges nested objects", function(){
     expect(apply({a:{b:1}, c:3}, {a:{b:2}})).to.deep.equal({a:{b:2}, c:3});
   });
+
   it("merges updated arrays", function(){
     expect(apply({a:[1,2]}, {a:{0:1, 1:3}})).to.deep.equal({a:[1,3]});
   });
 
+  it("merges created arrays", function(){
+    expect(apply({a:"A"}, { b:[{name:"A1"}]} as any)).to.deep.equal({a:"A", b:[{name:"A1"}]});
+  })
+
 });

source/server/utils/merge/apply.ts

@@ -66,7 +66,11 @@ function apply_array<T extends Array<any>>(into :T, ...diffs :Diff<T>[]):T{
 }
 
 /**
- * Return true if the value was applied, false if it needs further processing
+ * Return true if the value was applied, false if it needs further processing.
+ * Handles the trivial cases:
+ *  - Applies DELETE_KEY
+ *  - Applies primitives (typeof value !== "object")
+ *  - Applies properties that don't exist in the target
  */
 function apply_core<T extends Record<string, any>>(into :T, key:keyof T, value :any):boolean{
 
@@ -75,8 +79,8 @@ function apply_core<T extends Record<string, any>>(into :T, key:keyof T, value :
     return true;
   }
 
-  if(typeof value !== "object"){
-    //Handle primitives
+  if(typeof value !== "object" || typeof into[key] === "undefined"){
+    //Handle primitives and undefined values
     into[key] = value;
     return true;
   }

source/server/utils/merge/diff.test.ts

@@ -32,7 +32,15 @@ describe("merge.diff()", function(){
       expect(diff({
         v: {a:1},
       }, {v: null}))
-    })
+    });
+
+    it("ignores undefined values", function(){
+      //keys that are undefined will not be serialized so we can safely ignore them
+      expect(diff({v:undefined}, {v:undefined})).to.deep.equal({});
+      expect(diff({v:undefined}, {})).to.deep.equal({});
+      expect(diff({}, {v:undefined})).to.deep.equal({});
+    });
+
   });
 
   describe("arrays", function(){
@@ -65,6 +73,10 @@ describe("merge.diff()", function(){
       expect(diff({a:["foo", "bar", "baz"]}, {a:["foo", "baz"]})).to.deep.equal({a:{1: "baz", 2: DELETE_KEY}});
     });
 
+    it("handle Array creation", function(){
+      expect(diff({name:"A"}, {name:"A", articles: [{id:"1"}]})).to.deep.equal({articles: [{id:"1"}]});
+    });
+
     it("handle deep changes", function(){
       expect(diff({a:[{v:"foo"}, {v:"bar"}]}, {a:[{v:"foo"}, {v:"baz"}]})).to.deep.equal({a:{1:{v:"baz"}}});
     });

source/server/utils/merge/diff.ts

@@ -17,7 +17,7 @@ export default function diff<T extends Record<string,any>>(from :T, to :T) :Diff
   for(const key of keys.values()){
 
     if(typeof to[key] == "undefined"){
-      r[key] = DELETE_KEY;
+      if(typeof from[key] != "undefined") r[key] = DELETE_KEY;
       continue;
     }
 

source/server/utils/merge/merge.test.ts

@@ -47,7 +47,7 @@ describe("three-way merge", function(){
         );
       });
 
-      it("handle double array push (annotations)", function(){
+      it("handle double push (annotations)", function(){
         const ref = {annotations: [A[0]]};
         const current = {annotations: [A[0], A[1]]};
         const next = {annotations: [A[0], A[2]]};
@@ -57,7 +57,7 @@ describe("three-way merge", function(){
         expect(apply(current, d)).to.deep.equal({annotations: [A[0], A[1], A[2]]});
       });
 
-      it("handle array modify in place", function(){
+      it("handle in-place edits", function(){
         const m = {...A[0], titles: {EN: "A1 modified"}}
         const ref = {annotations: [A[0]]};
         const current = {annotations: [A[0], A[1]]};
@@ -80,7 +80,7 @@ describe("three-way merge", function(){
         );
       });
 
-      it("handle double array push", function(){
+      it("handle double push", function(){
         const ref = {nodes: [N[0]]};
         const current = {nodes: [N[0], N[1]]};
         const next = {nodes: [N[0], N[2]]};
@@ -144,5 +144,15 @@ describe("merge documents", function(){
       {type: "ambiant"},
       {type:"directional"}
     ])
+  });
+
+  it("detects a no-op", function(){
+    const current = JSON.parse(docString);
+    const next = JSON.parse(docString);
+    const d = diffDoc(doc, next);
+    expect(d, JSON.stringify(d)).to.deep.equal({});
+
+    const result = applyDoc(current, d);
+    expect(result).to.deep.equal(current);
   })
 })

source/server/utils/merge/pointers.ts

@@ -12,7 +12,9 @@ type DocumentCollections = keyof Omit<IDocument, "asset"|"scene">;
  * It unlinks all unreachable nodes (scenes, setups, etc...).
  * 
  * It's something of an emulation of what DPO-voyager does when building a scene graph from a document.
- * See CVScene.ts and CVNode.ts in voyager. 
+ * See [CVScene.ts](https://github.com/Smithsonian/dpo-voyager/blob/master/source/client/components/CVScene.ts) 
+ * and [CVNode.ts](https://github.com/Smithsonian/dpo-voyager/blob/master/source/client/components/CVNode.ts) in voyager.
+ * 
  */
 export function toPointers(src :IDocument) :DerefDocument {
   const nodes = src.nodes?.map(iNode=>{