fix: only remove slot children in synthetic shadow

[nolanlawson]

Details

Follow-up to #2840.

I'm not sure if this change is really worth making, but it seems nice to call out here that removeChildren is only necessary for synthetic shadow. Maybe this will make it easier to remove in the future when synthetic shadow is gone? Or if we do an "LWC lite" build?

If not, I'd also be happy just to add a code comment that this is only needed for synthetic.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ✅ No, it does not introduce an observable change.

[pmdartus]

I like the idea of being more explicit here to call out the synthetic shadow DOM quirks.

[pmdartus]

The boolean comparison will certainly be faster than the string comparison. It would be preferable to do the boolean check first.

I would also move the removeChildren check in the VNodeType.Element case since slots can only be elements.

Suggested change

	const removeChildren = sel === 'slot' && vnode.owner.shadowMode === ShadowMode.Synthetic;
	switch (type) {
	case VNodeType.Element:
	unmountVNodes(vnode.children, elm as ParentNode, removeChildren);
	const removeChildren = sel === 'slot' && vnode.owner.shadowMode === ShadowMode.Synthetic;
	switch (type) {
	case VNodeType.Element:
	const removeChildren = vnode.owner.shadowMode === ShadowMode.Synthetic && sel === 'slot';
	unmountVNodes(vnode.children, elm as ParentNode, removeChildren);

[abdulsattar]

We have a lint rule: no-case-declarations that prevents us from doing that.

[nolanlawson]

I assumed that the property lookup (vnode.owner.shadowMode) would be more expensive than a string comparison. I have no idea what the perf difference is, but it's probably a micro-optimization either way.

As Sattar said, yeah, we have to keep the removeChildren where it is unfortunately.

[pmdartus]

We have a lint rule: no-case-declarations that prevents us from doing that.

Aren't rules made to be broken? 🤣

More seriously, this ESLint rule prevents declaration in switch to avoid re-declaration. By default, variables are declared at the switch level and not at the case level (detail). The way to work around this is to create a new block scope for cases with variable declarations.

switch (type) {
  case VNodeType.Element: {
    const removeChildren = vnode.owner.shadowMode === ShadowMode.Synthetic && sel === 'slot';
  },
  // [...]
}

[nolanlawson]

Added a case block.