From 4e982f54607345c9ffb1fcf1c3d8544411894088 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Fri, 3 May 2019 09:49:25 +0200
Subject: [PATCH] [IMP] qweb: add warning if t-key is missing in some cases

closes #71
---
 src/qweb.ts                           | 17 +++++++++++++-
 tests/__snapshots__/qweb.test.ts.snap | 33 ++++++++++++++++++++++++++-
 tests/component.test.ts               |  4 ++--
 tests/qweb.test.ts                    | 23 +++++++++++++++++--
 4 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/src/qweb.ts b/src/qweb.ts
index 8a587b63..6783d396 100644
--- a/src/qweb.ts
+++ b/src/qweb.ts
@@ -962,7 +962,22 @@ const forEachDirective: Directive = {
     ctx.addLine(`context.${name} = _${keysID}[i];`);
     ctx.addLine(`context.${name}_value = _${valuesID}[i];`);
     const nodeCopy = <Element>node.cloneNode(true);
-    if (nodeCopy.tagName !== "t" && !nodeCopy.hasAttribute("t-key")) {
+    let shouldWarn =
+      nodeCopy.tagName !== "t" && !nodeCopy.hasAttribute("t-key");
+    if (!shouldWarn && node.tagName === "t") {
+      if (node.hasAttribute("t-widget") && !node.hasAttribute("t-key")) {
+        shouldWarn = true;
+      }
+      if (
+        !shouldWarn &&
+        node.children.length === 1 &&
+        node.children[0].tagName !== 't' &&
+        !node.children[0].hasAttribute("t-key")
+      ) {
+        shouldWarn = true;
+      }
+    }
+    if (shouldWarn) {
       console.warn(
         `Directive t-foreach should always be used with a t-key! (in template: '${
           ctx.templateName
diff --git a/tests/__snapshots__/qweb.test.ts.snap b/tests/__snapshots__/qweb.test.ts.snap
index 71ed8249..6cb5c869 100644
--- a/tests/__snapshots__/qweb.test.ts.snap
+++ b/tests/__snapshots__/qweb.test.ts.snap
@@ -534,6 +534,37 @@ exports[`foreach iterate, position 1`] = `
 }"
 `;
 
+exports[`foreach warn if no key in some case 1`] = `
+"function anonymous(context,extra
+) {
+    context = Object.create(context);
+    var h = this.utils.h;
+    var c1 = [], p1 = {key:1};
+    var vn1 = h('div', p1, c1);
+    var _2 = [1, 2];
+    if (!_2) { throw new Error('QWeb error: Invalid loop expression')}
+    if (typeof _2 === 'number') { _2 = Array.from(Array(_2).keys())}
+    var _3 = _2 instanceof Array ? _2 : Object.keys(_2);
+    var _4 = _2 instanceof Array ? _2 : Object.values(_2);
+    for (let i = 0; i < _3.length; i++) {
+        context.item_first = i === 0;
+        context.item_last = i === _3.length - 1;
+        context.item_parity = i % 2 === 0 ? 'even' : 'odd';
+        context.item_index = i;
+        context.item = _3[i];
+        context.item_value = _4[i];
+        var c5 = [], p5 = {key:5};
+        var vn5 = h('span', p5, c5);
+        c1.push(vn5);
+        var _6 = context['item'];
+        if (_6 || _6 === 0) {
+            c5.push({text: _6});
+        }
+    }
+    return vn1;
+}"
+`;
+
 exports[`loading templates can initialize qweb with a string 1`] = `
 "function anonymous(context,extra
 ) {
@@ -1367,7 +1398,7 @@ exports[`t-ref refs in a loop 1`] = `
         context.item_index = i;
         context.item = _3[i];
         context.item_value = _4[i];
-        var c5 = [], p5 = {key:5};
+        var c5 = [], p5 = {key:context['item']};
         var vn5 = h('div', p5, c5);
         c1.push(vn5);
         const ref6 = context['item']
diff --git a/tests/component.test.ts b/tests/component.test.ts
index ae9e29a4..1ca46d2a 100644
--- a/tests/component.test.ts
+++ b/tests/component.test.ts
@@ -819,7 +819,7 @@ describe("composition", () => {
     class ParentWidget extends Widget {
       inlineTemplate = `
         <div>
-          <t t-foreach="state.list" t-ref="'child'" t-widget="Widget"/>
+          <t t-foreach="state.list" t-as="elem" t-ref="'child'" t-key="elem" t-widget="Widget"/>
         </div>`;
       widgets = { Widget };
       state = { list: <any>[] };
@@ -1029,7 +1029,7 @@ describe("composition", () => {
       inlineTemplate = `
         <div>
           <t t-foreach="state.numbers" t-as="number">
-            <t t-widget="ChildWidget" t-props="{n: number}"/>
+            <t t-widget="ChildWidget" t-key="number" t-props="{n: number}"/>
           </t>
         </div>`;
       state = {
diff --git a/tests/qweb.test.ts b/tests/qweb.test.ts
index 8caec5ed..c3ab5088 100644
--- a/tests/qweb.test.ts
+++ b/tests/qweb.test.ts
@@ -726,10 +726,29 @@ describe("foreach", () => {
   test("throws error if invalid loop expression", () => {
     qweb.addTemplate(
       "test",
-      `<div><t t-foreach="abc" t-as="item"><span/></t></div>`
+      `<div><t t-foreach="abc" t-as="item"><span t-key="item_index"/></t></div>`
     );
     expect(() => qweb.render("test")).toThrow("Invalid loop expression");
   });
+
+  test("warn if no key in some case", () => {
+    const consoleWarn = console.warn;
+    console.warn = jest.fn();
+
+    qweb.addTemplate(
+      "test",
+      `
+      <div>
+        <t t-foreach="[1, 2]" t-as="item">
+          <span><t t-esc="item"/></span>
+        </t>
+    </div>`
+    );
+    renderToString(qweb, "test");
+    expect(console.warn).toHaveBeenCalledTimes(1);
+    console.warn = consoleWarn;
+  });
+
 });
 
 describe("misc", () => {
@@ -966,7 +985,7 @@ describe("t-ref", () => {
     qweb.addTemplate("test", `
       <div>
         <t t-foreach="items" t-as="item">
-          <div t-ref="item"><t t-esc="item"/></div>
+          <div t-ref="item" t-key="item"><t t-esc="item"/></div>
         </t>
       </div>`);
     let refs: any = {};