From a5b93ebb257b2d2bb5ce16d39dab6d493ccd6feb Mon Sep 17 00:00:00 2001
From: Michael Rawlings <mirawlings@ebay.com>
Date: Mon, 25 Jun 2018 21:35:39 -0700
Subject: [PATCH] fix for loop autokeys (#1067)

---
 .gitignore                                    |  1 +
 package.json                                  |  2 +-
 .../TransformHelper/assignComponentId.js      | 77 +++++++++++++++++++
 .../fixtures-html/at-tags-dynamic/expected.js |  4 +
 .../compiler/fixtures-html/simple/expected.js |  4 +
 .../compiler/fixtures-vdom/simple/expected.js |  8 +-
 .../index.marko                               | 21 +++++
 .../test.js                                   | 62 +++++++++++++++
 .../fixtures-html/auto-key-els/expected.js    | 10 ++-
 .../fixtures-html/macro-widget/expected.js    |  4 +
 .../preserve-no-update-for-loop/tests.js      |  5 +-
 11 files changed, 189 insertions(+), 9 deletions(-)
 create mode 100644 test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/index.marko
 create mode 100644 test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/test.js

diff --git a/.gitignore b/.gitignore
index af85823eb..b74bd67c0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -19,3 +19,4 @@ coverage
 /dist/
 /test-dist/
 /test-generated/
+.vs/
\ No newline at end of file
diff --git a/package.json b/package.json
index bf11a52d1..f58c21a4f 100644
--- a/package.json
+++ b/package.json
@@ -10,7 +10,7 @@
         "precommit": "lint-staged",
         "test": "npm run lint -s && npm run mocha -s",
         "test-ci": "npm run check-format && npm run test-generate-coverage",
-        "mocha": "mocha --timeout 10000 --max-old-space-size=768 ./test/*/*.test.js || exit 1",
+        "mocha": "mocha --timeout 10000 --max-old-space-size=768 ./test/*/*.test.js",
         "test-coverage": "npm run test-generate-coverage && nyc report --reporter=html && opn ./coverage/index.html",
         "test-generate-coverage": "nyc -asc npm test",
         "lint": "eslint .",
diff --git a/src/components/taglib/TransformHelper/assignComponentId.js b/src/components/taglib/TransformHelper/assignComponentId.js
index 7a61de874..68885a39f 100644
--- a/src/components/taglib/TransformHelper/assignComponentId.js
+++ b/src/components/taglib/TransformHelper/assignComponentId.js
@@ -106,6 +106,7 @@ module.exports = function assignComponentId(isRepeated) {
         }
     } else {
         // Case 3 - We need to add a unique auto key
+        let parentForKey = getParentForKeyVar(el, this);
         let uniqueKey = this.nextUniqueId();
 
         nestedIdExpression = isRepeated
@@ -114,6 +115,19 @@ module.exports = function assignComponentId(isRepeated) {
 
         idExpression = builder.literal(uniqueKey.toString());
 
+        if (parentForKey) {
+            idExpression = builder.binaryExpression(
+                idExpression,
+                "+",
+                parentForKey
+            );
+            nestedIdExpression = builder.binaryExpression(
+                nestedIdExpression,
+                "+",
+                parentForKey
+            );
+        }
+
         if (isCustomTag) {
             this.getComponentArgs().setKey(nestedIdExpression);
         } else {
@@ -168,3 +182,66 @@ module.exports = function assignComponentId(isRepeated) {
 
     return this.componentIdInfo;
 };
+
+const getParentForKeyVar = (el, transformHelper) => {
+    const context = transformHelper.context;
+    const builder = context.builder;
+    const parentFor = getParentFor(el);
+
+    if (!parentFor) return null;
+    if (parentFor.keyVar) return parentFor.keyVar;
+
+    const keyExpression =
+        getChildKey(parentFor) || createIndexKey(parentFor, transformHelper);
+    const bracketedKeyExpression = builder.binaryExpression(
+        builder.literal("["),
+        "+",
+        builder.binaryExpression(keyExpression, "+", builder.literal("]"))
+    );
+    const varName = "keyscope__" + transformHelper.nextUniqueId();
+    const varDeclaration = builder.var(varName, bracketedKeyExpression);
+
+    parentFor.prependChild(varDeclaration);
+
+    return (parentFor.keyVar = builder.identifier(varName));
+};
+
+const createIndexKey = (forNode, transformHelper) => {
+    const context = transformHelper.context;
+    const builder = context.builder;
+    const varName = "for__" + transformHelper.nextUniqueId();
+    const intialize = builder.parseStatement(`var ${varName} = 0;`);
+    const parentForKey = getParentForKeyVar(forNode, transformHelper);
+
+    forNode.insertSiblingBefore(intialize);
+
+    let keyExpression = builder.parseExpression(`${varName}++`);
+
+    if (parentForKey) {
+        keyExpression = builder.binaryExpression(
+            keyExpression,
+            "+",
+            parentForKey
+        );
+    }
+
+    return keyExpression;
+};
+
+const getParentFor = el => {
+    let current = el;
+    while ((current = current.parentNode)) {
+        if (current.tagName === "for") return current;
+    }
+};
+
+const getChildKey = el => {
+    let current = el.firstChild;
+    while (current) {
+        if (current.key) return current.key;
+        if (current.hasAttribute && current.hasAttribute("key")) {
+            return current.getAttributeValue("key");
+        }
+        current = current.nextSibling;
+    }
+};
diff --git a/test/compiler/fixtures-html/at-tags-dynamic/expected.js b/test/compiler/fixtures-html/at-tags-dynamic/expected.js
index 47ef654da..c23d67c03 100644
--- a/test/compiler/fixtures-html/at-tags-dynamic/expected.js
+++ b/test/compiler/fixtures-html/at-tags-dynamic/expected.js
@@ -21,7 +21,11 @@ function render(input, out, __component, component, state) {
 
   hello_tag(marko_mergeNestedTagsHelper({
       renderBody: function renderBody(out, hello0) {
+        var for__1 = 0;
+
         marko_forEach(input.colors, function(color) {
+          var keyscope__2 = "[" + ((for__1++) + "]");
+
           hello_foo_nested_tag({
               renderBody: function renderBody(out) {
                 out.w("Foo!");
diff --git a/test/compiler/fixtures-html/simple/expected.js b/test/compiler/fixtures-html/simple/expected.js
index 750b201a3..3c28356cf 100644
--- a/test/compiler/fixtures-html/simple/expected.js
+++ b/test/compiler/fixtures-html/simple/expected.js
@@ -33,7 +33,11 @@ function render(input, out, __component, component, state) {
   if (input.colors.length) {
     out.w("<ul>");
 
+    var for__4 = 0;
+
     marko_forEach(input.colors, function(color) {
+      var keyscope__5 = "[" + ((for__4++) + "]");
+
       out.w("<li>" +
         marko_escapeXml(color) +
         "</li>");
diff --git a/test/compiler/fixtures-vdom/simple/expected.js b/test/compiler/fixtures-vdom/simple/expected.js
index b9825194a..aabc11cd3 100644
--- a/test/compiler/fixtures-vdom/simple/expected.js
+++ b/test/compiler/fixtures-vdom/simple/expected.js
@@ -17,7 +17,7 @@ var marko_template = module.exports = require("marko/src/vdom").t(),
         i: marko_const_nextId()
       })
       .t("No colors!"),
-    marko_node1 = marko_createElement("DIV", null, "5", null, 1, 0, {
+    marko_node1 = marko_createElement("DIV", null, "7", null, 1, 0, {
         i: marko_const_nextId()
       })
       .t("No colors!");
@@ -47,8 +47,12 @@ function render(input, out, __component, component, state) {
   if (input.colors.length) {
     out.be("UL", null, "3", component);
 
+    var for__4 = 0;
+
     marko_forEach(input.colors, function(color) {
-      out.e("LI", null, "4", component, 1)
+      var keyscope__5 = "[" + ((for__4++) + "]");
+
+      out.e("LI", null, "6" + keyscope__5, component, 1)
         .t(color);
     });
 
diff --git a/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/index.marko b/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/index.marko
new file mode 100644
index 000000000..7c1e9e471
--- /dev/null
+++ b/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/index.marko
@@ -0,0 +1,21 @@
+class {
+    onCreate() {
+        this.state = { items: [] };
+    }
+
+    addItem(id) {
+        this.state.items = this.state.items.concat(id);
+    }
+
+    removeItem(id) {
+        this.state.items = this.state.items.filter(item => item !== id);
+    }
+}
+
+<div>
+    <for(item in state.items)>
+        <div key=item>
+          <span/>
+        </div>
+    </for>
+</div>
\ No newline at end of file
diff --git a/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/test.js b/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/test.js
new file mode 100644
index 000000000..7703688f3
--- /dev/null
+++ b/test/components-browser/fixtures/widget-preserve-all-state-when-rendered-in-loop/test.js
@@ -0,0 +1,62 @@
+var expect = require("chai").expect;
+
+module.exports = function(helpers) {
+    var component = helpers.mount(require.resolve("./index"), {});
+    var rootEl = component.getEl();
+    var itemIds = ["child-a", "child-b", "child-c"];
+
+    itemIds.forEach(id => component.addItem(id));
+    component.update();
+
+    var curLookup = getKeyLookup();
+
+    component.removeItem(itemIds[1]);
+    component.update();
+
+    ensurePreservedKeys(curLookup, (curLookup = getKeyLookup()));
+
+    component.removeItem(itemIds[0]);
+    component.update();
+
+    ensurePreservedKeys(curLookup, (curLookup = getKeyLookup()));
+
+    function getKeyLookup(els) {
+        els = els || rootEl.children;
+        var lookup = {};
+
+        for (var i = 0; i < els.length; i++) {
+            var el = els[i];
+            var component = helpers.getComponentForEl(el);
+            var key = el.___markoVElement.___key;
+            lookup[key] = {
+                el: el,
+                component: component,
+                children: getKeyLookup(el.children)
+            };
+        }
+
+        return lookup;
+    }
+
+    function ensurePreservedKeys(lookupA, lookupB, path) {
+        path = path || "";
+        for (var key in lookupB) {
+            if (lookupA[key]) {
+                var fullPath = path + " " + key;
+                expect(
+                    lookupA[key].el === lookupB[key].el,
+                    'unpreserved element "' + fullPath + '"'
+                ).to.equal(true);
+                expect(
+                    lookupA[key].component === lookupB[key].component,
+                    'unpreserved component "' + fullPath + '"'
+                ).to.equal(true);
+                ensurePreservedKeys(
+                    lookupA[key].children,
+                    lookupB[key].children,
+                    fullPath
+                );
+            }
+        }
+    }
+};
diff --git a/test/components-compilation/fixtures-html/auto-key-els/expected.js b/test/components-compilation/fixtures-html/auto-key-els/expected.js
index ccdbccb94..74d305008 100644
--- a/test/components-compilation/fixtures-html/auto-key-els/expected.js
+++ b/test/components-compilation/fixtures-html/auto-key-els/expected.js
@@ -20,7 +20,11 @@ function render(input, out, __component, component, state) {
 
   out.w("<div><span>A</span><ul>");
 
+  var for__2 = 0;
+
   marko_forEach(colors, function(color) {
+    var keyscope__3 = "[" + ((for__2++) + "]");
+
     out.w("<li>" +
       marko_escapeXml(color) +
       "</li>");
@@ -28,13 +32,13 @@ function render(input, out, __component, component, state) {
 
   out.w("</ul>");
 
-  var __key3 = __component.___nextKey("preservedP");
+  var __key5 = __component.___nextKey("preservedP");
 
   out.w("<p>");
 
   _preserve_tag({
       bodyOnly: true,
-      key: __key3,
+      key: __key5,
       renderBody: function renderBody(out) {
         out.w(marko_escapeXml(Date.now()));
       }
@@ -44,7 +48,7 @@ function render(input, out, __component, component, state) {
 
   include_tag({
       _target: foo_template
-    }, out, __component, "5");
+    }, out, __component, "7");
 }
 
 marko_template._ = marko_renderer(render, {
diff --git a/test/components-compilation/fixtures-html/macro-widget/expected.js b/test/components-compilation/fixtures-html/macro-widget/expected.js
index 68ae8e47f..28cd2bfeb 100644
--- a/test/components-compilation/fixtures-html/macro-widget/expected.js
+++ b/test/components-compilation/fixtures-html/macro-widget/expected.js
@@ -28,11 +28,15 @@ function render(input, out, __component, component, state) {
 
   out.w("<div>");
 
+  var for__2 = 0;
+
   marko_forEach([
       "red",
       "green",
       "blue"
     ], function(color) {
+    var keyscope__3 = "[" + ((for__2++) + "]");
+
     macro_renderButton(color, out);
   });
 
diff --git a/test/components-pages/fixtures/preserve-no-update-for-loop/tests.js b/test/components-pages/fixtures/preserve-no-update-for-loop/tests.js
index 7ef08a908..ca6e2a0cd 100644
--- a/test/components-pages/fixtures/preserve-no-update-for-loop/tests.js
+++ b/test/components-pages/fixtures/preserve-no-update-for-loop/tests.js
@@ -2,7 +2,7 @@ var path = require("path");
 var expect = require("chai").expect;
 
 describe(path.basename(__dirname), function() {
-    it.fails("should update correctly", function() {
+    it("should update correctly", function() {
         var component = window.component;
         var $el = component.getEl("root");
 
@@ -17,6 +17,5 @@ describe(path.basename(__dirname), function() {
         expect($el.innerHTML).to.eql(
             '<li id="a"><span>a</span></li><li id="b"><span>b</span></li>'
         );
-    }).details =
-        "Issue #952";
+    });
 });