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.

Sign up for GitHub

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(runtime): allow setting key attr on nested Stencil components #5164

Merged
merged 1 commit into from
Dec 13, 2023
Merged

fix(runtime): allow setting key attr on nested Stencil components #5164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 61 additions & 15 deletions src/runtime/test/hydrate-no-encapsulation.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA],
html: `<cmp-a></cmp-a>`,
Expand All @@ -38,7 +37,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA],
html: `<cmp-a></cmp-a>`,
Expand All @@ -54,7 +52,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA],
html: serverHydrated.root.outerHTML,
Expand All @@ -81,7 +78,6 @@ describe('hydrate no encapsulation', () => {
return <Host>Hello</Host>;
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA],
html: `<cmp-a></cmp-a>`,
Expand All @@ -95,7 +91,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA],
html: serverHydrated.root.outerHTML,
Expand Down Expand Up @@ -126,7 +121,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA],
html: `<cmp-a></cmp-a>`,
Expand All @@ -146,7 +140,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA],
html: serverHydrated.root.outerHTML,
Expand All @@ -166,6 +159,67 @@ describe('hydrate no encapsulation', () => {
`);
});

it('nested text slot with key', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is a reproduction of the issue I was seeing, if you wanted to see what happens you could check out this branch locally and either run the test without rebuilding or

git checkout origin/main -- src/runtime/vdom/vdom-render.ts
npm run build
npx jest --clearCache
npx --node-options=--experimental-vm-modules jest --coverage=false -- hydrate-no-encapsulation

which will run this test without the fix so you can see what happens

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran this locally, adding what I found for posterity:

  ● hydrate no encapsulation › nested text slot with key

    expect(received).toBe(expected) // Object.is equality

    - Expected  - 3
    + Received  + 1

      <cmp-a class="hydrated">
        <!--r.1-->
        <cmp-b class="hydrated">
    -     <!--r.2-->
    -     <!---->
    -     <!--s.2.0.0.0.-->
    +     <!---->
          light-dom
          <footer></footer>
        </cmp-b>
      </cmp-a>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup that's what I expect to see 👍

@Component({ tag: 'cmp-a' })
class CmpA {
render() {
return (
<Host key="test">
<cmp-b key="my-test-key">light-dom</cmp-b>
</Host>
);
}
}
@Component({ tag: 'cmp-b' })
class CmpB {
render() {
return (
<Host>
<slot key="key1"></slot>
<footer key="key2"></footer>
</Host>
);
}
}
const serverHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: `<cmp-a></cmp-a>`,
hydrateServerSide: true,
});
expect(serverHydrated.root).toEqualHtml(`
<cmp-a class="hydrated" s-id="1">
<!--r.1-->
<cmp-b class="hydrated" c-id="1.0.0.0" s-id="2">
<!--r.2-->
<!--o.1.1-->
<!--s.2.0.0.0.-->
<!--t.1.1.1.0-->
light-dom
<footer c-id="2.1.0.1"></footer>
</cmp-b>
</cmp-a>
`);

const clientHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: serverHydrated.root.outerHTML,
hydrateClientSide: true,
});

expect(clientHydrated.root).toEqualHtml(`
<cmp-a class="hydrated">
<!--r.1-->
<cmp-b class="hydrated">
<!--r.2-->
<!---->
<!--s.2.0.0.0.-->
light-dom
<footer></footer>
</cmp-b>
</cmp-a>
`);
});

it('nested, text slot, footer', async () => {
@Component({ tag: 'cmp-a' })
class CmpA {
Expand All @@ -188,7 +242,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: `<cmp-a></cmp-a>`,
Expand All @@ -208,7 +261,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: serverHydrated.root.outerHTML,
Expand Down Expand Up @@ -252,7 +304,6 @@ describe('hydrate no encapsulation', () => {
}
}

// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: `<cmp-a></cmp-a>`,
Expand All @@ -272,7 +323,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: serverHydrated.root.outerHTML,
Expand Down Expand Up @@ -316,7 +366,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: `<cmp-a></cmp-a>`,
Expand All @@ -337,7 +386,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: serverHydrated.root.outerHTML,
Expand Down Expand Up @@ -388,7 +436,6 @@ describe('hydrate no encapsulation', () => {
);
}
}
// @ts-ignore
const serverHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: `<cmp-a></cmp-a>`,
Expand Down Expand Up @@ -421,7 +468,6 @@ describe('hydrate no encapsulation', () => {
</cmp-a>
`);

// @ts-ignore
const clientHydrated = await newSpecPage({
components: [CmpA, CmpB],
html: serverHydrated.root.outerHTML,
Expand Down
46 changes: 30 additions & 16 deletions src/runtime/vdom/vdom-render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,15 @@ const removeVnodes = (vnodes: d.VNode[], startIdx: number, endIdx: number) => {
* @param oldCh the old children of the parent node
* @param newVNode the new VNode which will replace the parent
* @param newCh the new children of the parent node
* @param isInitialRender whether or not this is the first render of the vdom
*/
const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.VNode, newCh: d.VNode[]) => {
const updateChildren = (
parentElm: d.RenderNode,
oldCh: d.VNode[],
newVNode: d.VNode,
newCh: d.VNode[],
isInitialRender = false,
) => {
let oldStartIdx = 0;
let newStartIdx = 0;
let idxInOld = 0;
Expand All @@ -418,22 +425,22 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
newStartVnode = newCh[++newStartIdx];
} else if (newEndVnode == null) {
newEndVnode = newCh[--newEndIdx];
} else if (isSameVnode(oldStartVnode, newStartVnode)) {
} else if (isSameVnode(oldStartVnode, newStartVnode, isInitialRender)) {
// if the start nodes are the same then we should patch the new VNode
// onto the old one, and increment our `newStartIdx` and `oldStartIdx`
// indices to reflect that. We don't need to move any DOM Nodes around
// since things are matched up in order.
patch(oldStartVnode, newStartVnode);
patch(oldStartVnode, newStartVnode, isInitialRender);
oldStartVnode = oldCh[++oldStartIdx];
newStartVnode = newCh[++newStartIdx];
} else if (isSameVnode(oldEndVnode, newEndVnode)) {
} else if (isSameVnode(oldEndVnode, newEndVnode, isInitialRender)) {
// likewise, if the end nodes are the same we patch new onto old and
// decrement our end indices, and also likewise in this case we don't
// need to move any DOM Nodes.
patch(oldEndVnode, newEndVnode);
patch(oldEndVnode, newEndVnode, isInitialRender);
oldEndVnode = oldCh[--oldEndIdx];
newEndVnode = newCh[--newEndIdx];
} else if (isSameVnode(oldStartVnode, newEndVnode)) {
} else if (isSameVnode(oldStartVnode, newEndVnode, isInitialRender)) {
// case: "Vnode moved right"
//
// We've found that the last node in our window on the new children is
Expand All @@ -451,7 +458,7 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
if (BUILD.slotRelocation && (oldStartVnode.$tag$ === 'slot' || newEndVnode.$tag$ === 'slot')) {
putBackInOriginalLocation(oldStartVnode.$elm$.parentNode, false);
}
patch(oldStartVnode, newEndVnode);
patch(oldStartVnode, newEndVnode, isInitialRender);
// We need to move the element for `oldStartVnode` into a position which
// will be appropriate for `newEndVnode`. For this we can use
// `.insertBefore` and `oldEndVnode.$elm$.nextSibling`. If there is a
Expand All @@ -472,7 +479,7 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
parentElm.insertBefore(oldStartVnode.$elm$, oldEndVnode.$elm$.nextSibling as any);
oldStartVnode = oldCh[++oldStartIdx];
newEndVnode = newCh[--newEndIdx];
} else if (isSameVnode(oldEndVnode, newStartVnode)) {
} else if (isSameVnode(oldEndVnode, newStartVnode, isInitialRender)) {
// case: "Vnode moved left"
//
// We've found that the first node in our window on the new children is
Expand All @@ -491,7 +498,7 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
if (BUILD.slotRelocation && (oldStartVnode.$tag$ === 'slot' || newEndVnode.$tag$ === 'slot')) {
putBackInOriginalLocation(oldEndVnode.$elm$.parentNode, false);
}
patch(oldEndVnode, newStartVnode);
patch(oldEndVnode, newStartVnode, isInitialRender);
// We've already checked above if `oldStartVnode` and `newStartVnode` are
// the same node, so since we're here we know that they are not. Thus we
// can move the element for `oldEndVnode` _before_ the element for
Expand Down Expand Up @@ -528,7 +535,7 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
// the tag doesn't match so we'll need a new DOM element
node = createElm(oldCh && oldCh[newStartIdx], newVNode, idxInOld, parentElm);
} else {
patch(elmToMove, newStartVnode);
patch(elmToMove, newStartVnode, isInitialRender);
// invalidate the matching old node so that we won't try to update it
// again later on
oldCh[idxInOld] = undefined;
Expand Down Expand Up @@ -590,17 +597,22 @@ const updateChildren = (parentElm: d.RenderNode, oldCh: d.VNode[], newVNode: d.V
*
* @param leftVNode the first VNode to check
* @param rightVNode the second VNode to check
* @param isInitialRender whether or not this is the first render of the vdom
* @returns whether they're equal or not
*/
export const isSameVnode = (leftVNode: d.VNode, rightVNode: d.VNode) => {
export const isSameVnode = (leftVNode: d.VNode, rightVNode: d.VNode, isInitialRender = false) => {
// compare if two vnode to see if they're "technically" the same
// need to have the same element tag, and same key to be the same
if (leftVNode.$tag$ === rightVNode.$tag$) {
if (BUILD.slotRelocation && leftVNode.$tag$ === 'slot') {
return leftVNode.$name$ === rightVNode.$name$;
}
// this will be set if components in the build have `key` attrs set on them
if (BUILD.vdomKey) {
// this will be set if JSX tags in the build have `key` attrs set on them
// we only want to check this if we're not on the first render since on
// first render `leftVNode.$key$` will always be `null`, so we can be led
// astray and, for instance, accidentally delete a DOM node that we want to
// keep around.
if (BUILD.vdomKey && !isInitialRender) {
return leftVNode.$key$ === rightVNode.$key$;
}
return true;
Expand All @@ -625,8 +637,9 @@ const parentReferenceNode = (node: d.RenderNode) => (node['s-ol'] ? node['s-ol']
*
* @param oldVNode an old VNode whose DOM element and children we want to update
* @param newVNode a new VNode representing an updated version of the old one
* @param isInitialRender whether or not this is the first render of the vdom
*/
export const patch = (oldVNode: d.VNode, newVNode: d.VNode) => {
export const patch = (oldVNode: d.VNode, newVNode: d.VNode, isInitialRender = false) => {
const elm = (newVNode.$elm$ = oldVNode.$elm$);
const oldChildren = oldVNode.$children$;
const newChildren = newVNode.$children$;
Expand Down Expand Up @@ -655,7 +668,7 @@ export const patch = (oldVNode: d.VNode, newVNode: d.VNode) => {
if (BUILD.updatable && oldChildren !== null && newChildren !== null) {
// looks like there's child vnodes for both the old and new vnodes
// so we need to call `updateChildren` to reconcile them
updateChildren(elm, oldChildren, newVNode, newChildren);
updateChildren(elm, oldChildren, newVNode, newChildren, isInitialRender);
} else if (newChildren !== null) {
// no old child vnodes, but there are new child vnodes to add
if (BUILD.updatable && BUILD.vdomText && oldVNode.$text$ !== null) {
Expand Down Expand Up @@ -944,6 +957,7 @@ render() {
}
`);
}

if (BUILD.reflect && cmpMeta.$attrsToReflect$) {
rootVnode.$attrs$ = rootVnode.$attrs$ || {};
cmpMeta.$attrsToReflect$.map(
Expand Down Expand Up @@ -990,7 +1004,7 @@ render() {
}

// synchronous patch
patch(oldVNode, rootVnode);
patch(oldVNode, rootVnode, isInitialLoad);

if (BUILD.slotRelocation) {
// while we're moving nodes around existing nodes, temporarily disable
Expand Down