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

POC: dynamic component template directive #3218

Closed
wants to merge 3 commits into from

Conversation

jmsjtu
Copy link
Member

@jmsjtu jmsjtu commented Dec 12, 2022

Details

This is a follow-up to #3204 and is a POC of how lwc:dynamic can work as a template directive.

This POC works very similarly to how the existing lwc:dynamic works but rather than using a placeholder tag name for the dynamic component, it uses an inferred name.

Most of the work is done by the compiler to wire up the template in a way that can fit into the current element creation process using the diffing algo.

How it works

The syntax for using the directive is as follows:

<template>
    <template lwc:dynamic={ctor} lwc:attributes={databag}></template>
</template>

Mimicking the current behavior, lwc:dynamic stores the constructor but because this directive now belongs to a template we'll need another way to assign attributes and event listeners.

One option to store these values is to use another directive, for example, lwc:attributes that houses the values and passes them to the dynamic component once it's been instantiated.

Alternatively we may be able to just use lwc:spread for this.

The output of the template function will look as follows:

import { registerTemplate } from "lwc";
const stc0 = {
  key: 0,
};
function tmpl($api, $cmp, $slotset, $ctx) {
  const { dc: api_dynamic_component } = $api;
  return [api_dynamic_component($cmp.ctor, $cmp.databag)];
  /*LWC compiler vX.X.X*/
}
export default registerTemplate(tmpl);
tmpl.stylesheets = [];

The difference from the current behavior is that we're no longer passing a tag name to the dc function.

The tag name can be inferred in a number of different ways but in this example, I've stored the name on the Ctor.

Note: When the rollup plugin runs the namespace and name are both available, these can be used to infer the name of the component.

The new DC function would look something like this:

function dc(
    Ctor: LightningElementConstructor | null | undefined,
    data: VElementData,
    children: VNodes = EmptyArray
): VCustomElement | null {
    ...
    if (!isComponentConstructor(Ctor)) {
        throw new Error(`Invalid LWC Constructor ${toString(Ctor)}.`);
    }

    return c(Ctor.sel, Ctor, data, children);
}

With this design, we can continue to use the current flow for generating dynamic components (through the diffing algo), as the main functionality of the dc function remains the same.

This also means that the logic to instantiate and insert dynamic elements will follow the same flow as custom elements do currently.

Considerations

Here are a few points about the design that needs fleshing out / finalizing.

  1. We need a way to store the attributes and event listeners in the template itself.
  2. We need a way to infer the tag name for dynamic components.
  • We could infer the name by asking the component author to provide one, we could generate a name using a naming scheme, or we could use the folder structure like in this example.

return c(sel, Ctor, data, children);
// We don't ahve to get the custom element name from the Ctor, we could store it in a weakmap or somewhere to retrieve it.
// Alternatively, we could generate a name or ask the component author to provide a name.
return c(Ctor.sel, Ctor, data, children);
Copy link
Member Author

Choose a reason for hiding this comment

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

For the purposes of the POC I needed a way to store the inferred tag name. I chose to store it on the Ctor but there are other options as well.

Comment on lines +35 to +37
if (isComponentConstructor(Ctor)) {
Ctor.sel = sel;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

In this POC registerComponent would store the tag name on the Ctor as an example.

@abdulsattar
Copy link
Contributor

<template>
    <template lwc:dynamic={ctor} lwc:attributes={databag}></template>
</template>

Two problems with this approach I see:

  1. Introducing lwc:attributes which is one more API to maintain and adds confusion to developers. IMO, it would be better if we allowed <template lwc:dynamic={ctor} attr1={attr1Value} attr2={attr2Value} lwc:spread={dynamicProps}></template>
  2. We have several restrictions the template tag, like no directives etc. If we just allow something like <lwc-dynamic> as the tag, we wouldn't have those issues.

@jmsjtu
Copy link
Member Author

jmsjtu commented Dec 13, 2022

@abdulsattar Thanks for the feedback!

I discussed this with @ravijayaramappa and I think it may be better to only allow assigning props through lwc:spread for dynamically created components if we go with a <template> tag.

The idea is to stay consistent with respect to how the component is created. When the component is created programmatically with lwc:dynamic it should have props assigned programmatically as well.

That does leave a gap in assigning event handlers to the template though.

Using a pre-defined element like <lwc-dynamic> is a good idea, it would avoid the restrictions we're running into with <template>.

The reason I went with <template> is because I thought using a <template> clearly indicates the tag won't get rendered in the final output.

I'll add this feedback to the RFC.

@pmdartus
Copy link
Member

The reason I went with <template> is because I thought using a <template> clearly indicates the tag won't get rendered in the final output.

Something that I raised during the last sync that I would like to capture here is that the <template> tag has been used until now as a way to denote a fragment. Which is in line with the original intent of the <template> element. Here is a quote from the HTML specification.

The template element is used to declare fragments of HTML that can be cloned and inserted in the document by script.

IMO, it would be preferable to use a different tag name to denote a custom element if we pursue this route. I would vote for using the lwc:dynamic tag name (note that is using a : instead of a - which isn't a valid custom element name) to make it stand out from the rest of the tags in the templates.

@nolanlawson
Copy link
Collaborator

@pmdartus If we are going to use <lwc:dynamic>, then we may want to put in a mechanism to squat the lwc: "namespace". (I.e. prevent people from using it.) Because it is possible to do this today:

document.createElement('lwc:dynamic')

@jmsjtu
Copy link
Member Author

jmsjtu commented Dec 13, 2022

@pmdartus thanks for pointing this out, I agree the usage of dynamic components in this POC does not represent an HTML fragment.

I think to make a <template> work for dynamic components then we'd want to do something like this:

<template lwc:dynamic>
  {insertDynamicElementHere}
</template>

Additionally, I interpreted this line from the HTML spec:

the template element represents nothing.

to mean the <template> tag should not directly map to a DOM element but by placing attributes and event listeners on it we are treating it as if it were a DOM element.

Based on these two observations, I think going with a placeholder tag like lwc:dynamic makes more sense.

Going a step further, I wonder if making a lwc-dynamic custom element similar to DynamicLightningElement but where it is inserted and removed from the DOM when the constructor changes would be more flexible.

This could have the benefit of allowing for intermediate content to be displayed, like a loading screen, while the constructor is being retrieved.

I'll include this feedback in the RFC.

@nolanlawson would using

document.createElement('lwc:dynamic')

cause any issues? Could doing that interfere with the structure of the vnodes?

@nolanlawson
Copy link
Collaborator

@jmsjtu It's not that it would mess with the vnodes; it's just that it feels icky if folks on-platform are able to create their own <lwc:*> elements if we're going to "reserve" that prefix.

@pmdartus
Copy link
Member

then we may want to put in a mechanism to squat the lwc: "namespace".

I am not sure to understand why we would need to squat the lwc: namespace. I don't expect the LWC engine to create an actual element with the <lwc:dynamic? tag name. This special element in the component template is a signal from the LWC compiler that the given element will be replaced at runtime with a dynamic component at runtime. The engine would still use sel property set by the registerComponent to create the component with the correct tag name similar to how this PR does it.

I also don't think that we should prevent developers from creating elements using document.createElement with the prefix since it's a compile-time signal. To make this obvious for developers we can certainly add a DEV-only warning indicating that creating <lwc:dynamic> element at runtime is a no-op from the engine perspective.

Are you concerned that developers might already have HTML elements in their templates using the lwc: prefix?

to mean the <template> tag should not directly map to a DOM element but by placing attributes and event listeners on it we are treating it as if it were a DOM element.

I don't think this is the correct way to interpret the HTML specification. In the context of the spec, interpret refers to the semantic of the DOM node (interpret definition). If you look at how the same term is applied to other DOM elements like <nav>, <ol>, interprets defines the intrinsic meaning of an element. The <template> is a real DOM node, but it doesn't carry any semantic value in the document.

Going a step further, I wonder if making a lwc-dynamic custom element similar to DynamicLightningElement but where it is inserted and removed from the DOM when the constructor changes would be more flexible.

Yes, it's also another avenue worth pursuing. I am not married to the idea of using a special tag name for dynamic components. The main challenge we discussed in the past is how we ensure that the LWC engine is in control of the <lwc-dynamic> custom element.

@nolanlawson
Copy link
Collaborator

Are you concerned that developers might already have HTML elements in their templates using the lwc: prefix?

No, I just want to avoid any confusion. A dev warning at runtime is fine.

I am agnostic to what the developer-facing syntax looks like, but I much prefer handling this in the compiler rather than at runtime. It just seems like less code overall to maintain, and less potential for accidentally exposing subtle API surface at runtime. Hence why I prefer this proposal to #3204.

@jmsjtu
Copy link
Member Author

jmsjtu commented Dec 15, 2022

The <template> is a real DOM node, but it doesn't carry any semantic value in the document.

Thanks for clearing this up @pmdartus!

@caridy
Copy link
Contributor

caridy commented Dec 15, 2022

Ok, it feels that all of you are inching to the solution that I'm proposing, which is a good thing IMO.

@pmdartus the signal should be an attribute, not the name... that way we avoid the whole question about what the name should be... and how to query that name, etc. In the other proposal, the attribute lwc:dynamic is the signal for the compiler to treat that vnode very differently.

@nolanlawson @jmsjtu the talking point is really about re-creating of the element vs re-using the element. The implications for me are very clear, either you put the pressure on the framework (LWC) or you put it on the developer, someone has to track down the replacement of the element if they want to hold a reference, or add a manual listener, etc. Who is going to be, is the real question. In this case, I'm leaning toward the framework to take care of that, but that's just an opinion.

@caridy
Copy link
Contributor

caridy commented Dec 15, 2022

There is one more thing that I don't think has surface in this discussion, it is the mismatches between constructors, and the implications for allocation of children.

@jmsjtu jmsjtu closed this Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants