ing-bank / lion

Fundamental white label web component features for your design system.
https://lion-web.netlify.app/
MIT License
1.74k stars 292 forks source link

[SlotMixin] Can't use html function when no shadow root #1902

Open MatthieuLebigre opened 1 year ago

MatthieuLebigre commented 1 year ago

Expected behavior

We should be able to use the html function to define a slot, even when the element doesn't have a shadow root:

  createRenderRoot() {
    return this;
  }

  get slots() {
    return {
      ...super.slots,
      'my-custom-slot': () => html`<div></div>`,
    };
  }

Actual Behavior

An error is thrown at runtime, because of the line in SlotMixin:

const registryRoot = supportsScopedRegistry ? this.shadowRoot : document;

shadowRoot is null and the code tries to access it later with no additional check

Additional context

Easy to reproduce when extending LionOptions

tlouisse commented 1 year ago

As the name suggests, SlotMixin is designed for rendering slots (especially for solving a11y issues that can only be solved when AOM is ready). And slots only exist inside shadow dom :)

We might be able to make it work with:

const supportsScopedRegistry = !!ShadowRoot.prototype.createElement;
const hasShadowRoot = Boolean(this.shadowRoot);
const registryRoot = (supportsScopedRegistry && hasShadowRoot) ? this.shadowRoot : document;

But we would need to understand valid use cases. Can you elaborate on your use case a bit?

MatthieuLebigre commented 1 year ago

Haha, you're right :) Actually I had in my codebase a definition for a slot in a subclass of LionOptions (by overriding the get slots accessor - and applying the SlotMixin on this subclass. I never noticed that LionOptions didn't have a shadow dom :) But it happened to 'work', in the sense that it's replaced in the dom (not in a slot obviously)

But with the recent changes in SlotMixin, now I have the error mentionned.

So I'll fix my code to remove this slot definition from LionOptions subclass. But maybe it could nice to throw or log an error when someone (like me :)) does a mistake and try to use SlotMixin on an element without shadow dom

tlouisse commented 1 year ago

Ah, understandable. I think we should throw a more user friendly error here indeed

ParamjotSingh5 commented 1 year ago

Hi 😃, I want to contribute here, does this functionality, yet to be implemented?

Dozom commented 3 months ago

Hi :), I want to contribute here too (I have never contributed to an open source project before). I have created a pull request.

const hasShadowRoot = Boolean(this.shadowRoot);
if (!hasShadowRoot) {
  throw new Error(`No shadowRoot was found`);
}
const registryRoot = supportsScopedRegistry ? this.shadowRoot : document;