material-components / material-web

Material Design Web Components
https://material-web.dev
Apache License 2.0
9.2k stars 882 forks source link

No side-effects exports #2010

Closed guillemcordoba closed 3 years ago

guillemcordoba commented 3 years ago

Is your feature request related to a problem? Please describe. I can't import some of the elements without actually defining them with the customElements tag. Some elements declare their Base class as abstract and some not, and I can only import the Base classes without side-effects.

This makes it cumbersome to use this elements with something like ScopedElementsRegistry.

Describe the solution you'd like All elements should provide an exportable non-abstract class, without any side-effects.

Describe alternatives you've considered You could go either way, making all base classes non-abstract and exporting them, or making all of them abstract and providing another exported class with that specific use case in mind.

Additional context

customElements.define('mwc-linear-progress', LinearProgressBase), // This works
customElements.define('mwc-menu-surface', MenuSurfaceBase),      // This does not work, MenuSurfaceBase is abstract
asyncLiz commented 3 years ago

Thanks for the issue!

It definitely looks like MenuSurfaceBase shouldn't be abstract, but overall this pattern is correct for what our library is providing. For users that wish to define custom elements for side-effect-free imports, the appropriate pattern would be to extend the base class class and define the final class that is used in the custom element definition.

The base class itself should not be used when defining a custom element, since they will be missing styles. Additionally, some base classes in the future may be abstract and require final element classes to provide a property or implementation feature (such as a variant type).

This is our recommended pattern for extending and customizing elements.

import {LinearProgressBase} from '@material/mwc-linear-progress/mwc-linear-progress-base';
import {style} from '@material/mwc-linear-progress/mwc-linear-progress-css';

class LinearProgress extends LinearProgressBase {
  // Override styles and functionality
  static styles = [style];
}

// Scope or re-define tag name
customElements.define('mwc-linear-progress', LinearProgress);

Hope this clarifies things for usage with ScopedElementsRegistry and side-effect-free imports!

guillemcordoba commented 3 years ago

@asyncLiz Mmm I see your point, thanks a lot for the detailed explanation.

Although, there are some base classes that are importing directly other non-base classes. For instance, in ButtonBase:

import '@material/mwc-icon/mwc-icon';
import '@material/mwc-ripple/mwc-ripple';

This makes it so that even if I import ButtonBase trying to not have side-effects. mwc-icon is getting defined and will break if imported again. Is there some existing workaround for this? For now I'm replacing all global customElements definitions in my rollup build step but that's far from ideal.

asyncLiz commented 3 years ago

That is because those classes directly depend on those elements being defined since they use them in their render templates. If they were not defined, the elements would not work correctly and could throw errors when being used.

Unfortunately in those scenarios, I do not see an elegant way around the issue.

It also does not make sense to me for those elements to be re-defined in a scoped element registry since the base classes depend on their tag names in the base class templates. I would be curious as to what your use case is for needing to re-define them.

guillemcordoba commented 3 years ago

@asyncLiz sure :), BTW, really enjoying the quick feedback and the thoughtful discussion.

My use case is an application with pluggable UIs, in which "blocks" of UI in the form of webcomponents are uploaded to a "repository" and lazily downloaded to the browser on demand. In this scenario, the overarching application is agnostic as to which frameworks/components the blocks contain, but as much as possible they need to be side-effect free to not rely on global definitions that can cause collision.

This is the whole reason to use scoped registries, even if they need to be included via a polyfill (right now using this, but the open-wc alternative would work as well). These libraries react to a custom element tag being included in the dom of the elements and change it to a scoped version, that looks like <mwc-button-1343>, where mwc-button-1343 is registered in the custom elements registry owned by the shadow root of the rendering element.

Regarding the use of this materials-components library, the pain point is element versioning: if each of the blocks need to rely on globally defined elements, all of them need to agree on a version of those elements, instead of freely included whatever code and elements and defining them in a scoped manner.

Happy to keep the conversation going if there are more doubts around this.

guillemcordoba commented 3 years ago

@asyncLiz for now I'm pointing to this repo: https://github.com/guillemcordoba/scoped-material-components which reexports all the elements with scoped custom element registries in mind.