fix(compiler): Compiler errors for missing keys in iterator

packages/benchmark/src/components/benchmark-table-component/benchmark-table-component.html

@@ -2,7 +2,7 @@
     <table>
         <tbody>
             <template for:each={rows} for:item="row">
-                <tr is="benchmark-table-component-row"
+                <tr key={row.id} is="benchmark-table-component-row"
                     class={row.className}
                     row={row}
                     onselect={handleSelect}

packages/benchmark/src/components/benchmark-table/benchmark-table.html

@@ -2,7 +2,7 @@
     <table>
         <tbody>
             <template for:each={rows} for:item="row">
-                <tr class={row.className}>
+                <tr key={row.id} class={row.className}>
                     <td>{row.id}</td>
                     <td><a onclick={handleSelect}>{row.label}</a></td>
                     <td><a onclick={handleRemove}>Remove</a></td>

packages/lwc-engine/package.json

@@ -7,7 +7,7 @@
   "typings": "types/engine.d.ts",
   "scripts": {
     "build": "concurrently \"yarn build:es-and-cjs\" \"yarn build:umd:prod\" \"yarn build:umd:dev\"",
-    "test": "DIR=`pwd` && cd ../../ && jest $DIR",
+    "test": "DIR=`pwd` && cd ../../ && yarn test $DIR",
     "build:umd:dev": "rollup -c scripts/rollup.config.umd.dev.js",
     "build:umd:prod": "rollup -c scripts/rollup.config.umd.prod.js",
     "build:es-and-cjs": "rollup -c scripts/rollup.config.es-and-cjs.js"

packages/lwc-engine/src/framework/__tests__/api.spec.ts

@@ -295,14 +295,10 @@ describe('api', () => {
 
     describe('#k()', () => {
         it('should combine keys', () => {
-            let k1, k2, k3, k4, k5;
+            let k1, k2;
             function html($api) {
-                const o = {};
                 k1 = $api.k(123, 345);
                 k2 = $api.k(345, "678");
-                k3 = $api.k(678, o);
-                k4 = $api.k(678, o);
-                k5 = $api.k(678, {});
                 return [];
             }
             class Foo extends Element {
@@ -314,9 +310,21 @@ describe('api', () => {
             document.body.appendChild(elm);
             expect(k1).toEqual('123:345');
             expect(k2).toEqual('345:678');
-            expect(k3).toEqual(k4);
-            expect(k3 === k5).toEqual(false);
+        });
+        it('should throw when key is an object', () => {
+            function html($api) {
+                const k1 = $api.k(678, {});
+                return [];
+            }
+            class Foo extends Element {
+                render() {
+                    return html;
+                }
+            }
+            const elm = createElement('x-foo', { is: Foo });
+            expect(() => {
+                document.body.appendChild(elm);
+            }).toThrow('Invalid key value "[object Object]" in [object:vm Foo (7)]. Key must be a string or number.');
         });
     });
-
 });

packages/lwc-engine/src/framework/api.ts

@@ -1,5 +1,5 @@
 import assert from "./assert";
-import { freeze, isArray, isUndefined, isNull, isFunction, isObject, isString, ArrayPush, assign, create, forEach, StringIndexOf, StringSlice, StringCharCodeAt } from "./language";
+import { freeze, isArray, isUndefined, isNull, isFunction, isObject, isString, ArrayPush, assign, create, forEach, StringSlice, StringCharCodeAt, isNumber } from "./language";
 import { vmBeingRendered, invokeComponentCallback } from "./invoker";
 import { EmptyArray, SPACE_CHAR } from "./utils";
 import { renderVM, createVM, appendVM, removeVM, VM } from "./vm";
@@ -8,7 +8,6 @@ import { ComponentConstructor, markComponentAsDirty, isValidEvent } from "./comp
 
 import { VNode, VNodeData, VNodes, VElement, VComment, VText, Hooks } from "../3rdparty/snabbdom/types";
 import { getCustomElementVM } from "./html-element";
-import { unwrap } from "./reactive";
 import { pierce } from "./piercing";
 
 export interface RenderAPI {
@@ -256,6 +255,11 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number,
     let next = iterator.next();
     let j = 0;
     let { value, done: last } = next;
+    let keyMap;
+    if (process.env.NODE_ENV !== 'production') {
+        keyMap = create(null);
+    }
+
     while (last === false) {
         // implementing a look-back-approach because we need to know if the element is the last
         next = iterator.next();
@@ -272,9 +276,17 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number,
         if (process.env.NODE_ENV !== 'production') {
             const vnodes = isArray(vnode) ? vnode : [vnode];
             forEach.call(vnodes, (childVnode: VNode | null) => {
-                if (!isNull(childVnode) && isObject(childVnode) && !isUndefined(childVnode.sel) && StringIndexOf.call(childVnode.sel, '-') > 0 && isUndefined(childVnode.key)) {
-                    // TODO - it'd be nice to log the owner component rather than the iteration children
-                    assert.logWarning(`Missing "key" attribute in iteration with child "<${childVnode.sel}>", index ${i}. Instead set a unique "key" attribute value on all iteration children so internal state can be preserved during rehydration.`);
+                if (!isNull(childVnode) && isObject(childVnode) && !isUndefined(childVnode.sel)) {
+                    const { key } = childVnode;
+                    if (isString(key) || isNumber(key)) {
+                        if (keyMap[key] === 1) {
+                            assert.logWarning(`Invalid "key" attribute in iteration with child "<${childVnode.sel}>". Key with value "${childVnode.key}" appears more than once in iteration. Key values must be unique numbers or strings.`);
+                        }
+                        keyMap[key] = 1;
+                    } else {
+                        // TODO - it'd be nice to log the owner component rather than the iteration children
+                        assert.logWarning(`Missing "key" attribute in iteration with child "<${childVnode.sel}>", index ${i}. Instead set a unique "key" attribute value on all iteration children so internal state can be preserved during rehydration.`);
+                    }
                 }
             });
         }
@@ -356,10 +368,7 @@ export function b(fn: EventListener): EventListener {
     };
 }
 
-const objToKeyMap: WeakMap<any, number> = new WeakMap();
-let globalKey: number = 0;
-
-// [k]ind function
+// [k]ey function
 export function k(compilerKey: number, obj: any): number | string | void {
     switch (typeof obj) {
         case 'number':
@@ -368,17 +377,8 @@ export function k(compilerKey: number, obj: any): number | string | void {
         case 'string':
             return compilerKey + ':' + obj;
         case 'object':
-            if (isNull(obj)) {
-                return;
-            }
-            // Slow path. We get here when element is inside iterator
-            // but no key is specified.
-            const unwrapped = unwrap(obj);
-            let objKey = objToKeyMap.get(unwrapped);
-            if (isUndefined(objKey)) {
-                objKey = globalKey++;
-                objToKeyMap.set(unwrapped, objKey);
+            if (process.env.NODE_ENV !== 'production') {
+                assert.fail(`Invalid key value "${obj}" in ${vmBeingRendered}. Key must be a string or number.`);
             }
-            return compilerKey + ':' + objKey;
     }
 }

packages/lwc-integration/src/components/natives/test-array-push-concat-unshift/array-push-concat-unshift.html

@@ -2,25 +2,25 @@
     <div>
         <span>Unshift</span>
         <ul id="unshift-list" onclick={handleClick}>
-            <li for:each={items} for:item="item">
+            <li key={item.title} for:each={items} for:item="item">
                 {item.title}
             </li>
         </ul>
     </div>
     <div>
         <span>Push</span>
         <ul id="push-list" onclick={handlePushClick}>
-            <li for:each={pushItems} for:item="item">
+            <li key={item.title} for:each={pushItems} for:item="item">
                 {item.title}
             </li>
         </ul>
     </div>
     <div>
         <span>Concat</span>
         <ul id="concat-list" onclick={handleConcatClick}>
-            <li for:each={concatItems} for:item="item">
+            <li key={item.title} for:each={concatItems} for:item="item">
                 {item.title}
             </li>
         </ul>
     </div>
-</template>
+</template>

packages/lwc-integration/src/components/natives/test-function-arguments/function-arguments.html

@@ -2,7 +2,7 @@
     <div class="message-assert">{message}</div>
     <ul class="iterate-list">
         <template iterator:x={items}>
-            <li key={x.index}>{x.value}</li>
+            <li key={x.value}>{x.value}</li>
         </template>
     </ul>
-</template>
+</template>

packages/lwc-integration/src/components/natives/test-plain-array/plain-array.html

@@ -2,15 +2,15 @@
     <div>
         <span>Plain push</span>
         <ul id="push-list-plain">
-            <li for:each={plainPush} for:item="item">
+            <li key={item} for:each={plainPush} for:item="item">
                 {item}
             </li>
         </ul>
     </div>
     <div>
         <span>Plain push with proxy item</span>
         <ul id="push-list">
-            <li for:each={plainPushWithProxy} for:item="item">
+            <li key={item.title} for:each={plainPushWithProxy} for:item="item">
                 {item.title}
             </li>
         </ul>
@@ -19,17 +19,17 @@
     <div>
         <span>Plain concat</span>
         <ul id="concat-list-plain">
-            <li for:each={plainConcat} for:item="item">
+            <li key={item} for:each={plainConcat} for:item="item">
                 {item}
             </li>
         </ul>
     </div>
     <div>
         <span>Plain concat with proxy item</span>
         <ul id="concat-list-proxy">
-            <li for:each={plainConcatWithProxy} for:item="item">
+            <li key={item.title} for:each={plainConcatWithProxy} for:item="item">
                 {item.title}
             </li>
         </ul>
     </div>
-</template>
+</template>

packages/lwc-integration/src/components/reactivity/test-reactivity-object-freeze/reactivity-object-freeze.html

@@ -2,7 +2,7 @@
     <h2> Sample to show error when we use objects that are frozen. Click on button to see error in console.</h2>
     <ul>
         <template for:each={state.todos} for:item="todo">
-            <li>{todo.text}</li>
+            <li key={todo.text}>{todo.text}</li>
         </template>
     </ul>
-   </template>
+   </template>

packages/lwc-integration/src/components/rendering/test-rendering-array/rendering-array/rendering-array.html

@@ -1,7 +1,7 @@
 <template>
     <test-case issue-title="[proxy-compat] Error: Setting property Symbol(Symbol.iterator) during the rendering" issue-id="702">
         <template for:each={state.items} for:item="item">
-            <compat-item item={item}></compat-item>
+            <compat-item key={item.label} item={item}></compat-item>
         </template>
     </test-case>
-</template>
+</template>

packages/lwc-integration/src/components/rendering/test-simple-list-container/simple-list/simple-list.html

@@ -2,7 +2,7 @@
     <div>
         <ul>
             <li class="first">header</li>
-            <li class="number" for:each={data} for:item="item">
+            <li key={item.x} class="number" for:index="index" for:each={data} for:item="item">
                 Value of X = {item.x}
             </li>
              <li class="last">footer</li>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/children/actual.html

@@ -10,13 +10,13 @@
         Other Child
         <template if:true={isTrue}>
             <template for:each={items} for:item="item">
-                <p>X1</p><p>X2</p>
+                <p key={item.id}>X1</p><p key={item.id}>X2</p>
             </template>
         </template>
     </section>
     <section class="s3">
         <p>Last child</p>
-        <div for:each={items} for:item="item"></div>
+        <div for:each={items} for:item="item" key={item.id}></div>
     </section>
     <section class="s4">
         <p>Other child1</p>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/children/expected.js

@@ -48,7 +48,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                               api_element(
                                 'p',
                                 {
-                                    key: api_key(3, item)
+                                    key: api_key(3, item.id)
                                 },
                                 [
                                     api_text('X1')
@@ -57,7 +57,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                             api_element(
                                 'p',
                                 {
-                                    key: api_key(4, item)
+                                    key: api_key(4, item.id)
                                 },
                                 [
                                     api_text('X2')
@@ -90,7 +90,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                     return api_element(
                         'div',
                         {
-                            key: api_key(7, item)
+                            key: api_key(7, item.id)
                         },
                         []
                     );

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/error-missing-key/actual.html

@@ -0,0 +1,5 @@
+<template>
+    <template for:each={items} for:item="item">
+        <p>1{item}</p>
+    </template>
+</template>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/error-missing-key/metadata.json

@@ -0,0 +1,11 @@
+{
+  "warnings": [{
+    "level": "error",
+    "message": "Missing key for element <p> inside of iterator. Elements within iterators must have a unique, computed key value.",
+    "start": 67,
+    "length": 14
+  }],
+  "metadata": {
+    "templateUsedIds": ["items"]
+  }
+}

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-index/actual.html

@@ -1,6 +1,6 @@
 <template>
     <ul>
-        <li for:each={items} for:item="item" for:index="index">
+        <li key={item.id} for:each={items} for:item="item" for:index="index">
             {index} - {item}
         </li>
     </ul>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-index/expected.js

@@ -17,7 +17,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                 return api_element(
                     'li',
                     {
-                        key: api_key(1, item)
+                        key: api_key(1, item.id)
                     },
                     [
                         api_dynamic(index),

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-item/actual.html

@@ -1,6 +1,6 @@
 <template>
     <section>
-        <div class="my-list" for:each={items} for:item="item">
+        <div key={item.id} class="my-list" for:each={items} for:item="item">
             <p>{item}</p>
         </div>
     </section>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-item/expected.js

@@ -14,7 +14,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                         classMap: {
                             'my-list': true
                         },
-                        key: api_key(2, item)
+                        key: api_key(2, item.id)
                     },
                     [
                         api_element(

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-outside-scope-reference/actual.html

@@ -1,6 +1,6 @@
 <template>
     <section>
-        <div class="my-list" for:each={items} for:item="item">
+        <div key={item.id} class="my-list" for:each={items} for:item="item">
             <p>{item}</p>
             <p>{item2}</p>
         </div>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-outside-scope-reference/expected.js

@@ -14,7 +14,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                         classMap: {
                             'my-list': true
                         },
-                        key: api_key(3, item)
+                        key: api_key(3, item.id)
                     },
                     [
                         api_element(

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-refence-item/actual.html

@@ -1,5 +1,5 @@
 <template>
     <ul>
-        <li for:each={items} for:item="item" class={item.x}>{item}</li>
+        <li key={item.id} for:each={items} for:item="item" class={item.x}>{item}</li>
     </ul>
 </template>

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-refence-item/expected.js

@@ -12,7 +12,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                     'li',
                     {
                         className: item.x,
-                        key: api_key(1, item)
+                        key: api_key(1, item.id)
                     },
                     [
                         api_dynamic(item)

packages/lwc-template-compiler/src/__tests__/fixtures/directive-for-each/inline-sibling/actual.html

@@ -1,6 +1,6 @@
 <template>
     <ul>
-        <li for:each={items} for:item="item" class={item.x}>{item}</li>
+        <li for:each={items} for:item="item" class={item.x} key={item.id}>{item}</li>
         <li>Last</li>
     </ul>
 </template>