feat(engine): api to build custom elements #87
Conversation
|
Is the sample code supposed to be this? import { customElement } from "engine";
import LightningAvatar from "lightning-avatar";
const AvatarElement = customElement(LightningAvatar);
customElements.define('x-foo', AvatarElement); |
|
good catch @deadpoetJBA, updated! |
| ArrayPush.call(attrs, attrName); | ||
| }); | ||
| } | ||
| return class extends HTMLElement { |
There was a problem hiding this comment.
With the current implementation, the constructor name will be HTMLElement. Since the compiler enforce the component class to be named, what about dynamically set the custom element class name to match the component class name? This would greatly improve the debugability.
export function customElement() {
// ...
let LwcCustomElement = class extends HTMLElement {
// ...
};
Object.setProperty(LwcCustomElement, 'name', { value: Ctor.name });
return LwcCustomElement;
}There was a problem hiding this comment.
this constructor is never used by the engine, it is only used by the DOM apis via the custom element regsitraion and instantiation mechanism, not need to add a name here.
|
I ran several unsuccessful tests starting from scratch so the I decided to simply add a new component to the todo-mvc app with as less changes as possible. Here is my fork: https://git.soma.salesforce.com/priand/lwc-todo-mvc/tree/wc-components
Here is what I found so far:
|
|
@priand that's good inside, I know what I have to do! I will update this PR today. |
|
@priand can you take it for another spin? I tried this locally, and it works fine for me now after fixing the LWC registration order. You just need to pull from the branch, run |
|
Ok, it works! I tried several initialization orders and they all worked. Good job! |
|
@priand yes, working to resolve that in another PR since it doesn't need to be part of the experimental, but a general fix all together. |
caridy
force-pushed
the
caridy/wc-customElement
branch
from
97e06f1 to
99bdbf8
Compare
|
@priand can you try this again? it should be fine now to compose registered elements. No more duplication of content, and no need to load the same component twice. |
| import { Element } from "../main"; | ||
| import { customElement } from "../wc"; | ||
|
|
||
| describe('wc', () => { |
There was a problem hiding this comment.
notice that we can't add more tests here because jsdom doesn't support custom elements ATM.
Benchmark comparisonBase commit:
|
|
@caridy Thanks Caridy! A quick test this morning shows that there is no longer duplicated content. |
|
I created a library that exports some simple components and consume them in applications (basic html, Angular...). (internal link removed) The web components run properly from the testapp, defined in the library project, but not from the external apps consuming the library. At runtime, the engine complains about the "key" while there is no iterator involved. It is not clear to me if this is an issue with the way the components are packaged (using the iife version here) or an issue with the engine. |
|
@deadpoetJBA Our master branch has a breaking change in the template compiler. Each virtual node generated has now a unique |
|
@pmdartus That did the trick. Thanks Pierre-Marie. |
|
@priand excellent! btw, remember this is a to-be-open repo, any link or internal information should not be posted in issues. I have updated your comment to remove the internal links. |
|
Hi @caridy - Are you going to merge this branch soon? I'm still using it but this makes me behind compared to master. Moreover, have you made some progress on the attributes as well as the content slots? Thanks! |
|
@priand There are no plans to merge this branch into master. However, I just merged master back into this branch so you can be up to date. LMK if you come across anything. |
|
Ok, thanks @davidturissini, I'll use that. By "there is no plan to merge this branch", do you mean that the creation of custom elements that can be used from other frameworks (we are using Angular) is now out of scope? |
|
@priand Using custom elements under the hood is something we want to do. What I mean is that there is no timeline for merging this branch right now and its unclear (to me, at least) when this would get merged. |
|
We will definitely have to wait after 214 release. |
caridy
force-pushed
the
caridy/wc-customElement
branch
from
72ecce5 to
0353057
Compare
|
@priand I have updated this PR to match the current master (0.22) which enables the usage of slots :), enjoy it! |
|
Closing this PR since after talking to Philip we can get away with way simpler solution for now. |
|
@diervo there must be a misunderstanding here, as we currently use the custom elements. Our server actually generates a page that contains the custom tags and also use slots. Only the root element was using createElement(), but we found this morning a solution to not require that. Is there a way to get the code that enables the custom elements registry merged into the master branch? |
|
Lets have another sync, I really don't think you need any of this for your use case as far as I understood it the other day |
|
@diervo As of now, our JSP pages generate an hierarchy of custom elements. What we showed you last week was only the root element created using a JavaScript call. But we overcame our initial issue and we removed that piece of code: we are now fully creating the pages using custom elements. |
|
I have discussed this with @diervo and we will be merging this soon. @davidturissini @ekashida let's try to clean this up and add some integration tests since unit tests can't run Custom Elements today, so we can merge this. |
caridy
force-pushed
the
caridy/wc-customElement
branch
from
0353057 to
23edbd1
Compare
caridy
changed the title
| Ctor = resolveCircularModuleDependency(Ctor); | ||
| const def = getComponentDef(Ctor); | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.isTrue(isUndefined(Ctor.forceTagName), `The experimental support for web components does not include the support for \`static forceTagName\` to "${Ctor.forceTagName}" declaration in the class definition for ${Ctor}.`); |
There was a problem hiding this comment.
banning forceTagName since we can't really support this without the is attribute in safari.
There was a problem hiding this comment.
We will need to get ahead of this breaking change in core. Can you add this to #423 ?
There was a problem hiding this comment.
No no, this is only for buildCustomElementConstructor's 2nd argument. This does not affect anything else. Since we always extends HTMLElement in this factory method, there is no way to force or use the is attribute whatsoever.
| } | ||
| // adding all public descriptors to the prototype so we don't have to | ||
| // do it per instance in html-element.ts constructors | ||
| defineProperties(LightningWrapperElement.prototype, def.descriptors); |
| @@ -152,15 +152,15 @@ export function removeVM(vm: VM) { | |||
| patchShadowRoot(vm, []); | |||
| } | |||
|
|
|||
| interface ShadowRootInit { | |||
| export interface CreateVMInit { | |||
There was a problem hiding this comment.
ts was complaining that isRoot and fallback were not part of the standard ShadowRootInit interface.
| @@ -92,7 +92,7 @@ export function evaluateTemplate(vm: VM, html: Template): Array<VNode|null> { | |||
| } | |||
|
|
|||
| // TODO: add identity to the html functions | |||
| const { component, context, cmpSlots = EmptySlots, cmpTemplate } = vm; | |||
There was a problem hiding this comment.
@pmdartus this is what I mentioned this morning, we were passing empty instead of undefined for native.
| @@ -110,13 +110,18 @@ function LWCElement(this: Component) { | |||
| setInternalField(component, ViewModelReflection, vm); | |||
| setInternalField(elm, ViewModelReflection, vm); | |||
| setInternalField(cmpRoot, ViewModelReflection, vm); | |||
| let { elmProto } = def; | |||
There was a problem hiding this comment.
@pmdartus this is the refactor that I mentioned this morning. The super fast pass is for real WC in which case we need to do nothing because those descriptors are part of the custom element constructor's prototype (detectable via PatchedFlag mark).
On the other hand, if it needs to be patched, we need have a fast path when the proto is the expected proto, in which case we do the proto chain mutation. But if we need to go on the slow path, we don't do proto, we just define the properties (which is what we were doing before anyways).
| let tableElement; | ||
| let store; | ||
|
|
||
| before(async () => { | ||
| tableElement = createElement('benchmark-table', { is: Table }); | ||
| tableElement = createElement('benchmark-table-component', { is: Table }); |
There was a problem hiding this comment.
ambiguity on the name of the tests discussed in slack today.
| @@ -0,0 +1,33 @@ | |||
| import { buildCustomElementConstructor } from 'engine'; | |||
There was a problem hiding this comment.
adding a new set of benchmark tests for WC only.
Benchmark resultsBase commit: |
|
Do we have integration test for this? |
caridy
force-pushed
the
caridy/wc-customElement
branch
from
39ebd59 to
7326a10
Compare
@davidturissini has a whole new PR to run existing integration tests with the new mode. Still need some work with selenium and such. For now, getting the benchmark to run to completion seems like a good enough signal.
Seems like a flapper, running it again. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Details
This experimental PR allows the creation of custom elements that can be registered as web components from a LWCElement declaration. e.g.:
Does this PR introduce a breaking change?