feat(babel-plugin): dynamic components by jmsjtu · Pull Request #3338 · salesforce/lwc

jmsjtu · 2023-02-09T04:59:34Z

Details

This PR implements the changes to the babel-plugin and runtime from the dynamic components RFC.

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

Part 1 (template compiler 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:

ea52bae
a4699e1

This should reduce the number of files changed significantly.

Does this pull request introduce a breaking change?

✅ No, it does introduce a breaking change.

Does this pull request introduce an observable change?

🚨 Yes, it does not introduce an observable change.

These are the observable changes:

The module resolver will now return a type field.
The Id in the @lwc/rollup-plugin will include a alias specifier as a query string for module entries that use aliases.
The registerComponent function will now register an additional field called sel.

GUS work item

W-12190514

…omponents-babel

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

jmsjtu · 2023-02-09T05:25:36Z

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

+        // The only scenario where the n1.sel and n2.sel are different is when the constructor
+        // of a dynamic custom element has changed.
+        // ex: <lwc:component lwc:is={ctor}>
+        if (!isSameVnode(n1, n2) && !areDynamicVCustomElements(n1, n2)) {


I thought about having the new dynamic component vnodes only go through the updateDynamicChildren path but I think that it can also fit into the updateStaticChildren path, where this will be invoked.

With the new dynamic components, the value of vnode.sel will change when the constructor changes meaning this condition will fail in debug mode.

However, the main reason behind allowing dynamic components to use the updateStaticChildren path is that although the value of sel is not the same on update, its position within the child array will not change.

Eventually, when the vnode reaches the patchCustomElement routine, it checks to see if the ctors are equivalent and unmounts/mounts them.

This path is triggered whenever the dynamic components constructor is changed (same path that the current lwc:dynamic follows):

<template> <lwc:component lwc:is={lazyCtor}></lwc:component> </template>

import { LightningElement } from 'lwc' export default class extends LightningElement { lazyCtor; async connectedCallback() { const { default: ctor } = await import('x/foo'); lazyCtor = ctor; } }

It might be worth noting in a comment that we expect the node to be unmounted/remounted when the sel changes. Otherwise it might seem like the props from one component will be patched onto a different component, which is not at all what's happening here.

@nolanlawson I updated the comment in the latest commit, let me know if it's clear enough.

@caridy would love to get your thoughts on this part of the diffing algo.

@jmsjtu Per existing doc, dynamic components should be considered as dynamic for the purposes of diffing. I found these two references

https://github.com/salesforce/lwc/blob/v2.37.3/packages/%40lwc/engine-core/src/framework/api.ts#L572

feat: dynamic imports (experimental flag) #1397 (comment)

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

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

nolanlawson · 2023-02-15T00:24:10Z

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

+    const params = new URLSearchParams(query);
+    params.set('specifier', specifier);
+    return `${filename}?${params.toString()}`;
+}


Are we going to have to make this change in lwc-platform, lwc-webpack-plugin, lwc-test, and LWR as well? Usually when we add new query params, we have to.

Looking through these repositories, it looks like lwc-webpack-plugin and lwc-test will need a similar update.

lwc-platform invokes the transformers directly and passes in the namespace / name that's provided as part of the bundle so there's no need for LWC to scan the file directory.

It looks like the LWR code just calls the rollup plugin directly, so they should be able to use the plugin as is.

nolanlawson · 2023-02-15T00:25:59Z

These are the observable changes:

The module resolver will now return a type field.

The Id in the @lwc/rollup-plugin will include a alias specifier as a query string for module entries that use aliases.

The registerComponent function will now register an additional field called sel.

None of these changes seem to be something that will break users in practice. These are all quasi-internal APIs that are mostly only used by us and certain sister repos.

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

…to jtu/dynamic-components-babel

ravijayaramappa

minor questions, otherwise LGTM

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

ravijayaramappa · 2023-02-23T06:33:26Z

packages/@lwc/babel-plugin-component/src/component.js


+function getComponentRegisteredName(t, state) {
+    const { namespace, name } = state.opts;
+    const componentName = namespace && name ? `${namespace}-${name}` : '';


Minor note: Should this default to something other than an empty string? My suggestion is based on two things

The comment here that in the future an empty sel will throw. So namespace and name will both become required fields

Within salesforce namespace is familiar field and platform users will know to use a default value like c. OTOH, OSS users will have to contemplate what the namespace value needs to be.

Having said that, we are in a tough spot here. We don't have anything reliable to use for the component name. node.id is not always present. ~~May be make name a required field and default namespace to c or lwc?~~

Took a second look: per the @lwc/compiler README , name and namespace are required, but the fields in the TS definition TransformOptions are both optional. So my earlier suggesting of making it required will be a breaking change.

Took a second look: per the @lwc/compiler README , name and namespace are required, but the fields in the TS definition TransformOptions are both optional. So my earlier suggesting of making it required will be a breaking change.

I agree this is a tricky one. I think in practice if making the namespace and name required breaks any OSS customers, their components would already have issues, as those fields are used to generate the style tokens.

The only reason we need the component's name right now is for dynamic components.

My thinking was that by defaulting to an empty string, we could avoid the breaking change and just prevent components that don't have a registered name from being dynamically created.

The component author will have to correct the namespace and name if they want to dynamically create the component (this is most likely only an issue for OSS).

How would you feel if we left it as an empty string for now and make the namespace and name required once versioning comes out or in LWC v3?

This would line up the compiler flags with the description in the README and we wouldn't have to worry about defaulting to anything.

ravijayaramappa · 2023-02-23T07:18:05Z

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

    }

-    return null;
+    const sel = getComponentRegisteredName(Ctor);


For my understanding: The sel needs to be looked up from the Ctor here, at runtime, because the sel depends on the Ctor's metadata(name & namespace) and the template-compiler does not have this info ahead of time. Add a comment to that effect 🙂

Good call, added some comments. Let me know if it's clear enough.

ravijayaramappa · 2023-02-23T07:35:33Z

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

+        // The only scenario where the n1.sel and n2.sel are different is when the constructor
+        // of a dynamic custom element has changed.
+        // ex: <lwc:component lwc:is={ctor}>
+        if (!isSameVnode(n1, n2) && !areDynamicVCustomElements(n1, n2)) {


@jmsjtu Per existing doc, dynamic components should be considered as dynamic for the purposes of diffing. I found these two references

https://github.com/salesforce/lwc/blob/v2.37.3/packages/%40lwc/engine-core/src/framework/api.ts#L572

feat: dynamic imports (experimental flag) #1397 (comment)

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

…StaticChildren

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

jmsjtu · 2023-02-25T06:34:29Z

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

+                        if (
+                            n1.type === VNodeType.CustomElement &&
+                            n2.type === VNodeType.CustomElement
+                        ) {
+                            if (n1.ctor !== n2.ctor) {
+                                // If the constructors differ, unmount the current component and mount a new one using
+                                // the new constructor. This scenario only occurs for dynamic components.
+                                const dynElmRenderer = n2.data.renderer ?? renderer;
+                                const dynElmAnchor = dynElmRenderer.nextSibling(n1.elm);
+
+                                unmount(n1, parent, dynElmRenderer, true);
+                                mountCustomElement(n2, parent, dynElmAnchor, dynElmRenderer);
+                            }
+                        }
+                    }


This logic comes straight from patchCustomElements and is the logic that runs in the current diffing algo logic for dynamic components.

This logic was needed for lwc:dynamic because in the dynamic diffing algo the checks for isSameVnode would always trigger true. Thus if a new constructor was supplied, it would run patchCustomElement and need to unmount and mount the new component using the same tag name.

In the new implementation, isSameVnode will never be true when the dynamic component changes (the value of sel will be different).

Additionally, with the changes made in #2966 the dynamic components can now travel through the static path. This is allowed because we now consider the dynamic component before and after the constructor is changed to be equivalent.

Having said that, I've moved this logic out of patchCustomElement into updateStaticChildren because this path is still triggered when the constructor changes for a dynamic component. With the new implementation, when the constructors change, the dynamic components vnodes should still be considered equivalent (as they are in lwc:dynamic).

The logic is not needed in patchCustomElement anymore because the part of the dynamic diffing algo that relies on isSameVnode will not trigger true when the dynamic component's constructor changes in the new implmentation.

An alternative approach could be to emit a fragment for each dynamic component but I chose to avoid this because each fragment is wrapped in empty text nodes. I felt that the empty text nodes were unnecessary in this case since there will only ever be a single node.

@pmdartus @nolanlawson @ravijayaramappa would love to get your thoughts on the ☝️ if you get a chance 😃

The logic is not needed in patchCustomElement anymore because the part of the dynamic diffing algo that relies on isSameVnode will not trigger true when the dynamic component's constructor changes in the new implmentation.

I don't fully understand what you mean by the "the part of the dynamic diffing algo that relies on isSameVnode".

Overall, I don't think the dynamic component diffing logic belongs to the updateStaticChildren. How custom elements are patched should be an internal detail of the patchCustomElement.

Overall, I don't think the dynamic component diffing logic belongs to the updateStaticChildren. How custom elements are patched should be an internal detail of the patchCustomElement.

My reading is that this isn't really about dynamic components per se – it's just that the diffing algo never had to deal with diffing two nodes with different tagNames at this point in the code before.

Maybe the question is: Why is this code even needed?

if ( n1.type === VNodeType.CustomElement && n2.type === VNodeType.CustomElement )

I.e. why check that they are both custom elements? If one is a div and the other is an input, we still want to completely unmount one and mount the other one. We do need to check if the second is a custom element, though, so we can call mountCustomElement instead of mountElement.

I don't fully understand what you mean by the "the part of the dynamic diffing algo that relies on isSameVnode".

@pmdartus I was referring to the parts of updateDynamicChildren where isSameVnode is invoked, it's possible for the dynamic component (using the implementation for lwc:dynamic) to enter this path and traverse through patchCustomElement when the constructor changes.

This would cause the component to be recreated inside patchCustomElement because although the constructor changes the tag name will be the same (isSameVnode check passes).

What I was referring to is, in the new implementation isSameVnode won't pass the check when the constructor changes because the tag name will be different and therefore the only reason patchCustomElement has the logic for unmount / mounting is to be used with updateStaticChildren.

Here's an example using lwc:dynamic vs lwc:is.

My reading is that this isn't really about dynamic components per se – it's just that the diffing algo never had to deal with diffing two nodes with different tagNames at this point in the code before.

Yup, the reason I moved this logic out of the patchCustomElements routine is that the updateStaticChildren algo has never needed to diff against nodes when the tag names are different.

Maybe the question is: Why is this code even needed?
I.e. why check that they are both custom elements? If one is a div and the other is an input, we still want to completely unmount one and mount the other one. We do need to check if the second is a custom element, though, so we can call mountCustomElement instead of mountElement.

Originally I moved this out of patchCustomElements because I didn't see a need for it to do the unmounting / mounting logic when it could be done here. The reason being that updateDynamicChildren will not invoke patchCustomElements and enter the code path to unmount / mount the constructor in the new implementation.

This leaves only updateStaticChildren that would need to do the unmounting / mounting. However, now that I take a second look at updateStaticChildren, it doesn't have any specifics about a type of vnode, but works with vnodes in general. The logic for specifically handling vnodes types (VCustomElement, VElement, etc.) is done in patch.

I decided to move the logic back to patch for this reason. Although, I think in the future if we find a need to diff elements against tag names that have changed, we may want to update the rest of the patch methods to account for this.

~~Right now all the other patch functions only apply the attributes and props to their respective elements while patchCustomElement will also unmount / mount the element.~~

I've also updated the dev error here since patching a dynamic components no longer have the same tag names.

why check that they are both custom elements? If one is a div and the other is an input, we still want to completely unmount one and mount the other one.

@nolanlawson based on your feedback I updated the unmounting / mounting logic in updateStaticChildren to be agnostic of the type that is a mismatch.

With this implementation, patch now serves the sole purpose of only updating the same element.

Let me know what you think, I'm not sure if this is a breaking change or not. My thoughts are that it shouldn't be.

If the tests still pass, then it's probably not a breaking change. Nucleus also tends to catch diffing algo changes that cause downstream breakages.

Based on the review this morning, the team decided to isolate the changes in unmounting / mounting of the dynamic component to patchCustomElement.

In a future PR, we will introduce the unmounting / mounting in updateStaticChildren to help isolate any potential future regression.

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

jmsjtu · 2023-02-25T06:37:20Z

Per existing doc, dynamic components should be considered as dynamic for the purposes of diffing. I found these two references

@ravijayaramappa I don't think this is a strict requirement anymore after #2966.

I left a comment here explaining the details, let me know if it's clear.

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

pmdartus · 2023-02-27T13:20:25Z

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

+                        if (
+                            n1.type === VNodeType.CustomElement &&
+                            n2.type === VNodeType.CustomElement
+                        ) {
+                            if (n1.ctor !== n2.ctor) {
+                                // If the constructors differ, unmount the current component and mount a new one using
+                                // the new constructor. This scenario only occurs for dynamic components.
+                                const dynElmRenderer = n2.data.renderer ?? renderer;
+                                const dynElmAnchor = dynElmRenderer.nextSibling(n1.elm);
+
+                                unmount(n1, parent, dynElmRenderer, true);
+                                mountCustomElement(n2, parent, dynElmAnchor, dynElmRenderer);
+                            }
+                        }
+                    }


The logic is not needed in patchCustomElement anymore because the part of the dynamic diffing algo that relies on isSameVnode will not trigger true when the dynamic component's constructor changes in the new implmentation.

I don't fully understand what you mean by the "the part of the dynamic diffing algo that relies on isSameVnode".

Overall, I don't think the dynamic component diffing logic belongs to the updateStaticChildren. How custom elements are patched should be an internal detail of the patchCustomElement.

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

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

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

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

jmsjtu · 2023-02-27T23:44:08Z

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

+            // Currently the only scenario when patch does not receive the same vnodes are for
+            // dynamic components. This is why the final check here ensures n2.type is a custom element.
+            // When a dynamic component's constructor changes, the value of its tag name (sel) will be
+            // different. The engine will unmount the previous element and mount the new one using the
+            // new constructor in patchCustomElement.
+            n2.type !== VNodeType.CustomElement


I tried to make this comment as clear as possible but what I was going for was, we have a way to handle when the vnodes are not the same (different sel and key) for custom elements. We don't for the other types so we throw an error on those ones.

In this case, don't we also expect n1.type to be VNodeType.CustomElement?

Ah you're right, I was thinking it would be enough to only check that n2.type is a custom element because it's what's changing and we'd be replacing it in patchCustomElement. But if n1.type is not a custom element, patchCustomElement will break as well.

I've updated the validation logic in the latest commit.

I've updated the logic so this should no longer be needed, see my previous comment.

nolanlawson · 2023-02-27T23:48:13Z

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

                        if (type === RegistryType.alias) {
                            // specifier must be in in namespace/name format
-                            const hasValidSpecifier = specifier.split('/').length > 1;
+                            const hasValidSpecifier = specifier.split('/').length === 2;


This doesn't handle strings like bar/ and /foo correct? Their length would be 2, even though they're invalid.

Sorry about that I missed the previous comment. I've updated the logic to account for variations of invalid alias specifiers.

Also want to note that a relative import (starting with a './' or '/' is not subject to the aliasing, as they would be using the actual file path, ie ./foo.js which does not use an alias.

Here's the updated logic + unit tests.

…specifiers

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

…hildren" This reverts commit 1398300.

jmsjtu added 6 commits February 8, 2023 16:06

feat: update babel plugin for dynamic components

a6197d5

feat: update runtime for dynamic components

33ceb00

test: add unit tests

ea52bae

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

4a7a78f

…omponents-babel

feat: update rollup plugin to gather namespace and name on alias entries

621c5eb

test: add unit tests for rollup plugin

a4699e1

jmsjtu commented Feb 9, 2023

View reviewed changes

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

jmsjtu commented Feb 9, 2023

View reviewed changes

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

jmsjtu commented Feb 9, 2023

View reviewed changes

jmsjtu requested review from abdulsattar, caridy, nolanlawson, pmdartus and ravijayaramappa February 9, 2023 05:27

nolanlawson reviewed Feb 15, 2023

View reviewed changes

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

nolanlawson reviewed Feb 15, 2023

View reviewed changes

packages/@lwc/rollup-plugin/src/index.ts Show resolved Hide resolved

nolanlawson reviewed Feb 15, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/component.ts Outdated Show resolved Hide resolved

jmsjtu added 2 commits February 15, 2023 15:57

refactor: added better comments for patching algo and updated unit tests

47973e0

Merge branch 'jtu/dynamic-components' of github.com:salesforce/lwc in…

6860d56

…to jtu/dynamic-components-babel

jmsjtu mentioned this pull request Feb 20, 2023

test: dynamic components integration tests #3355

Merged

jmsjtu requested a review from divmain February 20, 2023 02:05

ravijayaramappa reviewed Feb 23, 2023

View reviewed changes

refactor: move dynamic component update logic out of patch for update…

ad169da

…StaticChildren

jmsjtu commented Feb 25, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/component.ts Outdated Show resolved Hide resolved

jmsjtu commented Feb 25, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved

jmsjtu requested a review from ravijayaramappa February 25, 2023 06:40

jmsjtu requested a review from nolanlawson February 25, 2023 06:40

pmdartus reviewed Feb 27, 2023

View reviewed changes

nolanlawson reviewed Feb 27, 2023

View reviewed changes

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

nolanlawson reviewed Feb 27, 2023

View reviewed changes

packages/@lwc/rollup-plugin/src/index.ts Outdated Show resolved Hide resolved

nolanlawson reviewed Feb 27, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/component.ts Show resolved Hide resolved

divmain reviewed Feb 27, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/vnodes.ts Outdated Show resolved Hide resolved

refactor: move diffing logic back to patchCustomElement

b7dcd96

jmsjtu requested review from divmain, nolanlawson and pmdartus February 27, 2023 23:40

jmsjtu commented Feb 27, 2023

View reviewed changes

nolanlawson reviewed Feb 27, 2023

View reviewed changes

jmsjtu added 3 commits February 27, 2023 23:05

refactor: update error message logic for diffing algo

74e10f0

refactor: update logic in rollup-plugin to account for invalid alias …

185f71b

…specifiers

refactor: update mounting / unmounting logic to updateStaticChildren

1398300

nolanlawson approved these changes Feb 28, 2023

View reviewed changes

nolanlawson reviewed Feb 28, 2023

View reviewed changes

packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved

jmsjtu added 3 commits February 28, 2023 11:48

Revert "refactor: update mounting / unmounting logic to updateStaticC…

d0333ee

…hildren" This reverts commit 1398300.

refactor: dont use importer alias for imported html and css

7c0ddff

fix: update comment

570d69a

jmsjtu merged commit 44fe1b6 into jtu/dynamic-components Mar 1, 2023

jmsjtu deleted the jtu/dynamic-components-babel branch March 1, 2023 04:16

jmsjtu mentioned this pull request Mar 3, 2023

feat: dynamic components #3381

Merged

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!

Uh oh!

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!

nolanlawson Feb 15, 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!

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!

nolanlawson commented Feb 15, 2023

Uh oh!

Uh oh!

ravijayaramappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu Feb 23, 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!

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!

jmsjtu Feb 25, 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!

jmsjtu Feb 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu Feb 27, 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!

jmsjtu Feb 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmsjtu commented Feb 25, 2023

Uh oh!

jmsjtu commented Feb 9, 2023 •

edited

Loading

jmsjtu Feb 9, 2023 •

edited

Loading

nolanlawson Feb 15, 2023 •

edited

Loading

jmsjtu Feb 23, 2023 •

edited

Loading

jmsjtu Feb 25, 2023 •

edited

Loading

jmsjtu Feb 25, 2023 •

edited

Loading

jmsjtu Feb 27, 2023 •

edited

Loading

jmsjtu Feb 28, 2023 •

edited

Loading

jmsjtu Feb 27, 2023 •

edited

Loading

jmsjtu Feb 28, 2023 •

edited

Loading

jmsjtu Feb 28, 2023 •

edited

Loading