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 forwarded dangling slots #5164

Merged
merged 6 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<x-dangling-container>
<x-slot>
<x-leaf>
<!---->
<x-component slot="dangling">
Component content
</x-component>
<!---->
<!---->
<x-component>
Component content
</x-component>
<!---->
</x-leaf>
</x-slot>
</x-dangling-container>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const tagName = 'x-dangling-container';
export { default } from 'x/container';
export * from 'x/container';
export const features = [];
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template lwc:render-mode="light">
Component content
</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-slot>
<x-component slot="top"></x-component>
<x-component slot="bottom"></x-component>
</x-slot>
</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,4 @@
<template lwc:render-mode="light">
<slot name="leafTop" slot="dangling"></slot>
<slot name="leafBottom"></slot>
</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>
<slot name="top" slot="leafTop"></slot>
<slot name="bottom" slot="leafBottom"></slot>
</x-leaf>
</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
Expand Up @@ -2,14 +2,14 @@
<x-slot>
<x-leaf>
<!---->
<h1 slot="bottom">
bottom content
<h1 slot="dangling">
top content
</h1>
<!---->
<!---->
<h1>
top content
</h1>
<h2>
bottom content
</h2>
<!---->
</x-leaf>
</x-slot>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template lwc:render-mode="light">
<x-slot>
<h1 slot="top">top content</h1>
<h1 slot="bottom">bottom content</h1>
<h2 slot="bottom">bottom content</h2>
</x-slot>
</template>
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<template lwc:render-mode="light">
<slot name="top" slot="bottom"></slot>
<slot name="bottom"></slot>
<slot name="leafTop" slot="dangling"></slot>
<slot name="leafBottom"></slot>
</template>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template lwc:render-mode="light">
<x-leaf>
<slot name="top" slot="bottom"></slot>
<slot name="bottom" slot="top"></slot>
<slot name="top" slot="leafTop"></slot>
<slot name="bottom" slot="leafBottom"></slot>
</x-leaf>
</template>
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ export const expectedFailures = new Set([
'exports/component-as-default/index.js',
'known-boolean-attributes/default-def-html-attributes/static-on-component/index.js',
'render-dynamic-value/index.js',
'slot-forwarding/slots/mixed/index.js',
'slot-forwarding/slots/dangling/index.js',
'wire/errors/throws-on-computed-key/index.js',
'wire/errors/throws-when-colliding-prop-then-method/index.js',
]);
3 changes: 3 additions & 0 deletions packages/@lwc/ssr-compiler/src/compile-template/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ const bExportTemplate = esTemplate`
let textContentBuffer = '';
let didBufferTextContent = false;

// This will get overridden but requires initialization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This will get overridden but requires initialization.
// This gets shadowed in slotted content generators, but needs to be initialized for top-level slots

Is that right?

const slotAttributeValue = null;
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

How does it get overridden if it's const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ }


// Establishes a contextual relationship between two components for ContextProviders.
// This variable will typically get overridden (shadowed) within slotted content.
const contextfulParent = instance;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ const bYieldFromChildGenerator = esTemplateWithYield`
{
const childProps = ${/* child props */ is.objectExpression};
const childAttrs = ${/* child attrs */ is.objectExpression};
/*
If a slotAttributeValue is present, it is dangling and should be assigned to any slotted content. This behavior aligns with v1 and engine-dom.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what "dangling" means here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: engine-server/src/__tests__/fixtures/slot-forwarding/slots/dangling/ for example case.
*/
if (slotAttributeValue) {
childAttrs.slot = slotAttributeValue;
}
${
/*
Slotted content is inserted here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ const bYieldFromDynamicComponentConstructorGenerator = esTemplateWithYield`
}
const childProps = ${/* child props */ is.objectExpression};
const childAttrs = ${/* child attrs */ is.objectExpression};
/*
If a slotAttributeValue is present, it is dangling and should be assigned to any slotted content. This behavior aligns with v1 and engine-dom.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what "dangling" means here?

Copy link
Contributor Author

@jhefferman-sfdc jhefferman-sfdc Jan 24, 2025

Choose a reason for hiding this comment

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

A slot that is referenced but does not exist (see example fixture on the line below)

Copy link
Contributor

Choose a reason for hiding this comment

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

Add that to the code comment 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If a slotAttributeValue is present, it is dangling and should be assigned to any slotted content. This behavior aligns with v1 and engine-dom.
If `slotAttributeValue` is set, it references a slot that does not exist, and the `slot` attribute should be set in the DOM. This behavior aligns with engine-server and engine-dom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, much clearer

See: engine-server/src/__tests__/fixtures/slot-forwarding/slots/dangling/ for example case.
*/
if (slotAttributeValue) {
childAttrs.slot = slotAttributeValue;
}
${
/*
Slotted content is inserted here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ const bGenerateSlottedContent = esTemplateWithYield`
const bAddSlottedContent = esTemplate`
addSlottedContent(${/* slot name */ is.expression} ?? "", async function* generateSlottedContent(contextfulParent, ${
/* scoped slot data variable */ isNullableOf(is.identifier)
}) {
}, slotAttributeValue) {
// FIXME: make validation work again
${/* slot content */ false}
}, ${/* content map */ is.identifier});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,16 @@ const bConditionallyYieldScopeTokenClass = esTemplateWithYield`
}
`<EsIfStatement>;

/*
If a slotAttributeValue is present, it is dangling and should be assigned to any slotted content. This behavior aligns with v1 and engine-dom.
See: engine-server/src/__tests__/fixtures/slot-forwarding/slots/dangling/ for example case.
*/
const bConditionallyYieldDanglingSlotName = esTemplateWithYield`
if (slotAttributeValue) {
yield \` slot="\${slotAttributeValue}"\`;
}
`<EsBlockStatement>;

const bYieldSanitizedHtml = esTemplateWithYield`
yield sanitizeHtmlContent(${/* lwc:inner-html content */ is.expression})
`;
Expand Down Expand Up @@ -263,6 +273,7 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu

return [
bYield(b.literal(`<${node.name}`)),
bConditionallyYieldDanglingSlotName(),
// If we haven't already prefixed the scope token to an existing class, add an explicit class here
...(hasClassAttribute ? [] : [bConditionallyYieldScopeTokenClass()]),
...yieldAttrsAndProps,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ const bConditionalSlot = esTemplateWithYield`
const scopedGenerators = scopedSlottedContent?.[slotName ?? ""];
const mismatchedSlots = isScopedSlot ? lightGenerators : scopedGenerators;
const generators = isScopedSlot ? scopedGenerators : lightGenerators;

/*
If a slotAttributeValue is present, it should be provided for assignment to any slotted content. This behavior aligns with v1 and engine-dom.
See: engine-server/src/__tests__/fixtures/slot-forwarding/slots/dangling/ for example.
Note the slot mapping does not work for scoped slots, so the slot name is not rendered in this case.
See: engine-server/src/__tests__/fixtures/slot-forwarding/scoped-slots for example.
*/
const danglingSlotName = !isScopedSlot ? ${/* slotAttributeValue */ is.expression} || slotAttributeValue : null;
// start bookend HTML comment for light DOM slot vfragment
if (!isSlotted) {
yield '<!---->';
Expand All @@ -43,7 +49,7 @@ const bConditionalSlot = esTemplateWithYield`

if (generators) {
for (let i = 0; i < generators.length; i++) {
yield* generators[i](contextfulParent, ${/* scoped slot data */ isNullableOf(is.expression)});
yield* generators[i](contextfulParent, ${/* scoped slot data */ isNullableOf(is.expression)}, danglingSlotName);
// Scoped slotted data is separated by bookends. Final bookends are added outside of the loop below.
if (isScopedSlot && i < generators.length - 1) {
yield '<!---->';
Expand Down Expand Up @@ -90,5 +96,16 @@ export const Slot: Transformer<IrSlot> = function Slot(node, ctx): EsStatement[]
const slotChildren = irChildrenToEs(node.children, ctx);
const isScopedSlot = b.literal(Boolean(slotBound));
const isSlotted = b.literal(Boolean(ctx.isSlotted));
return [bConditionalSlot(isScopedSlot, isSlotted, slotName, slotBound, slotChildren, slotAst)];
const slotAttributeValue = bAttributeValue(node, 'slot');
return [
bConditionalSlot(
isScopedSlot,
isSlotted,
slotName,
slotAttributeValue,
slotBound,
slotChildren,
slotAst
),
];
};