chore: use comment nodes for VFragment bookends (#3846)

* chore: add failing test for super-nested slots with static fragments + Light DOM + SSR hydration * chore: use comment nodes for VFragment bookends * test: adjacent text-node expressions of length zero * chore: update integration karma tests * chore: restore previous behavior in versions <= 59 * chore: use APIFeature.USE_COMMENTS_FOR_FRAGMENT_BOOKENDS instead of checking version directly * chore: use HIGHEST_API_VERSION from @lwc/shared in karma test options * chore: bump ci
salesforce · Nov 8, 2023 · d900aca · d900aca
1 parent 855661b
commit d900aca
Show file tree

Hide file tree

Showing 24 changed files with 243 additions and 39 deletions.
diff --git a/packages/@lwc/engine-core/src/framework/api.ts b/packages/@lwc/engine-core/src/framework/api.ts
@@ -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('');
+
     return {
         type: VNodeType.Fragment,
         sel: undefined,
         key,
         elm: undefined,
         children: [leading, ...children, trailing],
         stable,
-        owner: getVMBeingRendered()!,
+        owner,
         leading,
         trailing,
     };

diff --git a/packages/@lwc/engine-core/src/framework/hydration.ts b/packages/@lwc/engine-core/src/framework/hydration.ts
@@ -17,6 +17,8 @@ import {
     isTrue,
     isString,
     StringToLowerCase,
+    APIFeature,
+    isAPIFeatureEnabled,
 } from '@lwc/shared';
 
 import { logError, logWarn } from '../shared/logger';
@@ -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);
 }
 
@@ -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);
 }
@@ -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;
@@ -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);
@@ -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
 ) {
     let hasWarned = false;
     let nextNode: Node | null = node;
@@ -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.
+        //
+        // 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) {

diff --git a/packages/@lwc/engine-core/src/framework/vnodes.ts b/packages/@lwc/engine-core/src/framework/vnodes.ts
@@ -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 {

diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/basic/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/basic/expected.html
@@ -2,11 +2,13 @@
   <template shadowroot="open">
     <x-basic-child>
       <div>
-        ‍‍
+        <!---->
+        <!---->
         <span>
           99 - ssr
         </span>
-        ‍‍
+        <!---->
+        <!---->
       </div>
     </x-basic-child>
   </template>

diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/for-each/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/for-each/expected.html
@@ -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>

diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/named-slot/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/named-slot/expected.html
@@ -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>

diff --git a/packages/@lwc/integration-karma/helpers/test-hydrate.js b/packages/@lwc/integration-karma/helpers/test-hydrate.js
@@ -67,6 +67,8 @@ window.HydrateTest = (function (lwc, testUtils) {
                 Component,
                 hydrateComponent: lwc.hydrateComponent.bind(lwc),
                 consoleSpy,
+                container,
+                selector,
             });
         }
 

diff --git a/packages/@lwc/integration-karma/scripts/shared/options.js b/packages/@lwc/integration-karma/scripts/shared/options.js
@@ -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!');
@@ -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

diff --git a/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/index.spec.js b/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/index.spec.js
@@ -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);
+    },
+};
diff --git a/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/x/main/main.html b/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/x/main/main.html
@@ -0,0 +1,4 @@
+<template>
+    <p class="first">{zeroLengthText}‍{zeroLengthText}‍</p>
+    <p class="second">{zeroLengthText}{zeroLengthText}{zeroLengthText}{zeroLengthText}</p>
+</template>
diff --git a/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/x/main/main.js b/packages/@lwc/integration-karma/test-hydration/adjacent-text-nodes/x/main/main.js
@@ -0,0 +1,5 @@
+import { LightningElement } from 'lwc';
+
+export default class Main extends LightningElement {
+    zeroLengthText = '';
+}
diff --git a/...ges/@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/index.spec.js b/...ges/@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/index.spec.js
@@ -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);
+    },
+};
diff --git a/...lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/child/child.html b/...lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/child/child.html
@@ -0,0 +1,3 @@
+<template lwc:render-mode="light">
+    <div lwc:ref="childdiv">&lt;x-child /&gt;</div>
+</template>
diff --git a/.../@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/child/child.js b/.../@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/child/child.js
@@ -0,0 +1,5 @@
+import { LightningElement } from 'lwc';
+
+export default class Child extends LightningElement {
+    static renderMode = 'light';
+}
diff --git a/.../@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/main/main.html b/.../@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/main/main.html
@@ -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>
diff --git a/...es/@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/main/main.js b/...es/@lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/main/main.js
@@ -0,0 +1,5 @@
+import { LightningElement } from 'lwc';
+
+export default class Main extends LightningElement {
+    static renderMode = 'light';
+}
diff --git a/...c/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/scoped/scoped.html b/...c/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/scoped/scoped.html
@@ -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>
diff --git a/...lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/scoped/scoped.js b/...lwc/integration-karma/test-hydration/light-dom/slots/nested-and-scoped/x/scoped/scoped.js
@@ -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}`,
+        };
+    }
+}
diff --git a/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js b/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js
@@ -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 ? '<!---->' : '';
+
 it('should throw when trying to claim abstract LightningElement as custom element', () => {
     expect(() => LightningElement.CustomElementConstructor).toThrowError(
         TypeError,
@@ -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', () => {