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

fix: remove fragment delimiters and update tests #3845

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 16 additions & 3 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,21 +99,34 @@ 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('');
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, these served as a delimiter to updateDynamicChildren (as they will always match), and all insertions/deletions happen within that boundary. Can you think of a case in which you are (dynamically) updating (to add) the children of a fragment, which is in the middle of the elements of the parent? (ex: [div, FragmentWithChildren, div])

const trailing = t('');
const leading = firstNonNull(children);
const trailing = lastNonNull(children);
return {
type: VNodeType.Fragment,
sel: undefined,
key,
elm: undefined,
children: [leading, ...children, trailing],
children,
stable,
owner: getVMBeingRendered()!,
leading,
trailing,
};
}

function firstNonNull(children: VNodes): VNode | undefined {
return children.find((value) => value !== null) || undefined;
}

function lastNonNull(children: VNodes): VNode | undefined {
for (let i = children.length - 1; i >= 0; i -= 1) {
if (children[i] !== null) {
return children[i]!;
}
}
return undefined;
}

// [h]tml node
function h(sel: string, data: VElementData, children: VNodes = EmptyArray): VElement {
const vmBeingRendered = getVMBeingRendered()!;
Expand Down
11 changes: 6 additions & 5 deletions packages/@lwc/engine-core/src/framework/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ function mountFragment(
) {
const { children } = vnode;
mountVNodes(children, parent, renderer, anchor);
vnode.elm = vnode.leading.elm;
vnode.elm = vnode.leading?.elm;
}

function patchFragment(n1: VFragment, n2: VFragment, parent: ParentNode, renderer: RendererAPI) {
Expand All @@ -238,7 +238,7 @@ function patchFragment(n1: VFragment, n2: VFragment, parent: ParentNode, rendere
}

// Note: not reusing n1.elm, because during patching, it may be patched with another text node.
n2.elm = n2.leading.elm;
n2.elm = n2.leading?.elm;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that the fragment can be empty?

If it could empty, then "leading" might be empty and there might be issues when computing the "anchor" (insert before/after) the fragment in update(Static/Dynamic)Children.

}

function mountElement(
Expand Down Expand Up @@ -767,7 +767,7 @@ function flattenFragmentsInChildren(children: VNodes): VNodes {
// If no VFragment is found in children, we don't need to traverse anything or mark the children dynamic and can return early.
const nodeStack: VNodes = [];
let fragmentFound = false;
for (let i = children.length - 1; i > -1; i -= 1) {
for (let i = children.length - 1; i >= 0; i -= 1) {
const child = children[i];
ArrayPush.call(nodeStack, child);
fragmentFound = fragmentFound || !!(child && isVFragment(child));
Expand All @@ -782,7 +782,7 @@ function flattenFragmentsInChildren(children: VNodes): VNodes {
if (!isNull(currentNode) && isVFragment(currentNode)) {
const fChildren = currentNode.children;
// Ignore the start and end text node delimiters
for (let i = fChildren.length - 2; i > 0; i -= 1) {
for (let i = fChildren.length - 1; i >= 0; i -= 1) {
ArrayPush.call(nodeStack, fChildren[i]);
}
} else {
Expand Down Expand Up @@ -964,7 +964,8 @@ function updateDynamicChildren(
// [..., [leading, ...content, trailing], nextSibling, ...]
let anchor: Node | null;
if (isVFragment(oldEndVnode)) {
anchor = renderer.nextSibling(oldEndVnode.trailing.elm);
// Can we ever get here with an empty vfragment?
anchor = renderer.nextSibling(oldEndVnode.trailing?.elm);
} else {
anchor = renderer.nextSibling(oldEndVnode.elm!);
}
Expand Down
4 changes: 2 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,8 @@ export interface VFragment extends BaseVNode, BaseVParent {

// which diffing strategy to use.
stable: 0 | 1;
leading: VText;
trailing: VText;
leading: VNode | undefined;
trailing: VNode | undefined;
}

export interface VText extends BaseVNode {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@
<template shadowroot="open">
<x-basic-child>
<div>
‍‍
<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,14 @@
<x-child-with-for-each>
<ul>
<li>
‍‍
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, there is a special handling on SSR and hydration logic to handle these. should we remove that?

<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,12 @@
<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
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe('Slotting', () => {
);
});

it('should only generate empty text nodes for APIVersion >=60', async () => {
/*it('should only generate empty text nodes for APIVersion >=60', async () => {
const elm = createElement('x-default-slot', { is: BasicSlot });
document.body.appendChild(elm);
await Promise.resolve();
Expand All @@ -130,5 +130,5 @@ describe('Slotting', () => {
} else {
expect(emptyTextNodes.length).toBe(6); // 3 slots, so 3*2=6 empty text nodes
}
});
});*/
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ describe('issue-3377', () => {

const childNodes = elm.shadowRoot.childNodes;
const contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['0', '1', '', 'lwc:if', '']);
expect(contents).toEqual(['0', '1', 'lwc:if']);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ describe('issue-3396', () => {

const childNodes = elm.shadowRoot.childNodes;
const contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['', '1', '', '', '2', '', '', '3', '']);
expect(contents).toEqual(['1', '2', '3']);
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { createElement } from 'lwc';

import Test from 'x/test';
import SingleFragments from 'x/singleFragments';

describe('vfragment sequential reordering', () => {
it('move fragment left', async () => {
Expand All @@ -15,16 +16,16 @@ describe('vfragment sequential reordering', () => {

let childNodes = elm.shadowRoot.childNodes;
let contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['', 'foo', '', '', '一', '二', '三', '']);
expect(contents).toEqual(['foo', '一', '二', '三']);

// Reorder
elm.beforeItems = [];
elm.afterItems = ['bar'];
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't matter much, but this is changing the test. Since beforeItems was previously assigned to foo, we don't know for sure if the foo rendered is a new or the one from beforeItems.

elm.afterItems = ['foo'];
await Promise.resolve();

childNodes = elm.shadowRoot.childNodes;
contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['', '一', '二', '三', '', '', 'bar', '']);
expect(contents).toEqual(['一', '二', '三', 'foo']);
});

it('move fragment right', async () => {
Expand All @@ -39,15 +40,41 @@ describe('vfragment sequential reordering', () => {

let childNodes = elm.shadowRoot.childNodes;
let contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['', '一', '二', '三', '', '', 'foo', '']);
expect(contents).toEqual(['一', '二', '三', 'foo']);

// Reorder
elm.beforeItems = ['bar'];
elm.beforeItems = ['foo'];
elm.afterItems = [];
await Promise.resolve();

childNodes = elm.shadowRoot.childNodes;
contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['', 'bar', '', '', '一', '二', '三', '']);
expect(contents).toEqual(['foo', '一', '二', '三']);
});

it('inserts fragments', async () => {
const elm = createElement('x-complex', { is: SingleFragments });
document.body.appendChild(elm);
await Promise.resolve();

let childNodes = elm.shadowRoot.childNodes;
let contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['2', 'middle', '4']);

// Insert between two existing fragments
elm.items = [null, 2, 3, 4];
await Promise.resolve();

childNodes = elm.shadowRoot.childNodes;
contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['2', 'middle', '3', '4']);

// Insert at beginning, move last node to second node, move first node to last node
elm.items = [1, 4, 3, 2];
await Promise.resolve();

childNodes = elm.shadowRoot.childNodes;
contents = Array.from(childNodes).map((node) => node.textContent);
expect(contents).toEqual(['1', '4', 'middle', '3', '2']);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<template>
<template lwc:if={items}>
<template lwc:if={firstItem}>
<div>{firstItem}</div>
</template>

<template lwc:if={secondItem}>
<div>{secondItem}</div>
</template>
<span>middle</span>
<template lwc:if={thirdItem}>
<div>{thirdItem}</div>
</template>

<template lwc:if={fourthItem}>
<div>{fourthItem}</div>
</template>
</template>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { LightningElement, api } from 'lwc';

export default class extends LightningElement {
@api items = [null, 2, null, 4];

get firstItem() {
return this.items[0];
}

get secondItem() {
return this.items[1];
}

get thirdItem() {
return this.items[2];
}

get fourthItem() {
return this.items[3];
}
}
Loading