Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from Mar 16, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/lwc-engine/package.json
Expand Up @@ -7,12 +7,11 @@
"typings": "types/engine.d.ts",
"scripts": {
"build": "concurrently \"yarn build:es-and-cjs\" \"yarn build:umd:dev\"",
"test": "echo 'Running lwc-engine tests...' && DIR=`pwd` && cd ../../ && jest $DIR",
"test": "echo 'Running lwc-engine tests...' && DIR=`pwd` && cd ../../ && yarn test $DIR",
"build:all": "concurrently \"yarn build:es-and-cjs\" \"yarn build:umd:prod\" \"yarn build:umd:dev\"",
"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",
"test": "DIR=`pwd` && cd ../../ && jest $DIR"
"build:es-and-cjs": "rollup -c scripts/rollup.config.es-and-cjs.js"
},
"devDependencies": {
"concurrently": "^3.5.1",
Expand Down
24 changes: 16 additions & 8 deletions packages/lwc-engine/src/framework/__tests__/api.spec.ts
Expand Up @@ -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 {
Expand All @@ -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();
});
});

});
3 changes: 3 additions & 0 deletions packages/lwc-engine/src/framework/__tests__/template.spec.ts
Expand Up @@ -50,6 +50,9 @@ describe('template', () => {
});

it('should render arrays correctly', function() {
function tmpl($a, $c, $s, $m) {
return
}
const elm = createCustomComponent(function($api, $cmp) {
return $api.i(['a', 'b'], function(value) {
return $api.h('div', { key: 0 }, [
Expand Down
35 changes: 15 additions & 20 deletions packages/lwc-engine/src/framework/api.ts
Expand Up @@ -8,7 +8,6 @@ import { ComponentConstructor, markComponentAsDirty } from "./component";

import { VNode, VNodeData, VNodes, VElement, VComment, VText, Hooks } from "../3rdparty/snabbdom/types";
import { getCustomElementVM } from "./html-element";
import { unwrap } from "./reactive";

export interface RenderAPI {
h(tagName: string, data: VNodeData, children: VNodes): VNode;
Expand Down Expand Up @@ -255,6 +254,11 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number,
let next = iterator.next();
let j = 0;
let { value, done: last } = next;
if (process.env.NODE_ENV !== 'production') {
// var is intentional here, function level scoping is required.
var keyMap = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of a better way to hoist a map of used keys that will get populated in the while loop. Suggestions happily taken :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this a little bit more, I think this will still get minified if I use let outside of the prod check

}

while (last === false) {
// implementing a look-back-approach because we need to know if the element is the last
next = iterator.next();
Expand All @@ -271,9 +275,14 @@ export function i(iterable: Iterable<any>, factory: (value: any, index: number,
if (process.env.NODE_ENV !== 'production') {
const vnodes = isArray(vnode) ? vnode : [vnode];
vnodes.forEach((childVnode) => {
if (!isNull(childVnode) && isObject(childVnode) && !isUndefined(childVnode.sel) && childVnode.sel.indexOf('-') > 0 && isUndefined(childVnode.key)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want the childVnode.sel.indexOf('-') check here. This is valid for all elements, not just custom

// 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)) {
if (isUndefined(childVnode.key)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about key being null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to just test for typeof to be number or string

// 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.`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead set a unique "key" - 'instead' word is not needed here since there was no alternative. We can just start with 'Set a unique key ...'

} else if (keyMap[childVnode.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[childVnode.key!] = 1;
}
});
}
Expand Down Expand Up @@ -352,10 +361,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':
Expand All @@ -364,17 +370,6 @@ 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);
}
return compilerKey + ':' + objKey;
throw new Error(`Invalid key value ${obj}. Key must be a string or number.`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted for an Error instead of assert because I believe we will want this in production.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will have to do toString() around this, otherwise it might throw that it can't be stringify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error is only incomplete, we should at least tell them what VM is this, but in prod we don't have toString on VM.

my recommendation is to keep it as an assert until the next refactor of the errors.

}
}
Expand Up @@ -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>
Expand Up @@ -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>
Expand All @@ -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>
Expand Up @@ -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>
@@ -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>
Expand Up @@ -2,7 +2,7 @@
<div>
<ul>
<li class="first">header</li>
<li class="number" for:each={data} for:item="item">
<li key={index} class="number" for:index="index" for:each={data} for:item="item">
Value of X = {item.x}
</li>
<li class="last">footer</li>
Expand Down
Expand Up @@ -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>
Expand Down
Expand Up @@ -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')
Expand All @@ -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')
Expand Down Expand Up @@ -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)
},
[]
);
Expand Down
@@ -0,0 +1,5 @@
<template>
<template for:each={items} for:item="item">
<p>1{item}</p>
</template>
</template>
@@ -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"]
}
}
@@ -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>
Expand Down
Expand Up @@ -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),
Expand Down
@@ -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>
Expand Down
Expand Up @@ -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(
Expand Down
@@ -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>
Expand Down
Expand Up @@ -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(
Expand Down
@@ -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>
Expand Up @@ -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)
Expand Down
@@ -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>
Expand Up @@ -20,7 +20,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)]
);
Expand Down
@@ -1,6 +1,6 @@
<template>
<section>
<div class="my-list" for:each={items} for:item="item">
<div class="my-list" for:each={items} key={item.id} for:item="item">
<p>items</p>
</div>
</section>
Expand Down
Expand Up @@ -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(
Expand Down