Make attributes.length optional
#107
Conversation
SantosGuillamot
changed the base branch from
main-custom-elements-hydration
to
main-wp-directives-plugin
SantosGuillamot
requested review from
cbravobernal,
DAreRodz and
michalczaplinski
src/runtime/vdom.js
Outdated
| @@ -9,7 +9,7 @@ export default function toVdom(node) { | |||
|
|
|||
| if (node.nodeType === 3) return node.data; | |||
|
|
|||
| for (let i = 0; i < attributes.length; i++) { | |||
| for (let i = 0; i < attributes?.length; i++) { | |||
There was a problem hiding this comment.
attributes[i].name will also crash if attributes is undefined
There was a problem hiding this comment.
Oh, I thought that if attributes is undefined, it wasn't ever going to enter the for loop. What should we do then? Wrapping the whole for loop in a conditional like if(attributes){ for (...){} }?
There was a problem hiding this comment.
It is a curious case:
As you first in the file use destructure to get attributes (you could also define let attributes; which also defaults to undefined), in this case: attributes?.length is 'undefined'.
So the comparison is 0 < undefined which indeed is false 😆, so your thought is true, and it should not enter the loop.
So the code should work, but you may have unexpected comparisons 😅 . So I guess in this case preventing the loop with the conditional would be a good approach.
In case you try in a console without initializing attributes, it should crash 😄
There was a problem hiding this comment.
Thanks a lot for the explanation! It totally makes sense 🙂
I've just pushed a new commit with your suggestion. Hope that's correct 😁
|
I've inspected why those nodes don't have |
|
Great! That totally makes sense. Should I revert the PR to ensure we are not skipping other use cases like CDATA? |
|
Let's see what comes up from #109 first. |
We have found a couple of sites where our hydration script is failing because
attributes.lengthisundefined. It seems that adding?to it to make it optional solves the problem. These are the sites where it is failing: