feat(engine): custom element registry pivots #2724

nolanlawson · 2022-03-08T20:21:05Z

Details

This is an attempt to rebase #2564 and get it working.

The main goal is to allow third-party custom elements to use tag names that are the same as tag names already reserved by LWC custom elements:

// lwc-component.js
lwc.createElement('x-foo', { is: Foo })

// vanilla-component.js
customElements.define('x-foo', Foo)

It also allows LWC custom elements to re-use the same tag name to refer to different components:

// button-1.js
lwc.createElement('c-button', { is: Button1 })

// button-2.js
lwc.createElement('c-button', { is: Button2 })

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

Probably there is something observable about changing how we do custom element registry scoping, but I haven't thought it through yet.

nolanlawson · 2022-03-08T20:23:19Z

Most of the Karma tests are passing; almost all of the failures now are due to CustomElementConstructor, which is now throwing the error Illegal constructor.

I'm not sure how we plan to make CustomElementConstructor work with the new pivot design. My feeble brain is still struggling to understand all of our prototype/superclass/inheritance stuff.

nolanlawson · 2022-03-08T23:07:44Z

OK, all Karma tests are passing now; it's just the server that's still busted.

nolanlawson · 2022-03-09T00:57:25Z

Server is fixed, but compat karma tests need to be fixed.

nolanlawson · 2022-03-11T21:58:57Z

@caridy Local tests are passing, so it looks like your technique works. 🙂 My PR is still a bit rough, but overall this looks promising!

Some downstream tests are failing, but it looks like it may just be Jest/JSDOM issues.

nolanlawson · 2022-03-14T18:37:39Z

/nucleus test

caridy · 2022-03-14T20:14:08Z

packages/@lwc/engine-dom/src/apis/build-custom-element-constructor.ts

+        attributeChangedCallback(name: string, oldValue: any, newValue: any) {
+            attributeChangedCallback.call(this, name, oldValue, newValue);
+        }
+        static observedAttributes = observedAttributes;


this is one of the parts that worries me a little bit!

@caridy For the sake of my fuzzy memory, what part of it worries you?

I need to think more about it... but the fact that we now rely on observed attributes in the class that we produce, might introduce other issues with the inheritance path outside of LWC, and probably also conflict with authors list of attributes. I don't remember how observedAttributes from this record is constructed.

If I'm not mistaken, we handle inheritance of observed attributes in base-bridge-element.ts:

lwc/packages/@lwc/engine-core/src/framework/base-bridge-element.ts

Lines 187 to 193 in 838035d

// Specify attributes for which we want to reflect changes back to their corresponding

// properties via attributeChangedCallback.

defineProperty(HTMLBridgeElement, 'observedAttributes', {

get() {

return [...superObservedAttributes, ...keys(attributeToPropMap)];

},

});

right, but beyond that, I don't know.

The Karma tests are all passing currently. Is there a case you think isn't covered?

yeah, I don't doubt that, but why was this change needed in the first place? If the custom element inherit the right thing... then you should be able to get the observedAttributes and others from there rather than rebuilding that yourself.

nolanlawson · 2022-05-26T22:32:03Z

packages/@lwc/engine-dom/src/patches/global-registry.ts

+                'If this is a Jest environment, check your Jest/JSDOM configuration.'
+        );
+    }
+    (nativeRegistry as any)[registryPatchedSymbol] = true;


If we don't add this check, then an error will be thrown eventually for "Illegal constructor." I saw this in two of our downstreams' Jest tests.

I considered wrapping this in a process.env.NODE_ENV condition, but what if a consumer loads two versions of the engine and one is minified? Better safe than sorry.

this is one of the things that I want to discuss, this might actually be a red flag.

Chatted with Caridy, the concerns are:

If the patched registry doesn't work when applied twice, then this may limit our ability to have multiple versions of the framework running on top of the same registry. Ideally it should be resilient to being applied twice.

Abstracting the Custom Element Infrastructure out of Core Package #2799 might allow us to get rid of the UserCtor being passed in to the PivotCtor, we should explore that as well:

lwc/packages/@lwc/engine-dom/src/patches/global-registry.ts

Lines 59 to 60 in 5541855

class PivotCtor extends NativeHTMLElement {

constructor(UserCtor?: CustomElementConstructor) {

Good news, it works with two copies of the engine when running in Karma. Looks like this is a Jest-only issue.

nolanlawson · 2022-05-26T22:49:38Z

scripts/jest/root.config.js

        global: {
            branches: 80,
-            functions: 90,
+            functions: 85,


This is necessary because I added a Jest test to @lwc/engine-dom, and now Jest is complaining about the global-registry.ts file containing too much untested code. engine-dom is already well-tested elsewhere.

nolanlawson · 2022-05-26T22:49:54Z

scripts/bundlesize/bundlesize.config.json

        {
            "path": "packages/lwc/dist/engine-dom/umd/es2017/engine-dom.min.js",
-            "maxSize": "17.5KB",
+            "maxSize": "18KB",


The global-registry adds a decent chunk of code.

packages/integration-karma/test/upgradable-element/index.spec.js

nolanlawson · 2022-05-26T22:51:12Z

packages/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js

+        const TestCustomElement = Test.CustomElementConstructor;
+
+        expect(typeof TestCustomElement).toBe('function');
+    });


Could not figure out why, but this started throwing in IE. Didn't seem worth investigating beyond that.

It could be beneficial to throw an error when accessing CustomElementConstructor in an environment with no support for a native custom element eg. IE11.

Aha, so you're saying that the fact that the behavior changed here is not a big deal? What if someone is using their own custom elements polyfill with CustomElementConstructor in IE11 though?

nolanlawson · 2022-05-27T17:01:01Z

Blocked by #2855

This reverts commit f3788c0.

ravijayaramappa

LGTM. Please wait for @caridy to +1 since he is most adept at the Custom Registry spec.

nolanlawson · 2022-09-30T17:13:32Z

@ravijayaramappa We should move createCustomElement out of the renderer factory though, right? Make it excluded just like createStylesheet?

ravijayaramappa · 2022-09-30T17:29:01Z

We should move createCustomElement out of the renderer factory though, right? Make it excluded just like createStylesheet?

Yes. You can do it in this PR or a follow up.

…s-pivots

nolanlawson · 2022-09-30T17:41:42Z

@ravijayaramappa No problem. Done: f0a3746

nolanlawson · 2022-09-30T19:04:25Z

/nucleus ignore --reason "lwc-platform needs updating"

nolanlawson · 2022-09-30T20:12:44Z

Some feedback from discussion with @caridy:

Currently when we call lwc.createElement('x-foo'), then customElements.get('x-foo') will return the PivotCtor (which is useless without the UserCtor – it gives you a "dummy" custom element). I did it this way to match existing LWC behavior.

However, this may break a third-party component using this common pattern:

if (!customElements.get('my-tag')) {
  customElements.define('my-tag', ...)
}

The above will break if LWC reserves my-tag first.

However, it's worth acknowledging that even if we fix customElements.get and customElements.whenDefined to return undefined rather than the PivotCtor, we still can't fix the following case:

// LWC code
lwc.createElement('x-foo', ...)

// userland code
const elm = document.createElement('x-foo')
const PivotCtor = elm.constructor
new PivotCtor()

So in a sense, we can't fully hide the PivotCtor. But we can potentially hide it from customElements.get and customElements.whenDefined at least.

This means we also need to fix the case where someone does:

new PivotCtor(UserCtorThatWasNeverRegistered)

^ This should be detectable, and we should probably throw an error in this case.

nolanlawson · 2022-09-30T21:17:27Z

Regarding the above: I added a TODO to return undefined from customElements.get/whenDefined (#3073), and I fixed the case where the user passes in an arbitrary UserCtor: f9e0c50

To avoid making this a "forever" PR, I should probably just keep adding TODOs and stop for now! 😛

…s-pivots

ravijayaramappa · 2022-10-08T00:25:39Z

@nolanlawson You probably have test cases for these, if not these should be add-ons to #3073

class Foo extends HTMLElement {}
customElements.define('x-foo', Foo);

const elm = document.createElement('x-foo');
console.log(elm instanceof Foo); // true
console.log(elm.constructor === Foo); // true

nolanlawson · 2022-10-10T17:33:59Z

@ravijayaramappa In the pivots implementation, those last two statements would be false rather than true. The elm will be an instance of PivotCtor, which does not extend from Foo. However, we could add that test for the non-pivots case.

nolanlawson · 2022-10-10T20:30:17Z

/nucleus test

…s-pivots

nolanlawson · 2022-10-12T22:39:01Z

It's time to merge this... it's behind a flag, and we can fix remaining issues in follow-up PRs. Thanks to everyone who reviewed!

nolanlawson requested review from caridy, jodarove and pmdartus March 8, 2022 20:21

nolanlawson force-pushed the nolan/node-reactions-pivots branch from 7c4a7b9 to 476ca51 Compare March 11, 2022 18:48

nolanlawson added the nomerge label Mar 11, 2022

caridy reviewed Mar 14, 2022

View reviewed changes

nolanlawson force-pushed the nolan/node-reactions-pivots branch from 1339f84 to b11b830 Compare March 22, 2022 18:03

nolanlawson mentioned this pull request Mar 30, 2022

refactor(engine-core): Simplify unmount process #2709

Closed

nolanlawson force-pushed the nolan/node-reactions-pivots branch from 776139c to c01ea64 Compare May 26, 2022 17:39

nolanlawson commented May 26, 2022

View reviewed changes

packages/integration-karma/test/upgradable-element/index.spec.js Show resolved Hide resolved

nolanlawson commented May 26, 2022

View reviewed changes

nolanlawson mentioned this pull request May 27, 2022

chore(nucleus): remove more downstreams #2855

Merged

nolanlawson force-pushed the nolan/node-reactions-pivots branch from 86ae839 to fc1d389 Compare May 27, 2022 17:45

caridy and others added 8 commits May 27, 2022 14:37

refactor(engine): avoiding using global registry for local lwc entries

85e3937

fix: remove unused code

7905f62

fix: fix most of the karma tests

8f4583c

fix: fix errors

c7a6260

fix: use native connected callback

e3599cf

Revert "fix: use native connected callback"

64d4373

This reverts commit f3788c0.

fix: making progress

0b36bb8

test: remove unnecessary test

e356ce6

nolanlawson requested a review from ravijayaramappa September 28, 2022 20:18

ravijayaramappa approved these changes Sep 29, 2022

View reviewed changes

nolanlawson added 2 commits September 30, 2022 10:37

Merge remote-tracking branch 'origin/master' into nolan/node-reaction…

a54d697

…s-pivots

fix: move createCustomElement out of the renderer factory

f0a3746

ravijayaramappa approved these changes Sep 30, 2022

View reviewed changes

This was referenced Sep 30, 2022

[Pivots] Support "extends" key in customElements.define() #2983

Closed

[Pivots] customElements.get/whenDefined should not return PivotConstructor #3073

Closed

fix: add todo and check for unregistered UserCtor

f9e0c50

This was referenced Sep 30, 2022

[Pivots] Option to use global custom elements registry #3076

Closed

fix: add ENABLE_SCOPED_CUSTOM_ELEMENT_REGISTRY flag #3079

Merged

nolanlawson added 2 commits October 5, 2022 11:36

Merge remote-tracking branch 'origin/master' into nolan/node-reaction…

0beb349

…s-pivots

Merge remote-tracking branch 'origin/master' into nolan/node-reaction…

00dee5f

…s-pivots

nolanlawson added 2 commits October 10, 2022 11:11

fix: add ENABLE_SCOPED_CUSTOM_ELEMENT_REGISTRY flag (#3079)

79c8404

test: add test for constructor in registry

52121c8

nolanlawson mentioned this pull request Oct 10, 2022

[Pivots] Extract scoped registry into a standalone package #3096

Closed

Merge remote-tracking branch 'origin/master' into nolan/node-reaction…

3662a98

…s-pivots

nolanlawson merged commit 0710d6a into master Oct 12, 2022

nolanlawson deleted the nolan/node-reactions-pivots branch October 12, 2022 22:39

nolanlawson mentioned this pull request Oct 14, 2022

test(pivots): test instanceof custom element #3114

Merged

nolanlawson mentioned this pull request Nov 11, 2022

perf(engine-dom): optimize connected/disconnected callbacks #3162

Merged

	// Specify attributes for which we want to reflect changes back to their corresponding
	// properties via attributeChangedCallback.
	defineProperty(HTMLBridgeElement, 'observedAttributes', {
	get() {
	return [...superObservedAttributes, ...keys(attributeToPropMap)];
	},
	});

	class PivotCtor extends NativeHTMLElement {
	constructor(UserCtor?: CustomElementConstructor) {

feat(engine): custom element registry pivots #2724

feat(engine): custom element registry pivots #2724

Uh oh!

Conversation

nolanlawson commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

Uh oh!

nolanlawson commented Mar 8, 2022

Uh oh!

nolanlawson commented Mar 8, 2022

Uh oh!

nolanlawson commented Mar 9, 2022

Uh oh!

nolanlawson commented Mar 11, 2022

Uh oh!

nolanlawson commented Mar 14, 2022

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

nolanlawson commented May 27, 2022

Uh oh!

ravijayaramappa left a comment

Choose a reason for hiding this comment

Uh oh!

nolanlawson commented Sep 30, 2022

Uh oh!

ravijayaramappa commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nolanlawson commented Sep 30, 2022

Uh oh!

nolanlawson commented Sep 30, 2022

Uh oh!

nolanlawson commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nolanlawson commented Sep 30, 2022

Uh oh!

ravijayaramappa commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nolanlawson commented Oct 10, 2022

Uh oh!

nolanlawson commented Oct 10, 2022

Uh oh!

nolanlawson commented Oct 12, 2022

nolanlawson commented Mar 8, 2022 •

edited

Loading

ravijayaramappa commented Sep 30, 2022 •

edited

Loading

nolanlawson commented Sep 30, 2022 •

edited

Loading

ravijayaramappa commented Oct 8, 2022 •

edited

Loading