feat(template-compiler): dynamic components by jmsjtu · Pull Request #3337 · salesforce/lwc

jmsjtu · 2023-02-09T04:48:22Z

Details

This PR implements the changes to the template compiler from the dynamic components RFC.

This is part 1 of 3 of the dynamic components implementation.

Part 2 (babel plugin / runtime implementation) can be found here.
Part 3 (karma integration tests) will be created after parts 1 and 2 have been merged.

PR structure

This implementation is broken down into 3 parts to make it easier to review and will be merged into the jtu/dynamic-components branch.

The reason for merging into the jtu/dynamic-components branch first is to ensure a single atomic commit for the dynamic components feature and to facilitate a clean coordinated release to lwc-platform, as this is a breaking change for lwc-platform.

There are a large number of file changes in this PR, most of which come from updates to the unit tests.

I've grouped all of the unit tests under the following two commits, if you are reviewing this PR through the GitHub UI you can exclude these two commits:

5b27c96
d7b1c63

This should reduce the number of files changed significantly.

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-12190514

…omponents-template

jmsjtu · 2023-02-09T04:55:18Z

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

+export interface BaseLwcElement<T extends `${LwcTagName}`> extends AbstractBaseElement {
+    type: 'Lwc';
+    name: T;
+}
+
+/**
+ * Node representing the lwc:component element
+ */
+export interface LwcComponent extends BaseLwcElement<'lwc:component'> {}
+
+/**
+ * All supported special LWC tags, they should all begin with lwc:*
+ */
+export enum LwcTagName {
+    Component = 'lwc:component',
+}
+
+export type BaseElement = Element | ExternalComponent | Component | Slot | LwcComponent;


I created the AST nodes to look like this because I assumed we would have more lwc:* elements in the future.

I felt that this could help set the basis for future lwc:* components but it may also be premature to create the nodes structure ahead of time.

I can change this to a single AST node if the team prefers.

Let's plan for more lwc:* nodes. I think it will come in the next release. (lwc:host)

jmsjtu · 2023-02-09T04:58:08Z

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

+function dc(
+    Ctor: LightningElementConstructor | null | undefined,
+    data: VElementData,
+    children: VNodes = EmptyArray
+): VCustomElement | null {
+    if (process.env.NODE_ENV !== 'production') {
+        assert.isTrue(isObject(data), `dc() 2nd argument data must be an object.`);
+        assert.isTrue(
+            arguments.length === 3 || isArray(children),
+            `dc() 3rd argument data must be an array.`
+        );
+    }
+    // null or undefined values should produce a null value in the VNodes
+    if (isNull(Ctor) || isUndefined(Ctor)) {
+        return null;
+    }
+
+    if (!isComponentConstructor(Ctor)) {
+        throw new Error(
+            `Invalid constructor ${toString(Ctor)} is not a LightningElement constructor.`
+        );
+    }
+
+    return null;
+}


This is a dummy implementation and is not how the function is designed to work. The implementation can be found in part 2, here.

This is only added here because the template compiler will call the dc function for the new dynamic components syntax (<lwc:component lwc:is={}>)

...llup-plugin/src/__tests__/compilerConfig/fixtures/dynamicImportConfig/dynamicImportConfig.js

ravijayaramappa

First pass. Yet to review template-compiler code

ravijayaramappa · 2023-02-09T19:09:21Z

packages/@lwc/compiler/src/index.ts

    StylesheetConfig,
    CustomPropertiesResolution,
-    DynamicComponentConfig,
+    DynamicImportConfig,


This change will need a co-ordinated change in lwc-platform

Also needs a coordinated release as experimentalDynamicDirective should now be available as a compiler option.

ravijayaramappa · 2023-02-09T19:14:56Z

packages/@lwc/compiler/src/options.ts

-    experimentalDynamicComponent?: DynamicComponentConfig;
+    dynamicImportConfig?: DynamicImportConfig;
+    // TODO [#3331]: remove usage of lwc:dynamic in 246
+    experimentalDynamicDirective?: boolean;


IMO, these old types are best left alone for deprecation later.

The cascading effect of changing the existing types will become a overhead to manage. Here are some the repos that depend on this type:

lwc-platform

lwc-test

I debated about this initially but I think you're right. Changing these compiler options will lead to unnecessary overhead in managing the release of this feature to downstream dependencies.

I've changed dynamicImportConfig back to experimentalDynamicComponents to prevent breaking downstream consumers and to ease the enablement of this feature.

We're planning to remove the lwc:dynamic directive entirely in 246 and I plan to deprecate experimentalDynamicComponent in favor of dynamicImportConfig as part of it.

You can remove this from the PR description!

packages/@lwc/compiler/src/options.ts

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

nolanlawson · 2023-02-15T00:03:52Z

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

+
+    LWC_COMPONENT_TAG_WITHOUT_IS_DIRECTIVE: {
+        code: 1183,
+        message: `<lwc:component> must have an 'lwc:is' attribute.`,


I read this as "an ell" so I think it's fine to say an.

nolanlawson · 2023-02-15T00:04:21Z

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

+
+    UNSUPPORTED_LWC_TAG_NAME: {
+        code: 1184,
+        message: '{0} is not a special LWC tag name and will be treated as an HTML element.',


Same here. I know some Anglophones say "haitch" for "H", but "aitch" is more common AFAIK.

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

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

ravijayaramappa

Awesome work with the test cases!

...wc/template-compiler/src/__tests__/fixtures/directive-for-each/dynamic-component/actual.html

packages/@lwc/template-compiler/src/__tests__/fixtures/directive-inner-html/invalid/actual.html

ravijayaramappa · 2023-02-10T19:23:16Z

...-compiler/src/__tests__/fixtures/directive-lwc-if-else/dynamic-components/inline/actual.html

@@ -0,0 +1,11 @@
+<template>
+    <lwc:component lwc:is={ctor1} lwc:if={visible.if}>


This test case also covers slotting. But can you please add test case specifically for slotting

Slotting where content is not wrapped in vfragment

Slotting where content is not statically optimized

Scoped Slots

@ravijayaramappa the latest commit contains the updated unit tests, let me know what you think!

...late-compiler/src/__tests__/fixtures/renderer-hooks/directive-inner-html/invalid/actual.html

...rc/__tests__/fixtures/scoped-slots/invalid/slot-data/invalid-on-non-slot-content/actual.html

ravijayaramappa · 2023-02-10T19:53:51Z

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

+            );
+        }
+    } else {
+        // Certain tag names that start with lwc:* are signals to the compiler for special behavior.


With the current implementation, this block is never reached.
parseLwcElement is only invoked if isLwcElementTag is true. And isLwcElementTag is based on known LwcTagNames. I didn't see a test case with this error code either.

Great catch! I've updated the logic on isLwcElementTag to only look at the tag name.

The updated logic should now check if the tag starts with lwc: and then create the appropriate AST node.

I've updated the tests here to include the new warnings.

ravijayaramappa · 2023-02-10T19:56:59Z

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

+) {
+    const { tagName: tag, namespaceURI } = parse5Elm;
+
+    if (tag === LwcTagName.Component) {


This block is designed to handle all future lwc:* tag names. How about a switch statement here to handle all future lwc:* cases? 😸

Updated in the latest commit!

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

nolanlawson · 2023-02-15T00:07:58Z

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

+    let lwcElementParser;
+
+    switch (parse5Elm.tagName) {
+        case ctx.config.enableDynamicComponents && LwcTagName.Component:


This is kind of weird to me. So if ctx.config.enableDynamicComponents is false, then we will just treat lwc:component as an unknown HTML element? I guess it's fine, though, since throwing an error here is kind of pointless since we will just have to get rid of it when the enableDynamicComponents flag is removed.

Ah I see, we are warning below. OK, this makes sense then.

Agree, this switch statement is weird. @jmsjtu IMO, the checking of ctx.config.enableDynamicComponents should be inside parseLwcComponent. parseLwcComponent can validate if a template has lwc:is without enabling enableDynamicComponents and warn. This way all the validation for lwc:is is consolidated in one routine.

@nolanlawson @ravijayaramappa after some thought I've removed the LWC_COMPONENT_USED_WITHOUT_OPT warning.

The original intent of the warning was to notify component authors that lwc:component would be treated as an unknown HTML element if the ctx.config.enableDynamicComponents is not enabled.

However, now that I think about it, this can be confusing. I've opted instead to treat lwc:component the same regardless of whether ctx.config.enableDynamicComponents is enabled.

This way parseLwcComponent checks to see that lwc:component must be used with lwc:is and applyLwcIsDirective checks to see that ctx.config.enableDynamicComponents is enabled to use lwc:is.

The latest commit has the updated logic, let me know what you think.

@jmsjtu So what happens if enableDynamicComponents is false but the template contains lwc:component?

If enableDynamicComponents is false and a template contains a lwc:component without lwc:is it will throw an error saying you need to use lwc:is with lwc:component.

If enableDynamicComponents is false and have lwc:component + lwc:is you will get an error saying enableDynamicComponents must be enabled.

In either case if enableDynamicComponents is false or true we treat lwc:component the same and only validate the config on lwc:is.

IMO, in both case #1 and #2, the error should say that enableDynamicComponents must be enabled to use <lwc:component> element. In other words, enableDynamicComponents flag gates the usage of both lwc:component and/or lwc:is.
My original suggestion was related to the organization of the code.

@nolanlawson @ravijayaramappa - I decided to move the check for enableDynamicComponents from applyLwcIsDirective to parseLwcComponent.

I think this way the validation is cleaner, here are the new unit tests.

Let me know if you think should be changed. I'm going to merge this PR but it'll just go into the jtu/dynamic-components branch.

Sorry for the back and forth on this!

nolanlawson · 2023-02-15T00:09:47Z

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

+    // Bind LWC directives to element
+    for (const matchAndApply of LWC_DIRECTIVE_PROCESSORS) {
+        matchAndApply(ctx, parsedAttr, element);
+    }


nolanlawson

This LGTM, and thank you for breaking up the PRs!

ravijayaramappa

LGTM, minor comments.

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

ravijayaramappa · 2023-02-15T00:42:46Z

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

+    let lwcElementParser;
+
+    switch (parse5Elm.tagName) {
+        case ctx.config.enableDynamicComponents && LwcTagName.Component:


Agree, this switch statement is weird. @jmsjtu IMO, the checking of ctx.config.enableDynamicComponents should be inside parseLwcComponent. parseLwcComponent can validate if a template has lwc:is without enabling enableDynamicComponents and warn. This way all the validation for lwc:is is consolidated in one routine.

divmain · 2023-02-17T05:41:11Z

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

+) {
+    let lwcElementParser;
+
+    switch (parse5Elm.tagName) {


Is there a reason to use a switch here rather than if (parse5Elm.tagName === LwcTagName.Component) { ... } else { ... }? Do you expect there will be additional cases here?

Yup, we're planning to add more LWC managed elements (that start with lwc:*) like lwc:host

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

pmdartus · 2023-02-21T09:06:37Z

packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts

            }),
        ],
+        onwarn(warning) {
+            warnings.push(warning);


I would recommend checking whether or not the warning is related to lwc:dynamic before silencing it. Swallowing all the errors might hide some unexpected behavior in tests.

pmdartus · 2023-02-21T09:10:28Z

packages/@lwc/rollup-plugin/src/index.ts

    preserveHtmlComments?: boolean;
    /** The configuration to pass to `@lwc/compiler`. */
-    experimentalDynamicComponent?: DynamicComponentConfig;
+    experimentalDynamicComponent?: DynamicImportConfig;


IMO, it's quite tedious to update the RollupLwcOptions interface every time a new flag is added. It might be interesting to define the RollupLwcOptions interface as a union with the LWC compiler config interface.

jmsjtu added 9 commits February 7, 2023 17:14

feat: add new compiler flags for enabling <lwc:component lwc:is={}>

aad8a45

feat: add new types for dynamic components

757e9fb

feat: add new error messages

86cfb90

feat: update template compiler to parse new dynamic component syntax

3c566fd

test: create unit tests

5b27c96

Merge branch 'master' of github.com:salesforce/lwc into jtu/dynamic-c…

30b9bd0

…omponents-template

feat: update compiler config options

412abb0

test: add unit tests for compiler option changes

d7b1c63

Merge branch 'master' of github.com:salesforce/lwc into jtu/dynamic-c…

a5c26e3

…omponents-template

jmsjtu commented Feb 9, 2023

View reviewed changes

jmsjtu mentioned this pull request Feb 9, 2023

feat(babel-plugin): dynamic components #3338

Merged

jmsjtu requested review from abdulsattar, caridy, jye-sf, nolanlawson, pmdartus and ravijayaramappa February 9, 2023 05:28

caridy reviewed Feb 9, 2023

View reviewed changes

...llup-plugin/src/__tests__/compilerConfig/fixtures/dynamicImportConfig/dynamicImportConfig.js Outdated Show resolved Hide resolved

ravijayaramappa reviewed Feb 9, 2023

View reviewed changes

fix: update based on PR feedback

d9caaed

jmsjtu commented Feb 10, 2023

View reviewed changes

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

test: update tests based on review feedback

856fd97

ravijayaramappa reviewed Feb 10, 2023

View reviewed changes

fix: refactor based on PR feedback and also add more unit tests

db95aed

nolanlawson reviewed Feb 15, 2023

View reviewed changes

nolanlawson approved these changes Feb 15, 2023

View reviewed changes

ravijayaramappa reviewed Feb 15, 2023

View reviewed changes

refactor: remove warning when lwc:component used without config

7be5c78

ravijayaramappa approved these changes Feb 16, 2023

View reviewed changes

divmain reviewed Feb 17, 2023

View reviewed changes

refactor: update based on PR feedback

98211cf

jmsjtu merged commit 093015d into jtu/dynamic-components Feb 20, 2023

jmsjtu deleted the jtu/dynamic-components-template branch February 20, 2023 00:50

jmsjtu mentioned this pull request Feb 20, 2023

test: dynamic components integration tests #3355

Merged

pmdartus reviewed Feb 21, 2023

View reviewed changes

jmsjtu mentioned this pull request Mar 3, 2023

feat: dynamic components #3381

Merged

		@@ -0,0 +1,11 @@
		<template>
		<lwc:component lwc:is={ctor1} lwc:if={visible.if}>

Comments

Conversation

jmsjtu commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

PR structure

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ravijayaramappa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravijayaramappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu commented Feb 9, 2023 •

edited

Loading

jmsjtu Feb 9, 2023 •

edited

Loading

ravijayaramappa Feb 16, 2023 •

edited

Loading

jmsjtu Feb 20, 2023 •

edited

Loading