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

chore: use comment nodes for VFragment bookends #3846

Merged
merged 8 commits into from
Nov 8, 2023
13 changes: 10 additions & 3 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,23 @@ function st(fragment: Element, key: Key, parts?: VStaticPart[]): VStatic {

// [fr]agment node
function fr(key: Key, children: VNodes, stable: 0 | 1): VFragment {
const leading = t('');
const trailing = t('');
const owner = getVMBeingRendered()!;
const useCommentNodes = isAPIFeatureEnabled(
APIFeature.USE_COMMENTS_FOR_FRAGMENT_BOOKENDS,
owner.apiVersion
);

const leading = useCommentNodes ? co('') : t('');
const trailing = useCommentNodes ? co('') : t('');
divmain marked this conversation as resolved.
Show resolved Hide resolved

return {
type: VNodeType.Fragment,
sel: undefined,
key,
elm: undefined,
children: [leading, ...children, trailing],
stable,
owner: getVMBeingRendered()!,
owner,
leading,
trailing,
};
Expand Down
32 changes: 26 additions & 6 deletions packages/@lwc/engine-core/src/framework/hydration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import {
isTrue,
isString,
StringToLowerCase,
APIFeature,
isAPIFeatureEnabled,
} from '@lwc/shared';

import { logError, logWarn } from '../shared/logger';
Expand Down Expand Up @@ -89,7 +91,7 @@ function hydrateVM(vm: VM) {
renderRoot: parentNode,
renderer: { getFirstChild },
} = vm;
hydrateChildren(getFirstChild(parentNode), children, parentNode, vm);
hydrateChildren(getFirstChild(parentNode), children, parentNode, vm, false);
runRenderedCallback(vm);
}

Expand Down Expand Up @@ -234,7 +236,7 @@ function hydrateStaticElement(elm: Node, vnode: VStatic, renderer: RendererAPI):
function hydrateFragment(elm: Node, vnode: VFragment, renderer: RendererAPI): Node | null {
const { children, owner } = vnode;

hydrateChildren(elm, children, renderer.getProperty(elm, 'parentNode'), owner);
hydrateChildren(elm, children, renderer.getProperty(elm, 'parentNode'), owner, true);

return (vnode.elm = children[children.length - 1]!.elm as Node);
}
Expand Down Expand Up @@ -287,7 +289,7 @@ function hydrateElement(elm: Node, vnode: VElement, renderer: RendererAPI): Node

if (!isDomManual) {
const { getFirstChild } = renderer;
hydrateChildren(getFirstChild(elm), vnode.children, elm, owner);
hydrateChildren(getFirstChild(elm), vnode.children, elm, owner, false);
}

return elm;
Expand Down Expand Up @@ -344,7 +346,7 @@ function hydrateCustomElement(
const { getFirstChild } = renderer;
// VM is not rendering in Light DOM, we can proceed and hydrate the slotted content.
// Note: for Light DOM, this is handled while hydrating the VM
hydrateChildren(getFirstChild(elm), vnode.children, elm as Element, vm);
hydrateChildren(getFirstChild(elm), vnode.children, elm as Element, vm, false);
}

hydrateVM(vm);
Expand All @@ -355,7 +357,11 @@ function hydrateChildren(
node: Node | null,
children: VNodes,
parentNode: Element | ShadowRoot,
owner: VM
owner: VM,
// When rendering the children of a VFragment, additional siblings may follow the
// last node of the fragment. Hydration should not fail if a trailing sibling is
// found in this case.
expectAddlSiblings: boolean
divmain marked this conversation as resolved.
Show resolved Hide resolved
) {
let hasWarned = false;
let nextNode: Node | null = node;
Expand Down Expand Up @@ -383,7 +389,21 @@ function hydrateChildren(
}
}

if (nextNode) {
const useCommentsForBookends = isAPIFeatureEnabled(
APIFeature.USE_COMMENTS_FOR_FRAGMENT_BOOKENDS,
owner.apiVersion
);
if (
// If 1) comments are used for bookends, and 2) we're not expecting additional siblings,
// and 3) there exists an additional sibling, that's a hydration failure.
divmain marked this conversation as resolved.
Show resolved Hide resolved
//
// This preserves the previous behavior for text-node bookends where mismatches
// would incorrectly occur but which is unfortunately baked into the SSR hydration
// contract. It also preserves the behavior of valid hydration failures where the server
// rendered more nodes than the client.
(!useCommentsForBookends || !expectAddlSiblings) &&
nextNode
) {
hasMismatch = true;
if (process.env.NODE_ENV !== 'production') {
if (!hasWarned) {
Expand Down
6 changes: 4 additions & 2 deletions packages/@lwc/engine-core/src/framework/vnodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ export interface VFragment extends BaseVNode, BaseVParent {

// which diffing strategy to use.
stable: 0 | 1;
leading: VText;
trailing: VText;
// The leading and trailing nodes are text nodes when APIFeature.USE_COMMENTS_FOR_FRAGMENT_BOOKENDS
// is disabled and comment nodes when it is enabled.
leading: VText | VComment;
trailing: VText | VComment;
}

export interface VText extends BaseVNode {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
<template shadowroot="open">
<x-basic-child>
<div>
‍‍
<!---->
<!---->
divmain marked this conversation as resolved.
Show resolved Hide resolved
<span>
99 - ssr
</span>
‍‍
<!---->
<!---->
</div>
</x-basic-child>
</template>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@
<x-child-with-for-each>
<ul>
<li>
‍‍
<!---->
<!---->
<span>
39 - Audio
</span>
‍‍
<!---->
<!---->
</li>
<li>
‍‍
<!---->
<!---->
<span>
40 - Video
</span>
‍‍
<!---->
<!---->
</li>
</ul>
</x-child-with-for-each>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@
<template shadowroot="open">
<x-child>
<div>
‍‍
<!---->
<!---->
<span>
99 - ssr
</span>
‍‍
<!---->
<!---->
</div>
<p>
‍Default slot - default content‍
<!---->
Default slot - default content
<!---->
</p>
</x-child>
</template>
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/integration-karma/helpers/test-hydrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ window.HydrateTest = (function (lwc, testUtils) {
Component,
hydrateComponent: lwc.hydrateComponent.bind(lwc),
consoleSpy,
container,
selector,
});
}

Expand Down
6 changes: 5 additions & 1 deletion packages/@lwc/integration-karma/scripts/shared/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

'use strict';

const { HIGHEST_API_VERSION } = require('@lwc/shared');

// Helpful error. Remove after a few months.
if (process.env.NATIVE_SHADOW) {
throw new Error('NATIVE_SHADOW is deprecated. Use DISABLE_SYNTHETIC instead!');
Expand All @@ -28,7 +30,9 @@ const DISABLE_STATIC_CONTENT_OPTIMIZATION = Boolean(
process.env.DISABLE_STATIC_CONTENT_OPTIMIZATION
);
const NODE_ENV_FOR_TEST = process.env.NODE_ENV_FOR_TEST;
const API_VERSION = process.env.API_VERSION && parseInt(process.env.API_VERSION, 10);
const API_VERSION = process.env.API_VERSION
? parseInt(process.env.API_VERSION, 10)
: HIGHEST_API_VERSION;

module.exports = {
// Test configuration
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
export default {
props: {},
snapshot(target) {
const first = target.shadowRoot.querySelector('.first');
const second = target.shadowRoot.querySelector('.second');

return {
first,
second,
};
},
advancedTest(target, { Component, hydrateComponent, consoleSpy, container, selector }) {
const snapshotBeforeHydration = this.snapshot(target);
hydrateComponent(target, Component, this.props);
const hydratedTarget = container.querySelector(selector);
const snapshotAfterHydration = this.snapshot(hydratedTarget);

for (const snapshotKey of Object.keys(snapshotBeforeHydration)) {
expect(snapshotBeforeHydration[snapshotKey])
.withContext(
`${snapshotKey} should be the same DOM element both before and after hydration`
)
.toBe(snapshotAfterHydration[snapshotKey]);
expect(snapshotBeforeHydration[snapshotKey].childNodes)
.withContext(
`${snapshotKey} should have the same number of child nodes before & after hydration`
)
.toHaveSize(snapshotAfterHydration[snapshotKey].childNodes.length);
}

expect(consoleSpy.calls.warn).toHaveSize(0);
expect(consoleSpy.calls.error).toHaveSize(0);
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<template>
<p class="first">{zeroLengthText}‍{zeroLengthText}‍</p>
<p class="second">{zeroLengthText}{zeroLengthText}{zeroLengthText}{zeroLengthText}</p>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement } from 'lwc';

export default class Main extends LightningElement {
zeroLengthText = '';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
export default {
props: {},
snapshot(target) {
const cmpChild = target.querySelector('x-child');
const cmpChildDiv = target.querySelector('x-child div');
const [cmpScopedOuter, cmpScopedInner] = target.querySelectorAll('x-scoped');

return {
target,
cmpScopedOuter,
cmpScopedInner,
cmpChild,
cmpChildDiv,
};
},
advancedTest(target, { Component, hydrateComponent, consoleSpy, container, selector }) {
const snapshotBeforeHydration = this.snapshot(target);
hydrateComponent(target, Component, this.props);
const hydratedTarget = container.querySelector(selector);
const snapshotAfterHydration = this.snapshot(hydratedTarget);

for (const snapshotKey of Object.keys(snapshotBeforeHydration)) {
expect(snapshotBeforeHydration[snapshotKey])
.withContext(
`${snapshotKey} should be the same DOM element both before and after hydration`
)
.toBe(snapshotAfterHydration[snapshotKey]);
}

for (const snapshotKey of ['target', 'cmpScopedOuter', 'cmpScopedInner']) {
expect(snapshotBeforeHydration[snapshotKey].childNodes)
.withContext(
`${snapshotKey} should have the same number of child nodes before & after hydration`
)
.toHaveSize(snapshotAfterHydration[snapshotKey].childNodes.length);
}

expect(consoleSpy.calls.warn).toHaveSize(0);
expect(consoleSpy.calls.error).toHaveSize(0);
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template lwc:render-mode="light">
<div lwc:ref="childdiv">&lt;x-child /&gt;</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement } from 'lwc';

export default class Child extends LightningElement {
static renderMode = 'light';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template lwc:render-mode="light">
<div>&lt;x-main&gt;</div>
<x-scoped instance="1">
<template lwc:slot-data="item1">
<x-scoped instance="2">
<template lwc:slot-data="item2">
<span>main: {item1.msg} {item2.msg}</span>
<x-child></x-child>
</template>
</x-scoped>
</template>
</x-scoped>
<div>&lt;/x-main&gt;</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement } from 'lwc';

export default class Main extends LightningElement {
static renderMode = 'light';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<template lwc:render-mode="light">
<div lwc:ref="div1">&lt;x-scoped&gt; ({instance})</div>
<slot lwc:slot-bind={scopedItem}></slot>
<div lwc:ref="div2">&lt;/x-scoped&gt; ({instance})</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { LightningElement, api } from 'lwc';

export default class scoped extends LightningElement {
static renderMode = 'light';
@api instance = 'unknown';

get scopedItem() {
return {
msg: `from-x-scoped-${this.instance}`,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import ReflectCamel from 'x/reflectCamel';
import WithChildElmsHasSlot from 'x/withChildElmsHasSlot';
import WithChildElmsHasSlotLight from 'x/withChildElmsHasSlotLight';

const vFragBookend = process.env.API_VERSION > 59 ? '<!---->' : '';
divmain marked this conversation as resolved.
Show resolved Hide resolved

it('should throw when trying to claim abstract LightningElement as custom element', () => {
expect(() => LightningElement.CustomElementConstructor).toThrowError(
TypeError,
Expand Down Expand Up @@ -127,7 +129,7 @@ describe('non-empty custom element', () => {
expectWarnings([]);
}

expect(elm.innerHTML).toBe('');
expect(elm.innerHTML).toBe(`${vFragBookend}${vFragBookend}`);
});

it('should log error if slotted synthetic shadow dom custom element has children', () => {
Expand Down
Loading