fix(engine-core): fix warnings for invalid keys (#4097)

Author: nolanlawson

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

@@ -22,6 +22,7 @@ import {
     isTrue,
     isUndefined,
     StringReplace,
+    StringToLowerCase,
     toString,
 } from '@lwc/shared';
 
@@ -35,7 +36,9 @@ import { RenderMode, ShadowMode, SlotSet, VM } from './vm';
 import { LightningElementConstructor } from './base-lightning-element';
 import { markAsDynamicChildren } from './rendering';
 import {
+    isVBaseElement,
     isVScopedSlotFragment,
+    isVStatic,
     Key,
     VComment,
     VCustomElement,
@@ -47,11 +50,9 @@ import {
     VNodeType,
     VScopedSlotFragment,
     VStatic,
-    VText,
     VStaticPart,
     VStaticPartData,
-    isVBaseElement,
-    isVStatic,
+    VText,
 } from './vnodes';
 import { getComponentRegisteredName } from './component';
 
@@ -432,15 +433,18 @@ function i(
         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)) {
+                // Check that the child vnode is either an element or VStatic
+                if (!isNull(childVnode) && (isVBaseElement(childVnode) || isVStatic(childVnode))) {
                     const { key } = childVnode;
+                    const tagName =
+                        childVnode.sel ?? StringToLowerCase.call(childVnode.fragment.tagName);
                     if (isString(key) || isNumber(key)) {
                         if (keyMap[key] === 1 && isUndefined(iterationError)) {
-                            iterationError = `Duplicated "key" attribute value for "<${childVnode.sel}>" in ${vmBeingRendered} for item number ${j}. A key with value "${childVnode.key}" appears more than once in the iteration. Key values must be unique numbers or strings.`;
+                            iterationError = `Duplicated "key" attribute value for "<${tagName}>" in ${vmBeingRendered} for item number ${j}. A key with value "${childVnode.key}" appears more than once in the iteration. Key values must be unique numbers or strings.`;
                         }
                         keyMap[key] = 1;
                     } else if (isUndefined(iterationError)) {
-                        iterationError = `Invalid "key" attribute value in "<${childVnode.sel}>" in ${vmBeingRendered} for item number ${j}. Set a unique "key" value on all iterated child elements.`;
+                        iterationError = `Invalid "key" attribute value in "<${tagName}>" in ${vmBeingRendered} for item number ${j}. Set a unique "key" value on all iterated child elements.`;
                     }
                 }
             });

packages/@lwc/integration-karma/test/template/directive-for-each/index.spec.js

@@ -1,5 +1,7 @@
 import { createElement } from 'lwc';
 import XTest from 'x/test';
+import XTestStatic from 'x/testStatic';
+import XTestCustomElement from 'x/testCustomElement';
 import ArrayNullPrototype from 'x/arrayNullPrototype';
 
 function testForEach(type, obj) {
@@ -64,7 +66,7 @@ it('should throw an error when the passing a non iterable', () => {
 
     // TODO [#1283]: Improve this error message. The vm should not be exposed and the message is not helpful.
     expect(() => document.body.appendChild(elm)).toThrowCallbackReactionError(
-        /Invalid template iteration for value `\[object (ProxyObject|Object)\]` in \[object:vm Test \(\d+\)\]. It must be an array-like object and not `null` nor `undefined`.|is not a function/
+        /Invalid template iteration for value `\[object (ProxyObject|Object)]` in \[object:vm Test \(\d+\)]\. It must be an array-like object and not `null` nor `undefined`\.|is not a function/
     );
 });
 
@@ -75,13 +77,47 @@ it('should render an array of objects with null prototype', () => {
     expect(elm.shadowRoot.querySelector('span').textContent).toBe('text');
 });
 
-it('logs an error when passing an invalid key', () => {
-    const elm = createElement('x-test', { is: XTest });
-    elm.items = [{ key: null, value: 'one' }];
+const scenarios = [
+    {
+        testName: 'dynamic text node',
+        Ctor: XTest,
+        tagName: 'x-test',
+    },
+    {
+        testName: 'static text node',
+        Ctor: XTestStatic,
+        tagName: 'x-test-static',
+    },
+    {
+        testName: 'custom element',
+        Ctor: XTestCustomElement,
+        tagName: 'x-test-custom-element',
+    },
+];
+scenarios.forEach(({ testName, Ctor, tagName }) => {
+    describe(testName, () => {
+        it('logs an error when passing an invalid key', () => {
+            const elm = createElement(tagName, { is: Ctor });
+            elm.items = [{ key: null, value: 'one' }];
 
-    // TODO [#1283]: Improve this error message. The vm should not be exposed and the message is not helpful.
-    expect(() => document.body.appendChild(elm)).toLogErrorDev([
-        /Invalid key value "null" in \[object:vm Test \(\d+\)\]. Key must be a string or number./,
-        /Invalid "key" attribute value in "<li>"/,
-    ]);
+            // TODO [#1283]: Improve this error message. The vm should not be exposed and the message is not helpful.
+            expect(() => document.body.appendChild(elm)).toLogErrorDev([
+                /Invalid key value "null" in \[object:vm (TestStatic|TestCustomElement|Test) \(\d+\)]. Key must be a string or number\./,
+                /Invalid "key" attribute value in "<(li|x-custom)>"/,
+            ]);
+        });
+
+        it('logs an error when passing a duplicate key', () => {
+            const elm = createElement(tagName, { is: Ctor });
+            elm.items = [
+                { key: 'xyz', value: 'one' },
+                { key: 'xyz', value: 'two' },
+            ];
+
+            // TODO [#1283]: Improve this error message. The vm should not be exposed and the message is not helpful.
+            expect(() => document.body.appendChild(elm)).toLogErrorDev(
+                /Duplicated "key" attribute value for "<(li|x-custom)>" in \[object:vm (TestStatic|TestCustomElement|Test) \(\d+\)] for item number 1\. A key with value "\d:xyz" appears more than once in the iteration\. Key values must be unique numbers or strings\./
+            );
+        });
+    });
 });

packages/@lwc/integration-karma/test/template/directive-for-each/x/custom/custom.js

@@ -0,0 +1,3 @@
+import { LightningElement } from 'lwc';
+
+export default class Custom extends LightningElement {}

packages/@lwc/integration-karma/test/template/directive-for-each/x/test/test.html

@@ -1,7 +1,10 @@
 <template>
     <ul>
         <template for:each={items} for:item="item">
-            <li key={item.key}>{item.value}</li>
+            <li key={item.key}>
+                <!-- putting an lwc:if in here so that the LI will always be VElement instead of VStatic -->
+                <template lwc:if={item.value}>{item.value}</template>
+            </li>
         </template>
     </ul>
 </template>

packages/@lwc/integration-karma/test/template/directive-for-each/x/testCustomElement/testCustomElement.html

@@ -0,0 +1,7 @@
+<template>
+    <ul>
+        <template for:each={items} for:item="item">
+            <x-custom key={item.key}></x-custom>
+        </template>
+    </ul>
+</template>

packages/@lwc/integration-karma/test/template/directive-for-each/x/testCustomElement/testCustomElement.js

@@ -0,0 +1,5 @@
+import { LightningElement, api } from 'lwc';
+
+export default class TestCustomElement extends LightningElement {
+    @api items = [];
+}

packages/@lwc/integration-karma/test/template/directive-for-each/x/testStatic/testStatic.html

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

packages/@lwc/integration-karma/test/template/directive-for-each/x/testStatic/testStatic.js

@@ -0,0 +1,5 @@
+import { LightningElement, api } from 'lwc';
+
+export default class TestStatic extends LightningElement {
+    @api items = [];
+}