fix(core): properly handle swapping trailing element in FastIterableD…

…iffer
trotyl · May 14, 2018 · bdbf022 · bdbf022
1 parent d78aee2
commit bdbf022
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 17 deletions.
diff --git a/packages/core/fast-iterable-differ/container.ts b/packages/core/fast-iterable-differ/container.ts
@@ -11,17 +11,24 @@ export class DiffContainer implements Container {
   }
 
   append(node: ItemNode): void {
-    this.assertInactive(node);
+    let previousIndex = null;
+    if (node.to != null) {
+      previousIndex = node.to;
+      this.remove(node, false);
+    }
     node.to = this.list.length;
+    if (node.link != null) {
+      node.link.to = this.list.length;
+    }
     this.operations.push({
       node,
-      previousIndex: null,
+      previousIndex,
       currentIndex: node.to,
     });
     this.list.push(node);
   }
 
-  remove(nodeToRemove: ItemNode): void {
+  remove(nodeToRemove: ItemNode, isOperation: boolean = true): void {
     this.assertActive(nodeToRemove);
     for (let i = nodeToRemove.to! + 1; i < this.list.length; i++) {
       const node = this.list[i];
@@ -30,15 +37,19 @@ export class DiffContainer implements Container {
         node.link.to!--;
       }
     }
-    this.operations.push({
-      node: nodeToRemove,
-      previousIndex: nodeToRemove.to,
-      currentIndex: null,
-    });
+    if (isOperation) {
+      this.operations.push({
+        node: nodeToRemove,
+        previousIndex: nodeToRemove.to,
+        currentIndex: null,
+      });
+    }
     this.list.splice(nodeToRemove.to!, 1);
-    nodeToRemove.to = null;
-    if (nodeToRemove.link != null) {
-      nodeToRemove.link.to = null;
+    if (isOperation) {
+      nodeToRemove.to = null;
+      if (nodeToRemove.link != null) {
+        nodeToRemove.link.to = null;
+      }
     }
   }
 
@@ -111,10 +122,6 @@ export class DiffContainer implements Container {
   }
 
   private assertActive(node: ItemNode): void {
-    if (node.to == null) { throw new Error(`00`); }
-  }
-
-  private assertInactive(node: ItemNode): void {
-    if (node.to != null) { throw new Error(`01`); }
+    if (node.to == null) { throw new Error(`Expected item ${node.item} to be in list`); }
   }
 }
diff --git a/packages/core/fast-iterable-differ/fast-iterable-differ.spec.ts b/packages/core/fast-iterable-differ/fast-iterable-differ.spec.ts
@@ -86,6 +86,23 @@ describe('FastIterableDiffer', function () {
     }));
   });
 
+  // https://github.com/krausest/js-framework-benchmark/pull/389
+  it('should handle swapping internal and trailing elements', () => {
+    const l = [1, 2, 3, 4, 5, 6];
+    differ.check(l);
+
+    const a = l[1];
+    l[1] = l[5];
+    l[5] = a;
+
+    differ.check(l);
+    expect(iterableDifferToString(differ)).toEqual(iterableChangesAsString({
+      collection: ['1', '6[5->1]', '3', '4', '5', '2[1->5]'],
+      previous: ['1', '2[1->5]', '3', '4', '5', '6[5->1]'],
+      moves: ['6[5->1]', '2[1->5]'],
+    }));
+  });
+
   it('should handle incremental swapping element', () => {
     const l = ['a', 'b', 'c'];
     differ.check(l);

diff --git a/packages/core/fast-iterable-differ/instructions.ts b/packages/core/fast-iterable-differ/instructions.ts
@@ -198,7 +198,7 @@ function patchKeyedChildren(container: Container, a: ItemNode[], b: ItemNode[],
             pos = i + bStart;
             bNode = b[pos];
             nextPos = pos + 1;
-            insertOrAppend(container, bNode, nextPos < bLength ? b[nextPos] : null);
+            insertOrAppend(container, bNode, nextPos < bLength ? b[nextPos] : null, undefined, sources[i]);
           } else {
             j--;
           }