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

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

Merged
merged 24 commits into from
Oct 14, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
52f0d2f
test: initial set of tests for scoped slots
ravijayaramappa Sep 30, 2022
45c217d
feat: add handling of lwc:data-bind directive
ravijayaramappa Oct 2, 2022
90b9b0e
feat: add handling for lwc:slot-data directive
ravijayaramappa Oct 2, 2022
3c90967
fix: create SlotData directive node for continuity
ravijayaramappa Oct 3, 2022
4b9166d
fix: change warning to error
ravijayaramappa Oct 5, 2022
e0e49fe
feat: add capability in engine to accept a factory function as slot c…
ravijayaramappa Oct 5, 2022
3cd6be4
test: update tests after rebase
ravijayaramappa Oct 5, 2022
812ff70
test: update error message
ravijayaramappa Oct 5, 2022
3e51fa9
feat: add code gen for scoped slots
ravijayaramappa Oct 5, 2022
188e84f
fix: formatting a file
ravijayaramappa Oct 5, 2022
94edd67
test: basic karma test with default slot
ravijayaramappa Oct 5, 2022
3be71d6
test: test case with forEach
ravijayaramappa Oct 6, 2022
505e592
test: more tests for bindings and nested slots
ravijayaramappa Oct 6, 2022
4a426fe
fix: address review feedback
ravijayaramappa Oct 11, 2022
3576c5e
test: more tests for duplicates detection
ravijayaramappa Oct 12, 2022
1bbcbb4
fix: revert fixtures setup
ravijayaramappa Oct 12, 2022
21e7c11
fix: address PMs review comments
ravijayaramappa Oct 12, 2022
54ef4b5
fix: update fixture files
ravijayaramappa Oct 12, 2022
7840bfa
test: missing negative test cases
ravijayaramappa Oct 13, 2022
3a4edf5
fix: address PMs review comments
ravijayaramappa Oct 13, 2022
2a95159
fix: shuffle order of arugments in ssf api
ravijayaramappa Oct 13, 2022
1798af7
fix: update todo with issue id
ravijayaramappa Oct 13, 2022
ce36e81
test: add tests for ssr
ravijayaramappa Oct 14, 2022
8313089
fix: address pr comments from james
ravijayaramappa Oct 14, 2022
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
9 changes: 8 additions & 1 deletion packages/@lwc/compiler/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,25 @@ export interface TransformOptions {
enableStaticContentOptimization?: boolean;
customRendererConfig?: CustomRendererConfig;
enableLwcSpread?: boolean;
enableScopedSlots?: boolean;
}

type RequiredTransformOptions = Omit<
TransformOptions,
'name' | 'namespace' | 'scopedStyles' | 'customRendererConfig' | 'enableLwcSpread'
| 'name'
| 'namespace'
| 'scopedStyles'
| 'customRendererConfig'
| 'enableLwcSpread'
| 'enableScopedSlots'
>;
export interface NormalizedTransformOptions extends RecursiveRequired<RequiredTransformOptions> {
name?: string;
namespace?: string;
scopedStyles?: boolean;
customRendererConfig?: CustomRendererConfig;
enableLwcSpread?: boolean;
enableScopedSlots?: boolean;
}

export function validateTransformOptions(options: TransformOptions): NormalizedTransformOptions {
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/compiler/src/transformers/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export default function templateTransform(
enableStaticContentOptimization,
customRendererConfig,
enableLwcSpread,
enableScopedSlots,
} = options;
const experimentalDynamicDirective = Boolean(experimentalDynamicComponent);

Expand All @@ -44,6 +45,7 @@ export default function templateTransform(
enableStaticContentOptimization,
customRendererConfig,
enableLwcSpread,
enableScopedSlots,
});
} catch (e) {
throw normalizeToCompilerError(TransformerErrors.HTML_TRANSFORMER_ERROR, e, { filename });
Expand Down
45 changes: 41 additions & 4 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ import {
isUndefined,
StringReplace,
toString,
ArrayConcat,
} from '@lwc/shared';

import { logError } from '../shared/logger';

import { invokeEventListener } from './invoker';
import { getVMBeingRendered } from './template';
import { getVMBeingRendered, setVMBeingRendered } from './template';
import { EmptyArray, setRefVNode } from './utils';
import { isComponentConstructor } from './def';
import { ShadowMode, SlotSet, VM, RenderMode } from './vm';
Expand All @@ -44,6 +45,8 @@ import {
VStatic,
Key,
VFragment,
isVScopedSlotContent,
VScopedSlotContent,
} from './vnodes';

const SymbolIterator: typeof Symbol.iterator = Symbol.iterator;
Expand All @@ -52,6 +55,19 @@ function addVNodeToChildLWC(vnode: VCustomElement) {
ArrayPush.call(getVMBeingRendered()!.velements, vnode);
}

// [s]coped [s]lot [f]actory
function ssf(factory: (value: any) => VNodes, slotName?: string): VScopedSlotContent {
return {
type: VNodeType.ScopedSlotContent,
factory,
owner: getVMBeingRendered()!,
elm: undefined,
sel: undefined,
key: undefined,
slotName,
};
}

// [st]atic node
function st(fragment: Element, key: Key): VStatic {
return {
Expand Down Expand Up @@ -169,10 +185,30 @@ function s(
}
if (
!isUndefined(slotset) &&
!isUndefined(slotset[slotName]) &&
slotset[slotName].length !== 0
!isUndefined(slotset.slotAssignments) &&
!isUndefined(slotset.slotAssignments[slotName]) &&
slotset.slotAssignments[slotName].length !== 0
) {
children = slotset[slotName];
children = slotset.slotAssignments[slotName].reduce((acc, vnode) => {
// If the passed slot content is factory, evaluate it and use the produced vnodes
if (vnode && isVScopedSlotContent(vnode)) {
const vmBeingRenderedInception = getVMBeingRendered();
let children: VNodes = [];
if (!isUndefined(slotset.owner)) {
// Evaluate in the scope of the slot content's owner
setVMBeingRendered(slotset.owner);
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ravijayaramappa ravijayaramappa Oct 14, 2022

Choose a reason for hiding this comment

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

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!);

Copy link
Member

Choose a reason for hiding this comment

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

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.

try {
children = vnode.factory(data.slotData);

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on a separate PR for testing reactivity #3105

} finally {
setVMBeingRendered(vmBeingRenderedInception);
}
return ArrayConcat.call(acc, children);
} else {
// If the slot content is a static list of child nodes provided by the parent, nothing to do
return ArrayConcat.call(acc, vnode);
}
}, [] as VNodes);
}
const vmBeingRendered = getVMBeingRendered()!;
const { renderMode, shadowMode } = vmBeingRendered;
Expand Down Expand Up @@ -571,6 +607,7 @@ const api = ObjectFreeze({
gid,
fid,
shc,
ssf,
});

export default api;
Expand Down
33 changes: 21 additions & 12 deletions packages/@lwc/engine-core/src/framework/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
VNodeType,
VStatic,
VFragment,
isVScopedSlotContent,
} from './vnodes';

import { patchAttributes } from './modules/attrs';
Expand Down Expand Up @@ -576,7 +577,7 @@ export function allocateChildren(vnode: VCustomElement, vm: VM) {
const { renderMode, shadowMode } = vm;
if (shadowMode === ShadowMode.Synthetic || renderMode === RenderMode.Light) {
// slow path
allocateInSlot(vm, children);
allocateInSlot(vm, children, vnode.owner);
// save the allocated children in case this vnode is reused.
vnode.aChildren = children;
// every child vnode is now allocated, and the host should receive none directly, it receives them via the shadow!
Expand Down Expand Up @@ -611,9 +612,12 @@ function createViewModelHook(elm: HTMLElement, vnode: VCustomElement, renderer:
return vm;
}

function allocateInSlot(vm: VM, children: VNodes) {
const { cmpSlots: oldSlots } = vm;
const cmpSlots = (vm.cmpSlots = create(null));
function allocateInSlot(vm: VM, children: VNodes, owner: VM) {
const {
cmpSlots: { slotAssignments: oldSlotsMapping },
} = vm;
const cmpSlotsMapping = create(null);
vm.cmpSlots = { owner, slotAssignments: cmpSlotsMapping };
for (let i = 0, len = children.length; i < len; i += 1) {
const vnode = children[i];
if (isNull(vnode)) {
Expand All @@ -622,29 +626,34 @@ function allocateInSlot(vm: VM, children: VNodes) {

let slotName = '';
if (isVBaseElement(vnode)) {
slotName = (vnode.data.attrs?.slot as string) || '';
slotName = (vnode.data.attrs?.slot as string) ?? '';
} else if (isVScopedSlotContent(vnode)) {
slotName = vnode.slotName ?? '';
}

const vnodes: VNodes = (cmpSlots[slotName] = cmpSlots[slotName] || []);
const vnodes: VNodes = (cmpSlotsMapping[slotName] = cmpSlotsMapping[slotName] || []);
ArrayPush.call(vnodes, vnode);
}
if (isFalse(vm.isDirty)) {
// We need to determine if the old allocation is really different from the new one
// and mark the vm as dirty
const oldKeys = keys(oldSlots);
if (oldKeys.length !== keys(cmpSlots).length) {
const oldKeys = keys(oldSlotsMapping);
if (oldKeys.length !== keys(cmpSlotsMapping).length) {
markComponentAsDirty(vm);
return;
}
for (let i = 0, len = oldKeys.length; i < len; i += 1) {
const key = oldKeys[i];
if (isUndefined(cmpSlots[key]) || oldSlots[key].length !== cmpSlots[key].length) {
if (
isUndefined(cmpSlotsMapping[key]) ||
oldSlotsMapping[key].length !== cmpSlotsMapping[key].length
) {
markComponentAsDirty(vm);
return;
}
const oldVNodes = oldSlots[key];
const vnodes = cmpSlots[key];
for (let j = 0, a = cmpSlots[key].length; j < a; j += 1) {
const oldVNodes = oldSlotsMapping[key];
const vnodes = cmpSlotsMapping[key];
for (let j = 0, a = cmpSlotsMapping[key].length; j < a; j += 1) {
if (oldVNodes[j] !== vnodes[j]) {
markComponentAsDirty(vm);
return;
Expand Down
6 changes: 3 additions & 3 deletions packages/@lwc/engine-core/src/framework/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ function validateSlots(vm: VM, html: Template) {
const { cmpSlots } = vm;
const { slots = EmptyArray } = html;

for (const slotName in cmpSlots) {
for (const slotName in cmpSlots.slotAssignments) {
// eslint-disable-next-line @lwc/lwc-internal/no-production-assert
assert.isTrue(
isArray(cmpSlots[slotName]),
isArray(cmpSlots.slotAssignments[slotName]),
`Slots can only be set to an array, instead received ${toString(
cmpSlots[slotName]
cmpSlots.slotAssignments[slotName]
)} for slot "${slotName}" in ${vm}.`
);

Expand Down
9 changes: 6 additions & 3 deletions packages/@lwc/engine-core/src/framework/vm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export interface TemplateCache {
}

export interface SlotSet {
[key: string]: VNodes;
// Slot assignments by name
slotAssignments: { [key: string]: VNodes };
owner?: VM;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

export const enum VMState {
Expand Down Expand Up @@ -135,7 +137,8 @@ export interface VM<N = HostNode, E = HostElement> {
velements: VCustomElement[];
/** The component public properties. */
cmpProps: { [name: string]: any };
/** The mapping between the slot names and the slotted VNodes. */
/** Contains information about the mapping between the slot names and the slotted VNodes, and
* the owner of the slot content. */
cmpSlots: SlotSet;
/** The component internal reactive properties. */
cmpFields: { [name: string]: any };
Expand Down Expand Up @@ -301,7 +304,7 @@ export function createVM<HostNode, HostElement>(
velements: EmptyArray,
cmpProps: create(null),
cmpFields: create(null),
cmpSlots: create(null),
cmpSlots: { slotAssignments: create(null) },
oar: create(null),
cmpTemplate: null,
hydrated: Boolean(hydrated),
Expand Down
22 changes: 21 additions & 1 deletion packages/@lwc/engine-core/src/framework/vnodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,17 @@ export const enum VNodeType {
CustomElement,
Static,
Fragment,
ScopedSlotContent,
}

export type VNode = VText | VComment | VElement | VCustomElement | VStatic | VFragment;
export type VNode =
| VText
| VComment
| VElement
| VCustomElement
| VStatic
| VFragment
| VScopedSlotContent;
export type VParentElement = VElement | VCustomElement | VFragment;
export type VNodes = Readonly<Array<VNode | null>>;

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

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

Choose a reason for hiding this comment

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

9999? 🙃

factory: (value: any) => VNodes;
type: VNodeType.ScopedSlotContent;
slotName?: string;
}

export interface VStatic extends BaseVNode {
type: VNodeType.Static;
sel: undefined;
Expand Down Expand Up @@ -106,6 +121,7 @@ export interface VElementData extends VNodeData {
// Similar to above, all props are readonly
readonly key: Key;
readonly ref?: string;
readonly slotData?: any;
}

export function isVBaseElement(vnode: VNode): vnode is VElement | VCustomElement {
Expand All @@ -120,3 +136,7 @@ export function isSameVnode(vnode1: VNode, vnode2: VNode): boolean {
export function isVCustomElement(vnode: VBaseElement): vnode is VCustomElement {
return vnode.type === VNodeType.CustomElement;
}

export function isVScopedSlotContent(vnode: VNode): vnode is VScopedSlotContent {
return vnode.type === VNodeType.ScopedSlotContent;
}
2 changes: 1 addition & 1 deletion packages/@lwc/errors/src/compiler/error-info/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
/**
* Next error code: 1167
* Next error code: 1179
*/

export * from './compiler';
Expand Down
Loading