fix(engine-core): fix warnings for invalid keys #4097
Conversation
| @@ -431,16 +432,25 @@ 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)) { | |||
There was a problem hiding this comment.
childVnode.sel is just a sniff test for VElement or VCustomElement since those are the only types that can have a sel. I prefer to be explicit and just check for the childVnode.type instead.
There was a problem hiding this comment.
I wonder if we need the check on isObject since vnodes should always contain a vnode.
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.
I think you're right. It's not necessary.
| // 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 \S+ \(\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.
We previously didn't have any tests for this "Duplicated key" error message.
| Ctor: XTestCustomElement, | ||
| tagName: 'x-test-custom-element', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Testing the three scenarios we care about: Elements, CustomElements, and VStatics.
| @@ -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.
Unnecessary escapes in regexes, and some missing escapes (like for the .).
| @@ -431,16 +432,25 @@ 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)) { | |||
There was a problem hiding this comment.
I wonder if we need the check on isObject since vnodes should always contain a vnode.
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.
Details
Now that static blocks support
keys (#4091), this dev-only error message became incorrect. If the invalidkeyis on a static fragment, then we weren't properly checking for that – we assumed it would only be on an element or custom element.This fixes that, as well as adding more robust tests to capture more ways that this can log.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?