Skip to content
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

test: add test for snabbdom mutating old children #2602

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

nolanlawson
Copy link
Collaborator

@nolanlawson nolanlawson commented Dec 9, 2021

Details

See discussion here: #2589 (comment)

Prior to this PR, we didn't have any Jest or Karma tests to exercise the else block in this code path (line 133):

idxInOld = oldKeyToIdx[newStartVnode.key!];
if (isUndef(idxInOld)) {
// New element
newStartVnode.hook.create(newStartVnode);
newStartVnode.hook.insert(newStartVnode, parentElm, oldStartVnode.elm!);
newStartVnode = newCh[++newStartIdx];
} else {
elmToMove = oldCh[idxInOld];
if (isVNode(elmToMove)) {
if (elmToMove.sel !== newStartVnode.sel) {
// New element
newStartVnode.hook.create(newStartVnode);
newStartVnode.hook.insert(newStartVnode, parentElm, oldStartVnode.elm!);
} else {
patchVnode(elmToMove, newStartVnode);
oldCh[idxInOld] = undefined as any;
newStartVnode.hook.move(elmToMove, parentElm, oldStartVnode.elm!);
}
}
newStartVnode = newCh[++newStartIdx];

This PR adds a test that does hit this line of code:

Screen Shot 2021-12-09 at 11 53 19 AM

Interestingly, the if block here seems completely unreachable:

if (elmToMove.sel !== newStartVnode.sel) {

The only way it could be reached is if two VNodes with two different tags (e.g. x-foo and x-bar) occupy the same positions in the children array. I tried to repro this using both:

  1. lwc:dynamic and
  2. <template if:true> inside of a <template for:each>

…but neither seem to work. In the case of (1), the dynamic component has the same tag name (e.g. x-dynamic) regardless of whether it's actually x-foo or x-bar under the hood, and in the case of (2) there are holes in the array for the <template if:true> and <template if:false>, so two different element types cannot occupy the same index in the array.

We can probably remove that part of the code, but I'd prefer not to, because maybe someday we will refactor our templates to avoid creating holes for if/else blocks?

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

expect(r2).toBe(e2);
expect(r3).toBe(e3);
});
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the test above; it's basically the same, except I changed one row.

@nolanlawson
Copy link
Collaborator Author

nolanlawson commented Dec 9, 2021

Also: I can confirm that the test fails if I remove this line:

oldCh[idxInOld] = undefined as any;

The error is:

Chrome 96.0.4664.93 (Mac OS 10.15.7) rendering table-diffing Table diffing should reuse the elements when moving the end to the start and adding new to the end FAILED
        Expected $.length = 3 to equal 4.
        Expected $[1] = 2 to equal 1.
        Expected $[2] = 4 to equal 2.
        Expected $[3] = undefined to equal 4.
        Error: Expected $.length = 3 to equal 4.
        Expected $[1] = 2 to equal 1.
        Expected $[2] = 4 to equal 2.
        Expected $[3] = undefined to equal 4.

Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent!

@nolanlawson nolanlawson merged commit 39a1d90 into master Dec 9, 2021
@nolanlawson nolanlawson deleted the nolan/repro-snabbdom-code-path branch December 9, 2021 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants