Skip to content

Commit

Permalink
fix(runtime): allow setting key attr on nested Stencil components (#…
Browse files Browse the repository at this point in the history 
…5164)

This makes it possible to set a key attribute on a nested Stencil
component without messing up how hydration works. This fixes a bug which
was noticed while working on #5143.
  • Loading branch information
@alicewriteswrongs
alicewriteswrongs authored Dec 13, 2023
1 parent 2d16db6 commit f6903a8
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 31 deletions.
76 changes: 61 additions & 15 deletions src/runtime/test/hydrate-no-encapsulation.spec.tsx
View file
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 () => {
@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
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

0 comments on commit f6903a8

Please sign in to comment.