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

adding dynamic component configuration rfc #48

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alejandro-sf
Copy link

@alejandro-sf alejandro-sf commented Apr 12, 2021

@alejandro-sf
Copy link
Author

@jodarove @pmdartus @diervo please consider this RFC for dynamic component configuration (lwc:bind)

@nolanlawson
Copy link
Contributor

@alejandro-sf It looks like something is wrong with the formatting? GitHub is not able to render the markdown correctly: rendered view

@alejandro-sf
Copy link
Author

Oh I didn't know the markdown was visible even from the review...I'll update to fix I used the quip export to markdown feature so there's probably something wrong there

@alejandro-sf
Copy link
Author

Please see the new formatting here

text/0114-dynamic-component-configuration.md Show resolved Hide resolved
this.loadCtor();

async loadCtor() {
const ctor = await import(this.moduleName);
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave dynamic import aside from this proposal. How the component constructor is loaded is independent of how the attributes, properties, event listeners are passed.


get configurations() {
return {
ctor: this.customCtor,
Copy link
Member

Choose a reason for hiding this comment

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

LWC already implements the lwc:is directive to dynamically pass a component constructor at runtime. Accepting a ctor property creates a duplicate. IMO this proposal should work both for static and dynamic component constructors.

Choose a reason for hiding this comment

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

Does that mean you want 'ctor' to be renamed as 'is' or is there another mechanism for this?

Copy link
Member

Choose a reason for hiding this comment

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

It's not the lwc:is but rather the lwc:dynamic that I was referring to above: https://github.com/salesforce/lwc-rfcs/blob/master/text/0110-dynamic-components.md#api

</template>
```
```js
import { LightningElement, api } from "lwc";
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid JavaScript code block. I would also recommend making this example as minimal as possible to make it straightforward to understand.

Choose a reason for hiding this comment

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

Should we pare this down? I wanted to make sure we had a compelling usecase for each of the 4 things you can configure in 1 place.

Copy link
Member

Choose a reason for hiding this comment

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

I would keep the example in the RFC itself as simple a possible. You can add more complex examples at the end in an appendix for example this way is doesn't distract from the actual content of the RFC.

Before this proposal, dynamic component creation was done using the `lwc:dynamic` directive. This directive required a component author to know component configuration details like the property and event names at design time which breaks down for use cases that need true dynamic component creation. For instance, in the case of property values or event handlers one would have to keep a reference to the element and set the information retrieved at runtime manually.

```js
element;
Copy link
Member

Choose a reason for hiding this comment

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

This code block is not valid JavaScript.


## Proposal

The proposal involves adding a new directive that captures the relevant information to configure a web component at runtime. The assumption is that this directive will be used in cases where the component module is unknown at runtime (therefore used alongside dynamic imports) and zero or more of the following are unknown at runtime: properties, attributes, or event handlers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The proposal involves adding a new directive that captures the relevant information to configure a web component at runtime. The assumption is that this directive will be used in cases where the component module is unknown at runtime (therefore used alongside dynamic imports) and zero or more of the following are unknown at runtime: properties, attributes, or event handlers.
The proposal involves adding a new directive that captures the relevant information to configure a web component at runtime. The assumption is that this directive will be used in cases where the component constructor is unknown until runtime and zero or more of the following are unknown until runtime: properties, attributes, or event handlers.


The directive `lwc:bind` can retrieve component configuration data via a JavaScript object with a pre-defined shape. Constructor is the only required property in the configuration data.

## General Invariants
Copy link
Member

Choose a reason for hiding this comment

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

There is a mix of actual invariants and design details in this section.

For example, Changes to the configuration object must be reflected in the component is not an invariant of this proposal but rather a design decision based on the proposed solution. I would recommend breaking this section apart and specifying each individual behavior.


* Changes to the configuration object must be reflected in the component (ie changing the properties will cause component properties to be set again)

* Changing the constructor on a parent component will also trigger a cascade re-render of any child components
Copy link
Member

Choose a reason for hiding this comment

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

Changing a constructor requires unmounting the existing custom element and creating a new custom element. It actually doesn't re-render the children elements but creates brand new children elements.


This feature will be available in Salesforce Lightning Platform and LWR to certain teams depending on their use cases

## Unresolved Questions
Copy link
Member

Choose a reason for hiding this comment

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

Questions that are not covered by this proposal:

  • Can the lwc:bind directive be used with other directives, attributes and event handlers?
  • If the response to the previous answer is yes, what happens when the lwc:bind contains an attribute that is already defined via the template. For example: <x-foo lwc:bind={config} class="error"></x-foo> with config = { attributes: { class: 'success' } } ?
  • In which order properties and attributes are set? If the config is defined as config = { attributes: { class: 'error' }, properties: { className: 'success' } } what is the expected output?
  • How are attributes values treated? How to add and remove an attribute? What is the semantic of the following attribute values: null, undefined, true and false?

Copy link
Contributor

Choose a reason for hiding this comment

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

what about special properties, like innerHtml, outerHtml, innerText, etc? Will they be allowed?

There's already a [inner html binding RFC] (#15), which states some security concerns. Open question: Does this lwc:bind proposal overlaps with lwc:inner-html?

```
A new tool must be introduced that will allow component authors to configure their components with information retrieved at runtime without relying on maintaining element references or manually manipulating the DOM.

## Proposal
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add a section about how other frameworks are approaching this issue.

@salesforce-cla
Copy link

Thanks for the contribution! It looks like @wanyanxu is an internal user so signing the CLA is not required. However, we need to confirm this.

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Alejandro Lopez <a***@A***.i***.s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated.

Comment on lines +68 to +69
@wire(myAdapter, { id: 'someDashboardId' }) {
chartDefinitions({ data, error }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@wire(myAdapter, { id: 'someDashboardId' }) {
chartDefinitions({ data, error }) {
@wire(myAdapter, { id: 'someDashboardId' })
chartDefinitions({ data, error }) {

@api moduleName;

connectedCallback() {
const ctor = await import(this.moduleName);
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid js

Copy link
Author

Choose a reason for hiding this comment

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

Is this because await must be in an async function?

@pmdartus pmdartus added the draft label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants