fix forwarded dangling slots #5164
Conversation
| // This will get overridden but requires initialization. | ||
| const slotAttributeValue = null; |
There was a problem hiding this comment.
How does it get overridden if it's const?
| @@ -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. | |||
There was a problem hiding this comment.
Can you clarify what "dangling" means here?
There was a problem hiding this comment.
| @@ -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. | |||
There was a problem hiding this comment.
Can you clarify what "dangling" means here?
There was a problem hiding this comment.
A slot that is referenced but does not exist (see example fixture on the line below)
There was a problem hiding this comment.
Add that to the code comment 😉
There was a problem hiding this comment.
| 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. |
There was a problem hiding this comment.
Thanks, much clearer
| @@ -263,6 +275,7 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu | |||
|
|
|||
| return [ | |||
| bYield(b.literal(`<${node.name}`)), | |||
| ...[bConditionallyYieldDanglingSlotName()], | |||
There was a problem hiding this comment.
| ...[bConditionallyYieldDanglingSlotName()], | |
| bConditionallyYieldDanglingSlotName(), |
Creating and then spreading an array with one element is unnecessary.
There was a problem hiding this comment.
Good catch, thx
| const bConditionallyYieldDanglingSlotName = esTemplateWithYield` | ||
| { | ||
| if (slotAttributeValue) { | ||
| yield \` slot="\${slotAttributeValue}"\`; | ||
| } | ||
| } | ||
| `<EsBlockStatement>; |
There was a problem hiding this comment.
| const bConditionallyYieldDanglingSlotName = esTemplateWithYield` | |
| { | |
| if (slotAttributeValue) { | |
| yield \` slot="\${slotAttributeValue}"\`; | |
| } | |
| } | |
| `<EsBlockStatement>; | |
| const bConditionallyYieldDanglingSlotName = esTemplateWithYield` | |
| if (slotAttributeValue) { | |
| yield \` slot="\${slotAttributeValue}"\`; | |
| } | |
| `<EsIfStatement>; |
We don't need to use a block statement wrapper here, because there's no new variables being declared.
There was a problem hiding this comment.
Good catch, copy paste error
| @@ -32,6 +32,9 @@ const bExportTemplate = esTemplate` | |||
| let textContentBuffer = ''; | |||
| let didBufferTextContent = false; | |||
|
|
|||
| // This will get overridden but requires initialization. | |||
There was a problem hiding this comment.
| // 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?
Details
Does this pull request introduce an observable change?
GUS work item
W-17587763