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

refactor: fix forwarded light DOM slots mapping #3883

Merged
merged 31 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
9ced343
refactor: store slot assignment separately from attributes
jmsjtu Nov 22, 2023
7d5e9e2
test: update tests for template compiler
jmsjtu Nov 22, 2023
e2985f4
test: initial intigration-karma tests
jmsjtu Nov 22, 2023
45c0856
chore: setup playground
jmsjtu Nov 22, 2023
d49d919
test: add tests for slot forwarding
jmsjtu Nov 28, 2023
dc49d3f
test: remove comments
jmsjtu Nov 28, 2023
364e821
test: add scoped slots tests
jmsjtu Nov 28, 2023
9ce9de6
chore: update comments
jmsjtu Nov 28, 2023
5ee4651
chore: update comment
jmsjtu Nov 28, 2023
09abbb6
chore: revert playground changes
jmsjtu Nov 28, 2023
dce30db
chore: fix test failures
jmsjtu Nov 29, 2023
2296e1a
Merge branch 'jtu/lightdom-slots-mapping-playground' into jtu/lightdo…
jmsjtu Nov 29, 2023
22e5bfd
Revert "chore: fix test failures"
jmsjtu Nov 29, 2023
0389de7
chore: fix test cases
jmsjtu Nov 29, 2023
036cdb7
Merge branch 'jtu/lightdom-slots-mapping-playground' into jtu/lightdo…
jmsjtu Nov 29, 2023
5723bad
chore: fix tests
jmsjtu Nov 29, 2023
404eaaa
Update packages/@lwc/engine-core/src/framework/api.ts
jmsjtu Dec 5, 2023
b68b82e
chore: update comments based on PR feedback
jmsjtu Dec 5, 2023
e7f9d80
chore: add todo for scoped slots
jmsjtu Dec 5, 2023
cfdcf8f
chore: add test case for shadow to shadow
jmsjtu Dec 6, 2023
6896343
chore: add tests for if:true
jmsjtu Dec 6, 2023
90016b7
chore: update template compiler tests
jmsjtu Dec 6, 2023
4f00ceb
Merge branch 'master' of github.com:salesforce/lwc into jtu/lightdom-…
jmsjtu Dec 12, 2023
ca69cc2
chore: hoist slot assignment variable
jmsjtu Dec 12, 2023
6849290
Merge branch 'master' of github.com:salesforce/lwc into jtu/lightdom-…
jmsjtu Dec 13, 2023
7ed00f9
refactor: add api versioning to light dom slot forwarding (#3906)
jmsjtu Dec 15, 2023
576e04d
chore: fix karma test sceneario
jmsjtu Dec 15, 2023
e641944
chore: convert var to const in test-utils
jmsjtu Dec 15, 2023
98eacd2
Merge remote-tracking branch 'origin/master' into jtu/lightdom-slots-…
nolanlawson Jan 4, 2024
ffc696b
Merge branch 'master' of github.com:salesforce/lwc into jtu/lightdom-…
jmsjtu Jan 17, 2024
bacd4b0
Merge branch 'master' of github.com:salesforce/lwc into jtu/lightdom-…
jmsjtu Jan 17, 2024
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
35 changes: 28 additions & 7 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ import {
VText,
VStaticPart,
VStaticPartData,
isVBaseElement,
isVStatic,
} from './vnodes';
import { getComponentRegisteredName } from './component';

Expand Down Expand Up @@ -92,6 +94,7 @@ function st(fragment: Element, key: Key, parts?: VStaticPart[]): VStatic {
fragment,
owner,
parts,
slotAssignment: undefined,
};

return vnode;
Expand Down Expand Up @@ -160,7 +163,7 @@ function h(sel: string, data: VElementData, children: VNodes = EmptyArray): VEle
});
}

const { key } = data;
const { key, slotAssignment } = data;

const vnode: VElement = {
type: VNodeType.Element,
Expand All @@ -170,6 +173,7 @@ function h(sel: string, data: VElementData, children: VNodes = EmptyArray): VEle
elm: undefined,
key,
owner: vmBeingRendered,
slotAssignment,
};

return vnode;
Expand Down Expand Up @@ -207,6 +211,10 @@ function s(
assert.isTrue(isObject(data), `s() 2nd argument data must be an object.`);
assert.isTrue(isArray(children), `h() 3rd argument children must be an array.`);
}

const vmBeingRendered = getVMBeingRendered()!;
const { renderMode } = vmBeingRendered;

if (
!isUndefined(slotset) &&
!isUndefined(slotset.slotAssignments) &&
Expand Down Expand Up @@ -236,7 +244,6 @@ function s(
}
// If the passed slot content is factory, evaluate it and add the produced vnodes
if (assignedNodeIsScopedSlot) {
const vmBeingRenderedInception = getVMBeingRendered();
nolanlawson marked this conversation as resolved.
Show resolved Hide resolved
// Evaluate in the scope of the slot content's owner
// if a slotset is provided, there will always be an owner. The only case where owner is
// undefined is for root components, but root components cannot accept slotted content
Expand All @@ -249,19 +256,32 @@ function s(
ArrayPush.call(newChildren, vnode.factory(data.slotData, data.key));
});
} finally {
setVMBeingRendered(vmBeingRenderedInception);
setVMBeingRendered(vmBeingRendered);
}
} else {
// This block is for standard slots (non-scoped slots)
let clonedVNode;
if (
renderMode === RenderMode.Light &&
(isVBaseElement(vnode) || isVStatic(vnode)) &&
vnode.slotAssignment !== data.slotAssignment
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
) {
// When the light DOM slot assignment (slot attribute) changes we can't use the same reference
// to the vnode because the current way the diffing algo works, it will replace the original reference
// to the host element with a new one. This means the new element will be mounted and immediately unmounted.
// Creating a copy of the vnode to preserve a reference to the previous host element.
clonedVNode = { ...vnode };
clonedVNode.slotAssignment = data.slotAssignment;
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
}
// If the slot content is standard type, the content is static, no additional
// processing needed on the vnode
ArrayPush.call(newChildren, vnode);
ArrayPush.call(newChildren, clonedVNode ?? vnode);
}
}
}
children = newChildren;
}
const vmBeingRendered = getVMBeingRendered()!;
const { renderMode, shadowMode, apiVersion } = vmBeingRendered;
const { shadowMode, apiVersion } = vmBeingRendered;

if (renderMode === RenderMode.Light) {
// light DOM slots - backwards-compatible behavior uses flattening, new behavior uses fragments
Expand Down Expand Up @@ -324,7 +344,7 @@ function c(
});
}
}
const { key } = data;
const { key, slotAssignment } = data;
let elm, aChildren, vm;
const vnode: VCustomElement = {
type: VNodeType.CustomElement,
Expand All @@ -333,6 +353,7 @@ function c(
children,
elm,
key,
slotAssignment,

ctor: Ctor,
owner: vmBeingRendered,
Expand Down
21 changes: 20 additions & 1 deletion packages/@lwc/engine-core/src/framework/modules/attrs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
import { RendererAPI } from '../renderer';

import { EmptyObject } from '../utils';
import { VBaseElement } from '../vnodes';
import { VBaseElement, VStatic } from '../vnodes';

const ColonCharCode = 58;

Expand Down Expand Up @@ -63,3 +63,22 @@ export function patchAttributes(
}
}
}

export function patchSlotAssignment(
oldVnode: VBaseElement | VStatic | null,
vnode: VBaseElement | VStatic,
renderer: RendererAPI
) {
if (oldVnode?.slotAssignment === vnode.slotAssignment) {
return;
}

const { elm } = vnode;
const { setAttribute, removeAttribute } = renderer;

if (isUndefined(vnode.slotAssignment) || isNull(vnode.slotAssignment)) {
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
removeAttribute(elm, 'slot');
} else {
setAttribute(elm, 'slot', vnode.slotAssignment);
}
}
10 changes: 7 additions & 3 deletions packages/@lwc/engine-core/src/framework/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
isVCustomElement,
isVFragment,
isVScopedSlotFragment,
isVStatic,
Key,
VBaseElement,
VComment,
Expand All @@ -60,7 +61,7 @@ import {
VText,
} from './vnodes';

import { patchAttributes } from './modules/attrs';
import { patchAttributes, patchSlotAssignment } from './modules/attrs';
import { patchProps } from './modules/props';
import { patchClassAttribute } from './modules/computed-class-attr';
import { patchStyleAttribute } from './modules/computed-style-attr';
Expand Down Expand Up @@ -265,6 +266,7 @@ function mountElement(
function patchStatic(n1: VStatic, n2: VStatic, renderer: RendererAPI) {
const elm = (n2.elm = n1.elm!);

patchSlotAssignment(n1, n2, renderer);
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
// The `refs` object is blown away in every re-render, so we always need to re-apply them
applyStaticParts(elm, n2, renderer, false);
}
Expand Down Expand Up @@ -298,6 +300,7 @@ function mountStatic(
}
}

patchSlotAssignment(null, vnode, renderer);
insertNode(elm, parent, anchor, renderer);
applyStaticParts(elm, vnode, renderer, true);
}
Expand Down Expand Up @@ -597,6 +600,7 @@ function patchElementPropsAndAttrsAndRefs(

patchAttributes(oldVnode, vnode, renderer);
patchProps(oldVnode, vnode, renderer);
patchSlotAssignment(oldVnode, vnode, renderer);

// The `refs` object is blown away in every re-render, so we always need to re-apply them
applyRefs(vnode, vnode.owner);
Expand Down Expand Up @@ -795,8 +799,8 @@ function allocateInSlot(vm: VM, children: VNodes, owner: VM) {
}

let slotName: unknown = '';
if (isVBaseElement(vnode)) {
slotName = vnode.data.attrs?.slot ?? '';
if (isVBaseElement(vnode) || isVStatic(vnode)) {
slotName = vnode.slotAssignment ?? '';
} else if (isVScopedSlotFragment(vnode)) {
slotName = vnode.slotName;
}
Expand Down
8 changes: 8 additions & 0 deletions packages/@lwc/engine-core/src/framework/vnodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export interface VStatic extends BaseVNode {
readonly fragment: Element;
readonly parts: VStaticPart[] | undefined;
elm: Element | undefined;
slotAssignment: string | undefined;
}

export interface VFragment extends BaseVNode, BaseVParent {
Expand Down Expand Up @@ -99,6 +100,7 @@ export interface VBaseElement extends BaseVNode, BaseVParent {
data: VElementData;
elm: Element | undefined;
key: Key;
slotAssignment: string | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to move slot assignment outside the props in ElementData because static vnodes can also be reassigned during light DOM slot forwarding.

For example:

<x-slot-cmp>
    <div>This is a static element</div>
</x-slot-cmp>
<template>
    <slot slot="foo"></slot>
</template>
// x-slot-cmp

In the above example, the div, which is a static element needs to be forwarded to thefoo slot. This can't happen through the props because static vnodes don't update attributes from props.

}

export interface VElement extends VBaseElement {
Expand Down Expand Up @@ -134,6 +136,8 @@ export interface VElementData extends VNodeData {
readonly external?: boolean;
readonly ref?: string;
readonly slotData?: any;
// Corresponds to the slot attribute of the element and indicates which `slot` element it should be assigned to
readonly slotAssignment?: string;
}

export function isVBaseElement(vnode: VNode): vnode is VElement | VCustomElement {
Expand All @@ -156,3 +160,7 @@ export function isVFragment(vnode: VNode): vnode is VFragment {
export function isVScopedSlotFragment(vnode: VNode): vnode is VScopedSlotFragment {
return vnode.type === VNodeType.ScopedSlotFragment;
}

export function isVStatic(vnode: VNode): vnode is VStatic {
return vnode.type === VNodeType.Static;
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ export default function (define) {
'ui-something',
_uiSomething__default['default'],
{
attrs: {
slot: 'first',
},
slotAssignment: 'first',
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to add an ACT compiler change for this

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be an issue if we decide to use API versioning because of the mechanics of how ACT compiler + API versioning works.

Try publishing an alpha version of LWC with these and run the jest tests on core with this change.

props: {
text: 'Hello',
},
Expand All @@ -41,9 +39,7 @@ export default function (define) {
'ui-another',
_uiAnother__default['default'],
{
attrs: {
slot: 'second',
},
slotAssignment: 'second',
props: {
value: 'Foo',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ export default function (define) {
'ui-another',
_uiAnother__default['default'],
{
attrs: {
slot: 'adjacent',
},
slotAssignment: 'adjacent',
props: {
value: 'Foo',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ export default function (define) {
'ui-something',
_uiSomething__default['default'],
{
attrs: {
slot: 'first',
},
slotAssignment: 'first',
props: {
text: 'Hello',
},
Expand All @@ -40,9 +38,7 @@ export default function (define) {
'ui-another',
_uiAnother__default['default'],
{
attrs: {
slot: 'second',
},
slotAssignment: 'second',
props: {
value: 'Foo',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('scoped slots', () => {
// For standard slot content, "slot" attribute goes directly on the element unlike scoped
// slots where the attribute goes on the template tag
expect(child.querySelector('.slotname3').innerHTML).toBe(
`${vFragBookend}<p slot="slotname3">MLB</p>${vFragBookend}`
`${vFragBookend}<p>MLB</p>${vFragBookend}`
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
);
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { createElement } from 'lwc';

import LightContainer from './x/lightContainer/lightContainer';

describe('scoped slots, slot forwarding', () => {
let lightContainer;
beforeAll(() => {
lightContainer = createElement('x-light-container', { is: LightContainer });
document.body.appendChild(lightContainer);
});

afterAll(() => {
document.body.removeChild(lightContainer);
});

it('does not reassign slot content', () => {
const leaf = lightContainer.querySelector('x-leaf');
expect(leaf.shadowRoot.children.length).toEqual(2);

const defaultSlot = leaf.shadowRoot.querySelector('slot');
const defaultSlotContent = defaultSlot.assignedNodes();
expect(defaultSlotContent.length).toEqual(1);
expect(defaultSlotContent[0].innerText).toEqual('Hello world!');

const fooSlot = leaf.shadowRoot.querySelector('[name="foo"]');
const fooSlotContent = fooSlot.assignedNodes();
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
// Note the slot reassignment does not work on scoped slots
expect(fooSlotContent.length).toEqual(0);
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<template>
<slot></slot>
<slot name="foo"></slot>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { LightningElement } from 'lwc';

export default class extends LightningElement {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template lwc:render-mode="light">
<x-scoped-slot-parent></x-scoped-slot-parent>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement } from 'lwc';

export default class extends LightningElement {
static renderMode = 'light';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<template lwc:render-mode="light">
<x-leaf>
<!-- Note the slot mapping does not work for scoped slots -->
<slot lwc:slot-bind={title} slot="foo"></slot>
</x-leaf>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class extends LightningElement {
static renderMode = 'light';

title = 'Hello world!';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<template lwc:render-mode="light">
<x-scoped-slot-child>
<template lwc:slot-data="title">
<h2>{title}</h2>
</template>
</x-scoped-slot-child>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement } from 'lwc';

export default class extends LightningElement {
static renderMode = 'light';
}
Loading