-
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: add selector type to all vnodes #4234
Conversation
// Empty fragment | ||
expect(ul.children.length).toBe(0); | ||
|
||
const spy = spyConsole(); |
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'm importing the spy directly because the error is logged by the engine and can't be caught directly when elm.items = [{ value: 1 }]
.
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.
LGTM
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.
Is sel
always defined now? If so, should we update the type definition for BaseVNode
?
packages/@lwc/integration-karma/test/regression/invalid-key/index.spec.js
Outdated
Show resolved
Hide resolved
@jmsjtu Might as well add |
Agree with Nolan's comment, ScopeSlotFragment can get a sel too. |
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.
🤞
Details
By expanding static content optimization to more nodes we've revealed a bug with the existing diffing algo.
In special circumstances when a text vnode is compared against a static vnode they will be considered the same vnode because we use the
sel
andkey
to determine vnode equality.Since both text and static vnodes have an
undefined
key
andsel
values they are incorrectly evaluated.This PR updates the values of
sel
to distinguish these vnodes and prevent future false positives.Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item