-
Notifications
You must be signed in to change notification settings - Fork 386
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(engine-core): fix warnings for invalid keys #4097
Changes from all commits
2448984
bceb57a
5328f50
5a0eac4
c895be8
76ca44b
acf0665
1e36e15
4f08b88
316186a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary escapes in regexes, and some missing escapes (like for the |
||
); | ||
}); | ||
|
||
|
@@ -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', | ||
}, | ||
]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing the three scenarios we care about: Elements, CustomElements, and VStatics. |
||
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\./ | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We previously didn't have any tests for this "Duplicated key" error message. |
||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Custom extends LightningElement {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<template> | ||
<ul> | ||
<template for:each={items} for:item="item"> | ||
<x-custom key={item.key}></x-custom> | ||
</template> | ||
</ul> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class TestCustomElement extends LightningElement { | ||
@api items = []; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<template> | ||
<ul> | ||
<template for:each={items} for:item="item"> | ||
<li key={item.key}></li> | ||
</template> | ||
</ul> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement, api } from 'lwc'; | ||
|
||
export default class TestStatic extends LightningElement { | ||
@api items = []; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
childVnode.sel
is just a sniff test forVElement
orVCustomElement
since those are the only types that can have asel
. I prefer to be explicit and just check for thechildVnode.type
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need the check on
isObject
sincevnodes
should always contain avnode
.It doesn't break any karma tests when it's removed but I'm not sure if there's any edge cases I'm not thinking of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. It's not necessary.