feat(template-compiler): implement handling for scoped slot directives in template #3077

ravijayaramappa · 2022-10-02T23:53:55Z

Details

This PR introduces handling for the two scoped slot directives: lwc:slot-bind and lwc:slot-data.

Added two AST nodes to handle the new directives. Added a ScopedSlotContent to represent the template content provided by the parent component.
Added code gen parts for the ScopedSlotContent to generate a VScopedSlotContent
The feature is guarded by a config which is currently default off.

Follow up PRs:

Handle slotting at runtime
Use VFragment to represent return value of the ScopedSlotFactory

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

GUS work item

W-11469044

salesforce-nucleus · 2022-10-06T21:14:54Z

❌ An unexpected error occurred while attempting to start the test stage of workflow 94079. Please try to start the stage again, or reach out to #nucleus-talk for help.

salesforce-nucleus · 2022-10-06T21:52:18Z

❌ An unexpected error occurred while attempting to start the test stage of workflow 94098. Please try to start the stage again, or reach out to #nucleus-talk for help.

packages/@lwc/engine-core/src/framework/hydration.ts

@@ -246,7 +246,7 @@ function hydrateCustomElement(
    vnode.elm = elm;
    vnode.vm = vm;

-    allocateChildren(vnode, vm);
+    allocateChildren(vnode, vm, owner);


packages/@lwc/errors/src/compiler/error-info/template-transform.ts

@@ -473,33 +475,38 @@ export const ParserDiagnostics = {
        level: DiagnosticLevel.Error,
        url: '',
    },
+


ravijayaramappa · 2022-10-13T20:03:43Z

packages/@lwc/engine-core/src/framework/api.ts

+                        // Evaluate in the scope of the slot content's owner
+                        setVMBeingRendered(slotset.owner);
+                    }
+                    children = vnode.factory(data.slotData);


Working on a separate PR for testing reactivity #3105

packages/@lwc/engine-core/src/framework/rendering.ts

-        if (isVBaseElement(vnode)) {
-            slotName = (vnode.data.attrs?.slot as string) || '';
+        if (isVBaseElement(vnode) && !isUndefined(vnode.data.attrs?.slot)) {
+            slotName = vnode.data.attrs?.slot as string;


nolanlawson

First pass. LGTM but I am not super familiar with the parser and don't feel comfortable +1'ing yet.

packages/@lwc/engine-core/src/framework/api.ts

+                    if (!isUndefined(slotset.owner)) {
+                        // Evaluate in the scope of the slot content's owner
+                        setVMBeingRendered(slotset.owner);
+                    }


packages/@lwc/engine-core/src/framework/api.ts

+                } finally {
+                    setVMBeingRendered(vmBeingRenderedInception);
+                }
+                return acc.concat(children);


packages/@lwc/engine-core/src/framework/api.ts

+                return acc.concat(children);
+            } else {
+                // If the slot content is a static list of child nodes provided by the parent, nothing to do
+                return acc.concat(vnode);


nolanlawson · 2022-10-07T18:09:14Z

packages/@lwc/engine-core/src/framework/vnodes.ts

@@ -35,6 +43,13 @@ export interface BaseVNode {
    owner: VM;
 }

+export interface VScopedSlotContent extends BaseVNode {
+    // TODO [#9999]: should the factory return a VFragment instead?


packages/@lwc/template-compiler/src/codegen/index.ts

+        } = scopedSlotContent;
+        codeGen.beginScope();
+        codeGen.declareIdentifier(dataIdentifier);
+        // TODO [#9999]: Next Step: Return a VFragment


packages/@lwc/template-compiler/src/parser/index.ts

+    parent: ParentNode,
+    parse5Elm: parse5.Element
+): ScopedSlotContent | undefined {
+    const slotDataAttr = parsedAttr.pick('lwc:slot-data');


packages/@lwc/template-compiler/src/parser/index.ts

scripts/bundlesize/bundlesize.config.json

@@ -2,7 +2,7 @@
    "files": [
        {
            "path": "packages/lwc/dist/engine-dom/umd/es2017/engine-dom.min.js",
-            "maxSize": "18.5KB",
+            "maxSize": "18.6KB",


packages/@lwc/errors/src/compiler/error-info/template-transform.ts

@@ -394,7 +395,8 @@ export const ParserDiagnostics = {

    SLOT_TAG_CANNOT_HAVE_DIRECTIVES: {
        code: 1082,
-        message: "Slot tag can't be associated with directives",
+        message:
+            "Slot tag can't be associated with for:each, for:of, if:true, if:flase, lwc:if, lwc:else, lwc:elseif template directives.",


jmsjtu · 2022-10-11T15:18:37Z

packages/@lwc/template-compiler/src/parser/index.ts

@@ -252,7 +265,8 @@ function parseElementDirectives(
    ctx: ParserCtx,
    parsedAttr: ParsedAttribute,
    parent: ParentNode,
-    parse5ElmLocation: parse5.ElementLocation
+    parse5ElmLocation: parse5.ElementLocation,


Since we're passing the entire parse5.Element, we can skip passing the parse5.ElementLocation here and pass the needed values to the parsers.

Maybe something like this?

const { tagName, ElementLocation: { parse5ElmLocation } } = parse5Elm; .... const node = parser(ctx, parsedAttr, parse5ElmLocation, prev, tagName);

There is some processing to obtain parse5ElmLocation in parseElement

lwc/packages/@lwc/template-compiler/src/parser/index.ts

Line 177 in edab2dd

const parse5ElmLocation = parseElementLocation(ctx, parse5Elm, parse5ParentLocation);

const parse5ElmLocation = parseElementLocation(ctx, parse5Elm, parse5ParentLocation);

This location is used in multiple places in parseElement. Passing the parse5ElmLocation to parseElementDirectives helps reuse.

I am also on the side of @jmsjtu. I had to get back into the code to better understand why we don't access the elementLocation property directly on the element.

In a follow-up PR, it might be interesting to normalize the parse5Elm and fix the location attribute in place in parseElement to avoid having to carry along this parse5Location object.

Filed #3108

packages/@lwc/template-compiler/src/parser/index.ts

jye-sf

First pass with some questions.

jye-sf · 2022-10-11T16:54:41Z

packages/@lwc/template-compiler/src/parser/index.ts

        ctx.warnAtLocation(ParserDiagnostics.NO_SLOTS_IN_ITERATOR, location, [
            name === '' ? 'default' : `name="${name}"`,
        ]);
    }

+    if (isScopedSlot) {
+        ctx.seenScopedSlots.add(name);


One feature of if/elseif/else is that slots in mutually exclusive branches of a conditional tree are allowed to have the same name. Is that something that should hold true for scoped slots as well, or is this intentionally staying as a global store?

Sample Possible Use Case

<template lwc:render-mode="light"> <template lwc:if={showStandard}> <slot name="slotname1" lwc:slot-bind={slot1StandardData}></slot> </template> <template lwc:elseif={showVariant}> <slot name="slotname1" lwc:slot-bind={slot1VariantData}></slot> </template> <template lwc:else> <slot lwc:slot-bind={defaultdata}></slot> </template> </template>

Note that since the alreadySeen logic above reuses the standard slot name detection, the code in lines 1218 - 1219 will return false for alreadySeen when parsing the second slotname1 slot in the above example and no error will be thrown. We'll just be adding the name to the seenScopedSlots set twice.

With the way seenScopedSlots is currently used, the code should work fine, but the parser is in an interesting brittle state when we parse the second slot in the above example: alreadySeen === false, because the other slot is in a separate conditional branch and isn't relevant to the current context, while ctx.seenScopedSlots.has(name) === true, because we don't make this distinction when managing the seenScopedSlots Set.

@jye-sf
I thought about this and added some test cases to come with this opinion.

Basically, this.seenScopedSlots does not need to be if-block aware. The detection logic is handled by ctx.hasSeenSlot. The look up on ctx.seenScopedSlots is to just augment that information to produce an appropriate error/warning message.

However, it can be made context aware by replicating the same was slot names are tracked. The scoped slots set will be a subset of the slot set and it does not bring much more else. I didn't see much value in doing that.

Let me know if 1 is okay or if you strongly prefer 2.

I am okay with 1. It makes sense to not want to add a bunch of unnecessary duplicate code.

Maybe we can just rename alreadySeen to seenInContext or something like that; that might partially address the potential brittleness.

packages/@lwc/template-compiler/src/shared/ast.ts

@@ -44,6 +44,10 @@ import {
    SpreadDirective,
    ElementDirective,
    RootDirective,
+    SlotBindDirective,
+    /* SlotDataDirective, */


packages/@lwc/template-compiler/src/codegen/codegen.ts

@@ -356,6 +358,10 @@ export default class CodeGen {
        );
    }

+    getScopedSlotFactory(callback: t.FunctionExpression, slotName: t.SimpleLiteral) {


…ontent

pmdartus

Here is a first pass on the types and tests only.

packages/@lwc/template-compiler/src/shared/types.ts

pmdartus · 2022-10-12T14:28:20Z

packages/@lwc/template-compiler/src/shared/types.ts

+    | SlotBindDirective
+    | SlotDataDirective


This should be a lot more restrictive. The SlotBindDirective could only be present on a Slot element, while the SlotDataDirective can only be applied to ScopedSlotContent.

I would remove the SlotBindDirective from this list and add it specifically to the Slot node (if possible). Regarding the ScopedSlotContent directive, there is something wrong in this PR, see comment below.

Tried to do it, but it creates branching logic in downstream consumers of element.directives, for example renderer-hooks.ts:checkElement and codegen/index.ts:elementDataBag. They have to add a special branch for <slot> elements.

Currently the lwc:slot-bind directive is restricted to only <slot> elements in applyLwcSlotBindDirective() routine.

packages/@lwc/errors/src/compiler/error-info/template-transform.ts

...er/src/__tests__/fixtures/scoped-slots/invalid/slot-data/with-for-each-sibling/metadata.json

...ges/@lwc/template-compiler/src/__tests__/fixtures/scoped-slots/parent-named-slot/actual.html

@@ -0,0 +1,15 @@
+<template>
+    <x-child> <!-- no `lwc:slot-data` on the component tag -->


...ompiler/src/__tests__/fixtures/scoped-slots/invalid/config-unset/child-slot-bind/actual.html

pmdartus · 2022-10-12T15:08:44Z

...ate-compiler/src/__tests__/fixtures/scoped-slots/if-block/valid/mixed-slot-types/actual.html

+    <template lwc:if={showStandard}>
+        <slot name="slotname1"></slot>
+    </template>
+    <template lwc:elseif={showVariant}>
+        <slot name="slotname1" lwc:slot-bind={slot1VariantData}></slot>
+    </template>


How does this component work at runtime? I do see how a consumer could use this component.

The slot content type(standard v/s scoped) has to be in sync with the child's slots. Unfortunately, this cannot be checked at compile time. So at runtime, the combinations are as follows:

child and parent use same type of slot: happy path, works fine.

child and parent have mixed slot type: log an error in non-prod mode, degrade gracefully and revert to fallback content

Will add tests cases and handling for 2 in followup PR(as part of W-11838547)

.../__tests__/fixtures/scoped-slots/invalid/slot-bind/invalid-on-non-slot-content/metadata.json

packages/@lwc/template-compiler/src/shared/types.ts

packages/@lwc/template-compiler/src/parser/index.ts

pmdartus · 2022-10-13T12:36:20Z

packages/@lwc/template-compiler/src/parser/index.ts

@@ -252,7 +265,8 @@ function parseElementDirectives(
    ctx: ParserCtx,
    parsedAttr: ParsedAttribute,
    parent: ParentNode,
-    parse5ElmLocation: parse5.ElementLocation
+    parse5ElmLocation: parse5.ElementLocation,


I am also on the side of @jmsjtu. I had to get back into the code to better understand why we don't access the elementLocation property directly on the element.

In a follow-up PR, it might be interesting to normalize the parse5Elm and fix the location attribute in place in parseElement to avoid having to carry along this parse5Location object.

packages/@lwc/template-compiler/src/parser/index.ts

packages/@lwc/template-compiler/src/codegen/index.ts

+        } = scopedSlotContent;
+        codeGen.beginScope();
+        codeGen.declareIdentifier(dataIdentifier);
+        // TODO [#9999]: Next Step: Return a VFragment


packages/@lwc/engine-core/src/framework/api.ts

pmdartus · 2022-10-13T13:41:37Z

packages/@lwc/engine-core/src/framework/vm.ts

-    [key: string]: VNodes;
+    // Slot assignments by name
+    slotAssignments: { [key: string]: VNodes };
+    owner?: VM;


Why isn't the owner property required? All the VNodes associated with the slotAssignments has to have an owner.

The root vm will not have an owner for its slotset(its slotAssignments are also empty). One option is to make VM['cmpSlots'] an optional field. It introduces a few checks every time vm.cmpSlots are accessed. I felt this was a better tradeoff.

One option is to make VM['cmpSlots'] an optional field.

I think this is a preferable approach to avoid allocating 2 empty objects for all the components that don't accept slots.

pmdartus · 2022-10-13T13:44:16Z

packages/@lwc/engine-core/src/framework/api.ts

+                if (!isUndefined(slotset.owner)) {
+                    // Evaluate in the scope of the slot content's owner
+                    setVMBeingRendered(slotset.owner);
+                }


This branch should always evaluate, in other words there shouldn't be a case where owner is undefined. See the comment below on the SlotSet interface.

I could remove the isUndefined check and always assume slotset.owner is present. It is indeed present if we consider that root component will never get slotted content.
setVMBeingRendered(slotset.owner!);

I could remove the isUndefined check and always assume slotset.owner is present.

I think this is preferable since it better models the actual runtime behavior. It's impossible in active to have slotted content without an owner.

pmdartus

Added a couple of last nitpicks otherwise this PR looks good.

pmdartus · 2022-10-14T12:07:14Z

packages/@lwc/engine-core/src/framework/api.ts

+                if (!isUndefined(slotset.owner)) {
+                    // Evaluate in the scope of the slot content's owner
+                    setVMBeingRendered(slotset.owner);
+                }


I could remove the isUndefined check and always assume slotset.owner is present.

I think this is preferable since it better models the actual runtime behavior. It's impossible in active to have slotted content without an owner.

pmdartus · 2022-10-14T12:11:26Z

packages/@lwc/engine-core/src/framework/api.ts

+                let children: VNodes = [];
+                if (!isUndefined(slotset.owner)) {
+                    // Evaluate in the scope of the slot content's owner
+                    setVMBeingRendered(slotset.owner);


Side note, setVMBeingRendered is used in 2 different places with this PR: for template evaluation and for fragment evaluation. I was thinking about a way to abstract this since LWC template is just a supercharged fragment.

I haven't found an elegant way to do this for now. It's certainly something worth keeping in the back of our head for the next time we touch this part of the code.

pmdartus · 2022-10-14T12:14:37Z

packages/@lwc/integration-karma/test/light-dom/scoped-slot/index.spec.js

@@ -0,0 +1,40 @@
+import { createElement } from 'lwc';


For the sanity test, I would also recommend adding a simple test for SSR.

jmsjtu

I ran the coverage report and added small nitpicks. Otherwise, looks good!

packages/@lwc/template-compiler/src/parser/index.ts

jmsjtu

LGTM!

* feat(template-compiler): implement handling for scoped slot directives in template (#3077) * feat: add handling of lwc:data-bind directive * feat: add handling for lwc:slot-data directive * feat: add capability in engine to accept a factory function as slot content * chore: release v2.27.0 (#3118)

ravijayaramappa marked this pull request as draft October 3, 2022 18:09

ravijayaramappa force-pushed the ravi/master/scoped-slots branch from 3691114 to e8f640b Compare October 5, 2022 18:59

ravijayaramappa marked this pull request as ready for review October 6, 2022 21:52

ravijayaramappa requested review from nolanlawson, jye-sf, pmdartus, jmsjtu and jodarove October 6, 2022 21:53

ravijayaramappa changed the title ~~feat(template-compiler): implement handling for scoped slot directives in template - WIP~~ feat(template-compiler): implement handling for scoped slot directives in template Oct 6, 2022

jodarove reviewed Oct 7, 2022

View reviewed changes

nolanlawson reviewed Oct 7, 2022

View reviewed changes

jmsjtu requested changes Oct 11, 2022

View reviewed changes

ravijayaramappa requested a review from caridy October 11, 2022 16:51

jye-sf requested changes Oct 11, 2022

View reviewed changes

ravijayaramappa added 14 commits October 11, 2022 11:06

test: initial set of tests for scoped slots

52f0d2f

feat: add handling of lwc:data-bind directive

45c217d

feat: add handling for lwc:slot-data directive

90b9b0e

fix: create SlotData directive node for continuity

3c90967

fix: change warning to error

4b9166d

feat: add capability in engine to accept a factory function as slot c…

e0e49fe

…ontent

test: update tests after rebase

3cd6be4

test: update error message

812ff70

feat: add code gen for scoped slots

3e51fa9

fix: formatting a file

188e84f

test: basic karma test with default slot

94edd67

test: test case with forEach

3be71d6

test: more tests for bindings and nested slots

505e592

fix: address review feedback

4a426fe

ravijayaramappa mentioned this pull request Oct 11, 2022

[Scoped Slots][Doc] Update error message about directives usage restriction to point to external documentation. #3100

Open

test: more tests for duplicates detection

3576c5e

ravijayaramappa force-pushed the ravi/master/scoped-slots branch from 2f44a97 to 3576c5e Compare October 12, 2022 01:57

fix: revert fixtures setup

1bbcbb4

pmdartus reviewed Oct 12, 2022

View reviewed changes

ravijayaramappa added 3 commits October 12, 2022 16:31

fix: address PMs review comments

21e7c11

fix: update fixture files

54ef4b5

test: missing negative test cases

7840bfa

ravijayaramappa mentioned this pull request Oct 13, 2022

test: [scoped slots] Add runtime handling and karma tests for reactivity #3105

Merged

ravijayaramappa requested review from jye-sf, jmsjtu, pmdartus, nolanlawson and jodarove October 13, 2022 00:55

pmdartus reviewed Oct 13, 2022

View reviewed changes

ravijayaramappa added 3 commits October 13, 2022 12:18

fix: address PMs review comments

3a4edf5

fix: shuffle order of arugments in ssf api

2a95159

fix: update todo with issue id

1798af7

ravijayaramappa requested a review from pmdartus October 13, 2022 20:18

pmdartus approved these changes Oct 14, 2022

View reviewed changes

test: add tests for ssr

ce36e81

jye-sf approved these changes Oct 14, 2022

View reviewed changes

jmsjtu approved these changes Oct 14, 2022

View reviewed changes

packages/@lwc/template-compiler/src/parser/index.ts Show resolved Hide resolved

packages/@lwc/template-compiler/src/parser/index.ts Show resolved Hide resolved

fix: address pr comments from james

8313089

jmsjtu approved these changes Oct 14, 2022

View reviewed changes

ravijayaramappa merged commit 2ada663 into master Oct 14, 2022

ravijayaramappa deleted the ravi/master/scoped-slots branch October 14, 2022 22:28

nolanlawson mentioned this pull request Mar 1, 2023

Support scoped slots #210

Closed

		@@ -0,0 +1,15 @@
		<template>
		<x-child> <!-- no `lwc:slot-data` on the component tag -->

feat(template-compiler): implement handling for scoped slot directives in template #3077

feat(template-compiler): implement handling for scoped slot directives in template #3077

Conversation

ravijayaramappa commented Oct 2, 2022 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

salesforce-nucleus bot commented Oct 6, 2022

salesforce-nucleus bot commented Oct 6, 2022

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

nolanlawson left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

ravijayaramappa Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

pmdartus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravijayaramappa Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmdartus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsjtu left a comment

Choose a reason for hiding this comment

jmsjtu left a comment

Choose a reason for hiding this comment

ravijayaramappa commented Oct 2, 2022 •

edited

Loading

ravijayaramappa Oct 11, 2022 •

edited

Loading

ravijayaramappa Oct 14, 2022 •

edited

Loading