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

[caridy]

Details

This a POC (a 3rd attempt) of using the global registry in a non-observable ways for authors in order to allow LWC to detect connectness of elements via the custom elements.

The previous attempts were:

1. using the global registry by registering lwc elements on it automatically, this creates conflicts with customers doing the same, even if they were just registering lwc as web components.

An example of the broken app was reported by Diego, the documentation SPA was loading and registering global elements when needed by a markdown file, as the user navigates throughout the app, but because some of those global elements might have been used by a previous view as a secondary element (an element from an LWC template), the app fails to register the global one complaining that the name was claimed by LWC.

2. using an iframe to isolated the new entries, this creates other issues with identities of built-in elements when created off of the shadow root attached to the element.

This approach is different, it uses what we call pivot elements. This is the same technique used by polymer's scope registry polyfill.

[salesforce-cla]

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Caridy Patino <c***@c***.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.

[nolanlawson]

It would be great to see a test to demonstrate the fix. I also added some nitpicks about isUndefined and other @lwc/shared functions.

[nolanlawson]

AIUI, the @lwc/engine-dom package doesn't modify any globals (whereas @lwc/synthetic-shadow modifies lots). Is there a risk to having LWC get into the business of modifying the global window.HTMLElement here?

(Same question for window.customElements as well.)

[nolanlawson]

@caridy I was thinking about this some more... It seems to me that the global patches should ideally go in a separate package. We do the same thing for @lwc/synthetic-shadow. A few benefits:

• @lwc/engine-dom remains pure, doesn't override globals
• OSS consumers who don't need this behavior (i.e who are comfortable with WCs living in the same namespace) don't have to get it
• if/when we can get the behavior we need from the web platform (e.g. scoped custom elements registry), we can remove the polyfill

What do you think?

[caridy]

I'm fine either way... but if you move it to another package, you need to have ways to interact with the internals of that other package, so, it is not really a polyfill, it has to have some backdoor for lwc to register internal stuff.

[caridy]

also, the DOM package is about the DOM, and this is about the DOM as well, I don't see why not patching is really a problem for this case since the change is not observable for others.

[nolanlawson]

if you move it to another package, you need to have ways to interact with the internals of that other package

We already do this with @lwc/synthetic-shadow; we add some special options to attachShadow to communicate between @lwc/engine-dom and the polyfill.

I don't see why not patching is really a problem for this case since the change is not observable for others

Patching globals is always risky business; it's just the law of unintended consequences. E.g. the browser API may change in the future. On platform we own everything, but off-platform I think we should try to be good citizens and avoid patching globals.

[nolanlawson]

I just realized we are already patching globals in @lwc/engine-dom, and it seems to be for the same reason as this PR (lack of a usable scoped custom element registry):

lwc/packages/@lwc/engine-dom/src/apis/create-element.ts

Lines 39 to 42 in 6ebaaf1

	// Monkey patching Node methods to be able to detect the insertions and removal of root elements
	// created via createElement.
	const { appendChild, insertBefore, removeChild, replaceChild } = _Node.prototype;
	assign(_Node.prototype, {

I guess ideally we should have all these global mutations in the same place? @lwc/scoped-custom-element-registry-polyfill or something like that?

[caridy]

yeah.

[nolanlawson]

@caridy This PR got massively bitrotted by changes in master (sorry about that). I made an effort to rebase it (a5e247d), but the Karma tests are failing, so there's probably something I missed.

With this PR, are we able to remove the global DOM-patching here?

lwc/packages/@lwc/engine-dom/src/apis/create-element.ts

Lines 42 to 61 in fd6ad1a

	assign(_Node.prototype, {
	appendChild(newChild) {
	const appendedNode = appendChild.call(this, newChild);
	return callNodeSlot(appendedNode, ConnectingSlot);
	},
	insertBefore(newChild, referenceNode) {
	const insertedNode = insertBefore.call(this, newChild, referenceNode);
	return callNodeSlot(insertedNode, ConnectingSlot);
	},
	removeChild(oldChild) {
	const removedNode = removeChild.call(this, oldChild);
	return callNodeSlot(removedNode, DisconnectingSlot);
	},
	replaceChild(newChild, oldChild) {
	const replacedNode = replaceChild.call(this, newChild, oldChild);
	callNodeSlot(replacedNode, DisconnectingSlot);
	callNodeSlot(newChild, ConnectingSlot);
	return replacedNode;
	},
	} as Pick<Node, 'appendChild' | 'insertBefore' | 'removeChild' | 'replaceChild'>);

[nolanlawson]

Well, I got most of the tests passing (1453edf), but there are still some failures due to some subtle issues with the HTMLBridgeElement.

[nolanlawson]

With this PR, are we able to remove the global DOM-patching here?

N/m, just confirmed that yes, we can.

[caridy]

closing in favor of the rebased PR done by @nolanlawson on #2724