-
Notifications
You must be signed in to change notification settings - Fork 272
chore(find): proper typings #129
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
Conversation
console.log(reversedChildren) | ||
reversedChildren.forEach( | ||
(node: string | number | boolean | VNodeArrayChildren | VNode) => { | ||
nodes.unshift(node) |
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 need help here, please.
If I follow correctly until that point, children
is of type VNodeNormalizedChildren
.
That means its elements, after filtering the potential null/undefined ones (which I added to be safe), can be either string | number | boolean | VNodeArrayChildren | VNode
(see https://github.com/vuejs/vue-next/blob/24168bbb333727417ddcb310c615afc55ee923ad/packages/runtime-core/src/vnode.ts#L85-L92) . The current code considers it is always a VNode. Can we be sure of that?
If yes, I can force the cast and add a comment explaining why.
If no, we need to handle the other cases, but I'm not sure what needs to be done.
cc @dobromir-hristov
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 I was just collecting all children, then in the match
I am filtering away the unnecessary.
Children can be other things, but I did not yet find such cases cases.
String
means its just a plain DOM element.
VNode
is Vue component instance.
Boolean I have no idea when 😆
VNodeArrayChildren
is probably for multi root components? idk.. :/
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.
Right, I did not manage to reproduce a case where we have VNodeArrayChildren
either. To be safe, I added a branch if we end up with it, and call aggregateChildren on them. We should be safe in all cases.
b55916d
to
06aa50d
Compare
The PR is now up for review. |
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.
Quite difficult to review this (I don't have a ton of context on this part of the codebase).
Not exactly sure how useful these particular types are, but I don't think it makes the codebase worse by any means. Just left one comment!
Adds some typings to the find functions, and handle a case surfaced by TS (but that I did not manage to reproduce via unit tests, so it might never happen).