feat(engine-core): data-passing heuristic for external components

[ekashida]

Details

https://rfcs.lwc.dev/rfcs/lwc/0118-native-web-components-support#passing-data

This was originally implemented using a new type of vnode for external custom elements but the amount of required changes to existing code didn't seem worth it, especially since the engine only needs to know whether an element is external when patching attributes/properties. I think we can consider defining a new vnode type if we end up having to do more than this in the future.

Does this pull request introduce a breaking change?

No

Does this pull request introduce an observable change?

No

GUS work item

W-11610752

[abdulsattar]

why not move this one check to attrs.ts and do the prop assignment there?

// attrs.ts
if(vnode.data.external && htmlAttributeToProperty(name) in elm) {
  (elm as any)[propName] = cur;
}

this will prevent duplication of the attribute assignment logic below.

[caridy]

I don't think we want to slow down the regular cases with more conditions @abdulsattar, I think it is fine to have this small duplication. Another alternative is to move the logic below into an abstraction layer that both modules can rely on.

[abdulsattar]

@caridy I did think of that but I realized that we're doing the check anyway: https://github.com/salesforce/lwc/pull/3150/files#diff-8622743d79e146365dd8c59fb85057a09b0f9577d99f065628abdd95d0d7a754R549

So, the first conditional in the above if(vnode.data.external && ...) is equivalent to what we have here.

[nolanlawson]

if (propName in elm) {

Will this be an issue if propName is constructor? Or __proto__? Should this be a hasOwnProperty check instead?

[caridy]

It can't be hasOwnProperty because inheritance on web components. but since our compiler is responsible for constructing the attrs dictionary per vnode, we can ban those names when parsing the element. It is probably already a problem with spread directive, so we better do that soon here. Although I don't know how to solve it with spread directive, since that's runtime. I believe class declarations will prevent the modification of some of that though, making them non-configurable, non-writable.

[ekashida]

@abdulsattar The check that you pointed out is done once per element, whereas the check required in attrs.ts would be once per attribute.

[pmdartus]

For what it's worth Vue and Svelte uses the same in check to differentiate between props and attributes.

[caridy]

adjusts needed

[ekashida]

@caridy ready for another pass

[caridy]

LGTM

[caridy]

use setProperty instead