From dd4848f6022d136f00f80a60ee5eee6df8938711 Mon Sep 17 00:00:00 2001
From: "Lucas Perais (lpe)" <lpe@odoo.com>
Date: Tue, 26 Oct 2021 09:48:31 +0200
Subject: [PATCH] [FIX] qweb: t-key in t-foreach is mandatory, throws otherwise

---
 src/qweb/compiler.ts                          |  5 --
 src/qweb/parser.ts                            |  5 ++
 tests/qweb/__snapshots__/parser.test.ts.snap  |  3 +
 .../qweb/__snapshots__/t_foreach.test.ts.snap | 47 ++++++------
 tests/qweb/__snapshots__/t_key.test.ts.snap   | 40 ++++++++++
 tests/qweb/parser.test.ts                     | 69 +++++++++--------
 tests/qweb/t_foreach.test.ts                  | 75 ++++++++-----------
 tests/qweb/t_key.test.ts                      | 33 ++++----
 8 files changed, 156 insertions(+), 121 deletions(-)
 create mode 100644 tests/qweb/__snapshots__/parser.test.ts.snap
 create mode 100644 tests/qweb/__snapshots__/t_key.test.ts.snap

diff --git a/src/qweb/compiler.ts b/src/qweb/compiler.ts
index 9dc92983..ecda985b 100644
--- a/src/qweb/compiler.ts
+++ b/src/qweb/compiler.ts
@@ -742,11 +742,6 @@ export class QWebCompiler {
 
     const subCtx: Context = createContext(ctx, { block, index: loopVar });
     this.compileAST(ast.body, subCtx);
-    if (!ast.key) {
-      console.warn(
-        `"Directive t-foreach should always be used with a t-key! (in template: '${this.templateName}')"`
-      );
-    }
     if (ast.memo) {
       this.addLine(
         `nextCache[key${this.target.loopLevel}] = assign(${c}[${loopVar}], {memo: memo${id!}});`
diff --git a/src/qweb/parser.ts b/src/qweb/parser.ts
index 018a12a8..18bcceca 100644
--- a/src/qweb/parser.ts
+++ b/src/qweb/parser.ts
@@ -415,6 +415,11 @@ function parseTForEach(node: Element, ctx: ParsingContext): AST | null {
   const elem = node.getAttribute("t-as") || "";
   node.removeAttribute("t-as");
   const key = node.getAttribute("t-key");
+  if (!key) {
+    throw new Error(
+      `"Directive t-foreach should always be used with a t-key!" (expression: t-foreach="${collection}" t-as="${elem}")`
+    );
+  }
   node.removeAttribute("t-key");
   const memo = node.getAttribute("t-memo") || "";
   node.removeAttribute("t-memo");
diff --git a/tests/qweb/__snapshots__/parser.test.ts.snap b/tests/qweb/__snapshots__/parser.test.ts.snap
new file mode 100644
index 00000000..e28b80d3
--- /dev/null
+++ b/tests/qweb/__snapshots__/parser.test.ts.snap
@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`qweb parser simple t-foreach expression, t-key mandatory 1`] = `"\\"Directive t-foreach should always be used with a t-key!\\" (expression: t-foreach=\\"list\\" t-as=\\"item\\")"`;
diff --git a/tests/qweb/__snapshots__/t_foreach.test.ts.snap b/tests/qweb/__snapshots__/t_foreach.test.ts.snap
index e279a38c..ea1363c9 100644
--- a/tests/qweb/__snapshots__/t_foreach.test.ts.snap
+++ b/tests/qweb/__snapshots__/t_foreach.test.ts.snap
@@ -440,6 +440,29 @@ exports[`t-foreach t-foreach with t-if inside 1`] = `
 }"
 `;
 
+exports[`t-foreach t-key on t-foreach 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers;
+  
+  let block1 = createBlock(\`<div><block-child-0/></div>\`);
+  let block3 = createBlock(\`<span/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block2, v_block2, l_block2, c_block2] = prepareList(ctx['things']);
+    for (let i1 = 0; i1 < l_block2; i1++) {
+      ctx[\`thing\`] = v_block2[i1];
+      let key1 = ctx['thing'];
+      c_block2[i1] = withKey(block3(), key1);
+    }
+    let b2 = list(c_block2);
+    return block1([], [b2]);
+  }
+}"
+`;
+
 exports[`t-foreach throws error if invalid loop expression 1`] = `
 "function anonymous(bdom, helpers
 ) {
@@ -464,30 +487,6 @@ exports[`t-foreach throws error if invalid loop expression 1`] = `
 }"
 `;
 
-exports[`t-foreach warn if no key in some case 1`] = `
-"function anonymous(bdom, helpers
-) {
-  let { text, createBlock, list, multi, html, toggler, component } = bdom;
-  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers;
-  
-  let block1 = createBlock(\`<div><block-child-0/></div>\`);
-  let block3 = createBlock(\`<span><block-text-0/></span>\`);
-  
-  return function template(ctx, node, key = \\"\\") {
-    ctx = Object.create(ctx);
-    const [k_block2, v_block2, l_block2, c_block2] = prepareList([1,2]);
-    for (let i1 = 0; i1 < l_block2; i1++) {
-      ctx[\`item\`] = v_block2[i1];
-      let key1 = i1;
-      let d1 = ctx['item'];
-      c_block2[i1] = withKey(block3([d1]), key1);
-    }
-    let b2 = list(c_block2);
-    return block1([], [b2]);
-  }
-}"
-`;
-
 exports[`t-foreach with t-memo 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/qweb/__snapshots__/t_key.test.ts.snap b/tests/qweb/__snapshots__/t_key.test.ts.snap
new file mode 100644
index 00000000..2822d8f5
--- /dev/null
+++ b/tests/qweb/__snapshots__/t_key.test.ts.snap
@@ -0,0 +1,40 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`t-key can use t-key directive on a node 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers;
+  
+  let block1 = createBlock(\`<div><block-text-0/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let d1 = ctx['beer'].name;
+    return block1([d1]);
+  }
+}"
+`;
+
+exports[`t-key t-key directive in a list 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue } = helpers;
+  
+  let block1 = createBlock(\`<ul><block-child-0/></ul>\`);
+  let block3 = createBlock(\`<li><block-text-0/></li>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block2, v_block2, l_block2, c_block2] = prepareList(ctx['beers']);
+    for (let i1 = 0; i1 < l_block2; i1++) {
+      ctx[\`beer\`] = v_block2[i1];
+      let key1 = ctx['beer'].id;
+      let d1 = ctx['beer'].name;
+      c_block2[i1] = withKey(block3([d1]), key1);
+    }
+    let b2 = list(c_block2);
+    return block1([], [b2]);
+  }
+}"
+`;
diff --git a/tests/qweb/parser.test.ts b/tests/qweb/parser.test.ts
index c63d881d..cec9b4d8 100644
--- a/tests/qweb/parser.test.ts
+++ b/tests/qweb/parser.test.ts
@@ -464,48 +464,53 @@ describe("qweb parser", () => {
   // t-foreach
   // ---------------------------------------------------------------------------
 
+  test("simple t-foreach expression, t-key mandatory", async () => {
+    expect(() => parse(`<t t-foreach="list" t-as="item"><t t-esc="item"/></t>`)).toThrowErrorMatchingSnapshot();
+
+  })
+
   test("simple t-foreach expression", async () => {
-    expect(parse(`<t t-foreach="list" t-as="item"><t t-esc="item"/></t>`)).toEqual({
+    expect(parse(`<t t-foreach="list" t-as="item" t-key="item_index"><t t-esc="item"/></t>`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: true,
       isOnlyChild: false,
       body: { type: ASTType.TEsc, expr: "item", defaultValue: "" },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
   });
 
   test("t-foreach expression with t-esc", async () => {
-    expect(parse(`<t t-foreach="list" t-as="item" t-esc="item"/>`)).toEqual({
+    expect(parse(`<t t-foreach="list" t-as="item" t-esc="item" t-key="item_index"/>`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: true,
       isOnlyChild: false,
       body: { type: ASTType.TEsc, expr: "item", defaultValue: "" },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
   });
 
   test("t-foreach on a div expression with t-esc", async () => {
-    expect(parse(`<div t-foreach="list" t-as="item" t-esc="item"/>`)).toEqual({
+    expect(parse(`<div t-foreach="list" t-as="item" t-esc="item" t-key="item_index"/>`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
       hasNoComponent: true,
       isOnlyChild: false,
-      key: null,
+      key: "item_index",
       body: {
         type: ASTType.DomNode,
         tag: "div",
@@ -516,7 +521,7 @@ describe("qweb parser", () => {
       },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
@@ -540,11 +545,11 @@ describe("qweb parser", () => {
   });
 
   test("t-foreach expression on a span", async () => {
-    expect(parse(`<span t-foreach="list" t-as="item"><t t-esc="item"/></span>`)).toEqual({
+    expect(parse(`<span t-foreach="list" t-as="item" t-key="item_index"><t t-esc="item"/></span>`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: true,
       isOnlyChild: false,
       body: {
@@ -557,7 +562,7 @@ describe("qweb parser", () => {
       },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
@@ -565,12 +570,12 @@ describe("qweb parser", () => {
 
   test("t-foreach expression on a span", async () => {
     expect(
-      parse(`<span t-foreach="list" t-if="condition" t-as="item"><t t-esc="item"/></span>`)
+      parse(`<span t-foreach="list" t-if="condition" t-as="item" t-key="item_index"><t t-esc="item"/></span>`)
     ).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: true,
       isOnlyChild: false,
       body: {
@@ -589,7 +594,7 @@ describe("qweb parser", () => {
       },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
@@ -598,13 +603,13 @@ describe("qweb parser", () => {
   test("more complex t-foreach expression on an option", async () => {
     expect(
       parse(
-        `<option t-foreach="categories" t-as="category" t-att-value="category.id" t-esc="category.name" t-att-selected="category.id==options.active_category_id"/>`
+        `<option t-foreach="categories" t-as="category" t-att-value="category.id" t-esc="category.name" t-att-selected="category.id==options.active_category_id" t-key="category_index"/>`
       )
     ).toEqual({
       type: ASTType.TForEach,
       collection: "categories",
       elem: "category",
-      key: null,
+      key: "category_index",
       hasNoComponent: true,
       isOnlyChild: false,
       body: {
@@ -620,14 +625,14 @@ describe("qweb parser", () => {
       },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
   });
 
   test("t-foreach in a div", async () => {
-    expect(parse(`<div><t t-foreach="list" t-as="item"><t t-esc="item"/></t></div>`)).toEqual({
+    expect(parse(`<div><t t-foreach="list" t-as="item" t-key="item_index"><t t-esc="item"/></t></div>`)).toEqual({
       type: ASTType.DomNode,
       attrs: {},
       on: {},
@@ -638,13 +643,13 @@ describe("qweb parser", () => {
           type: ASTType.TForEach,
           collection: "list",
           elem: "item",
-          key: null,
+          key: "item_index",
           hasNoComponent: true,
           isOnlyChild: true,
           body: { type: ASTType.TEsc, expr: "item", defaultValue: "" },
           memo: "",
           hasNoFirst: true,
-          hasNoIndex: true,
+          hasNoIndex: false,
           hasNoLast: true,
           hasNoValue: true,
         },
@@ -687,11 +692,11 @@ describe("qweb parser", () => {
   });
 
   test("t-foreach expression with a component inside", async () => {
-    expect(parse(`<Comp t-foreach="list" t-as="item" />`)).toEqual({
+    expect(parse(`<Comp t-foreach="list" t-as="item" t-key="item_index" />`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: false,
       isOnlyChild: false,
       body: {
@@ -705,18 +710,18 @@ describe("qweb parser", () => {
       },
       memo: "",
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
     });
   });
 
   test("t-foreach expression with a t-call inside", async () => {
-    expect(parse(`<t t-foreach="list" t-as="item"><t t-call="blap"/></t>`)).toEqual({
+    expect(parse(`<t t-foreach="list" t-as="item" t-key="item_index"><t t-call="blap"/></t>`)).toEqual({
       type: ASTType.TForEach,
       collection: "list",
       elem: "item",
-      key: null,
+      key: "item_index",
       hasNoComponent: false,
       isOnlyChild: false,
       body: {
@@ -733,18 +738,18 @@ describe("qweb parser", () => {
   });
 
   test("t-foreach expression with t-memo", async () => {
-    expect(parse(`<t t-foreach="list" t-as="item" t-memo="[row.x]"><t t-esc="item"/></t>`)).toEqual(
+    expect(parse(`<t t-foreach="list" t-as="item" t-memo="[row.x]" t-key="item_index"><t t-esc="item"/></t>`)).toEqual(
       {
         type: ASTType.TForEach,
         collection: "list",
         elem: "item",
-        key: null,
+        key: "item_index",
         hasNoComponent: true,
         isOnlyChild: false,
         body: { type: ASTType.TEsc, expr: "item", defaultValue: "" },
         memo: "[row.x]",
         hasNoFirst: true,
-        hasNoIndex: true,
+        hasNoIndex: false,
         hasNoLast: true,
         hasNoValue: true,
       }
@@ -1180,7 +1185,7 @@ describe("qweb parser", () => {
       },
     });
 
-    expect(parse(`<div t-foreach="list" t-translation="off" t-as="item">word</div>`)).toEqual({
+    expect(parse(`<div t-foreach="list" t-translation="off" t-as="item" t-key="item_index">word</div>`)).toEqual({
       body: {
         content: {
           attrs: {},
@@ -1201,11 +1206,11 @@ describe("qweb parser", () => {
       elem: "item",
       hasNoComponent: true,
       hasNoFirst: true,
-      hasNoIndex: true,
+      hasNoIndex: false,
       hasNoLast: true,
       hasNoValue: true,
       isOnlyChild: false,
-      key: null,
+      key: "item_index",
       memo: "",
       type: 9,
     });
diff --git a/tests/qweb/t_foreach.test.ts b/tests/qweb/t_foreach.test.ts
index 11a0668d..e77acab9 100644
--- a/tests/qweb/t_foreach.test.ts
+++ b/tests/qweb/t_foreach.test.ts
@@ -1,4 +1,11 @@
-import { renderToString, snapshotEverything, TestContext } from "../helpers";
+import {
+  renderToBdom,
+  renderToString,
+  snapshotEverything,
+  TestContext,
+  makeTestFixture,
+} from "../helpers";
+import { mount, patch } from "../../src/blockdom";
 
 snapshotEverything();
 
@@ -23,26 +30,29 @@ describe("t-foreach", () => {
     expect(renderToString(template)).toBe(expected);
   });
 
-  test.skip("t-key on t-foreach", async () => {
-    // qweb.addTemplate(
-    //   "test",
-    //   `
-    //     <div>
-    //       <t t-foreach="things" t-as="thing" t-key="thing">
-    //         <span/>
-    //       </t>
-    //     </div>`
-    // );
-    // let vnode = qweb.render("test", { things: [1, 2] });
-    // vnode = patch(document.createElement("div"), vnode);
-    // let elm = vnode.elm as HTMLElement;
-    // expect(elm.outerHTML).toBe("<div><span></span><span></span></div>");
-    // const first = elm.querySelectorAll("span")[0];
-    // const second = elm.querySelectorAll("span")[1];
-    // patch(vnode, qweb.render("test", { things: [2, 1] }));
-    // expect(elm.outerHTML).toBe("<div><span></span><span></span></div>");
-    // expect(first).toBe(elm.querySelectorAll("span")[1]);
-    // expect(second).toBe(elm.querySelectorAll("span")[0]);
+  test("t-key on t-foreach", async () => {
+    const template = `
+        <div>
+          <t t-foreach="things" t-as="thing" t-key="thing">
+            <span/>
+          </t>
+        </div>`;
+
+    const fixture = makeTestFixture();
+
+    const vnode1 = renderToBdom(template, { things: [1, 2] });
+    mount(vnode1, fixture);
+    let elm = fixture;
+    expect(elm.innerHTML).toBe("<div><span></span><span></span></div>");
+    const first = elm.querySelectorAll("span")[0];
+    const second = elm.querySelectorAll("span")[1];
+
+    const vnode2 = renderToBdom(template, { things: [2, 1] });
+    patch(vnode1, vnode2);
+
+    expect(elm.innerHTML).toBe("<div><span></span><span></span></div>");
+    expect(first).toBe(elm.querySelectorAll("span")[1]);
+    expect(second).toBe(elm.querySelectorAll("span")[0]);
   });
 
   test("simple iteration (in a node)", () => {
@@ -186,29 +196,6 @@ describe("t-foreach", () => {
     expect(() => renderToString(test)).toThrow("Invalid loop expression");
   });
 
-  test("warn if no key in some case", () => {
-    const consoleWarn = console.warn;
-    console.warn = jest.fn();
-
-    const template = `
-        <div>
-          <t t-foreach="[1, 2]" t-as="item">
-            <span><t t-esc="item"/></span>
-          </t>
-        </div>`;
-    renderToString(template);
-    expect(console.warn).toHaveBeenCalledTimes(1);
-    expect(console.warn).toHaveBeenCalledWith(
-      `\"Directive t-foreach should always be used with a t-key! (in template: '
-        <div>
-          <t t-foreach=\"[1, 2]\" t-as=\"item\">
-            <span><t t-esc=\"item\"/></span>
-          </t>
-        </div>')\"`
-    );
-    console.warn = consoleWarn;
-  });
-
   test("t-foreach with t-if inside", () => {
     const template = `
         <div>
diff --git a/tests/qweb/t_key.test.ts b/tests/qweb/t_key.test.ts
index f6cc5515..08ec93b5 100644
--- a/tests/qweb/t_key.test.ts
+++ b/tests/qweb/t_key.test.ts
@@ -1,22 +1,23 @@
-describe.skip("t-key", () => {
+import { renderToString, snapshotEverything } from "../helpers";
+
+snapshotEverything();
+
+describe("t-key", () => {
   test("can use t-key directive on a node", () => {
-    // qweb.addTemplate("test", `<div t-key="beer.id"><t t-esc="beer.name"/></div>`);
-    // expect(renderToString(qweb, "test", { beer: { id: 12, name: "Chimay Rouge" } })).toBe(
-    //   "<div>Chimay Rouge</div>"
-    // );
+    const template = `<div t-key="beer.id"><t t-esc="beer.name"/></div>`;
+    expect(renderToString(template, { beer: { id: 12, name: "Chimay Rouge" } })).toBe(
+      "<div>Chimay Rouge</div>"
+    );
   });
 
   test("t-key directive in a list", () => {
-    // qweb.addTemplate(
-    //   "test",
-    //   `<ul>
-    //     <li t-foreach="beers" t-as="beer" t-key="beer.id"><t t-esc="beer.name"/></li>
-    //    </ul>`
-    // );
-    // expect(
-    //   renderToString(qweb, "test", {
-    //     beers: [{ id: 12, name: "Chimay Rouge" }],
-    //   })
-    // ).toMatchSnapshot();
+    const template = `<ul>
+        <li t-foreach="beers" t-as="beer" t-key="beer.id"><t t-esc="beer.name"/></li>
+       </ul>`;
+    expect(
+      renderToString(template, {
+        beers: [{ id: 12, name: "Chimay Rouge" }],
+      })
+    ).toBe("<ul><li>Chimay Rouge</li></ul>");
   });
 });