-
-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
you are still relying on mwc elementes to be defined globally by HA #52
Comments
I don't have much input on this as it was not my addition. Any input? Thanks |
BTW, if you do decide to "allow" the usage of HA elements, there is a need for a custom mechanism to allow that, as some HA elements are lazy-loaded (specifically the ones available in the config flow and the ones described here). I applied such a mechanism in the mentioned repo above, if you want to discuss it further, have a look here |
Right, we have to pass the registry of the host down to the mwc-elements, this is a way to do that: class CustomCard extends ScopedRegistryHost(LitElement) {
static get elementDefinitions() {
return {
...textfieldDefinition(CustomCard),
};
}
....
}
export const textfieldDefinition = (registryHost) => ({
"mwc-notched-outline": class extends NotchedOutlineBase {
static get styles() {
return notchedOutlineStyles;
}
},
"mwc-textfield": class extends TextFieldBase {
static get shadowRootOptions() {
return {
...super.shadowRootOptions,
customElements: registryHost.registry,
};
}
createRenderRoot() {
return (this.renderOptions.creationScope = super.createRenderRoot());
}
static get styles() {
return textfieldStyles;
}
},
}); |
We can create a little mixin to do that: export function ScopedRegistryChild(superclass) {
return class ScopedRegistryChildMixin extends superclass {
static get shadowRootOptions() {
return {
...super.shadowRootOptions,
customElements: this.registryHost.registry,
};
}
createRenderRoot() {
return (this.renderOptions.creationScope = super.createRenderRoot());
}
};
} Usage: "mwc-textfield": class extends ScopedRegistryChild(TextFieldBase) {
static get registryHost() {
return registryHost;
}
static get styles() {
return textfieldStyles;
}
}, Could also pass the styles and host to the mixin function, to make it even simpler: "mwc-textfield": ScopedRegistryChild(TextFieldBase, textfieldStyles, registryHost), export function ScopedRegistryChild(superclass, styles, registryHost) {
return class ScopedRegistryChildMixin extends superclass {
static get shadowRootOptions() {
return {
...super.shadowRootOptions,
customElements: registryHost.registry,
};
}
static get styles() {
return styles;
}
createRenderRoot() {
return (this.renderOptions.creationScope = super.createRenderRoot());
}
};
} |
This seems very verbose and plays too much with the internals. Also, it still doesn't support multi nested elements. How about this: import { ScopedRegistryHost } from '@lit-labs/scoped-registry-mixin';
import { ListBase } from '@material/mwc-list/mwc-list-base';
import { styles as listStyles } from '@material/mwc-list/mwc-list.css';
import buildElementDefinitions from '../buildElementDefinitions';
import MwcListItem from './list-item';
export default class MwcList extends ScopedRegistryHost(ListBase) {
static get defineId() { return 'mwc-list'; }
static get elementDefinitions() {
return buildElementDefinitions([MwcListItem], MwcList);
}
static get styles() {
return listStyles;
}
} import { ScopedRegistryHost } from '@lit-labs/scoped-registry-mixin';
import { ListItemBase } from '@material/mwc-list/mwc-list-item-base';
import { styles as listItemStyles } from '@material/mwc-list/mwc-list-item.css';
import buildElementDefinitions from '../buildElementDefinitions';
import MwcRipple from './ripple';
export default class MwcListItem extends ScopedRegistryHost(ListItemBase) {
static get defineId() { return 'mwc-list-item'; }
static get elementDefinitions() {
return buildElementDefinitions([MwcRipple], MwcListItem);
}
static get styles() {
return listItemStyles;
}
} import { RippleBase } from '@material/mwc-ripple/mwc-ripple-base';
import { styles as rippleStyles } from '@material/mwc-ripple/mwc-ripple.css';
import { ScopedRegistryHost } from '@lit-labs/scoped-registry-mixin';
import buildElementDefinitions from '../buildElementDefinitions';
export default class MwcRipple extends ScopedRegistryHost(RippleBase) {
static get defineId() { return 'mwc-ripple'; }
static get elementDefinitions() {
return buildElementDefinitions([], MwcRipple);
}
static get styles() {
return rippleStyles;
}
} const buildElementDefinitions = (elements, constructor) => elements.reduce((aggregate, element) => {
if (element.defineId) {
// eslint-disable-next-line no-param-reassign
aggregate[element.defineId] = element;
} else {
element.promise.then((clazz) => {
if (constructor.registry.get(element.name) === undefined) {
constructor.registry.define(element.name, clazz);
}
});
}
return aggregate;
}, {});
export default buildElementDefinitions; here we define (and encapsulate) each element direct children within itself. No magic. const globalElementLoader = name => ({
name,
promise: customElements.whenDefined(name).then(() => customElements.get(name)),
});
export default globalElementLoader; ...
static get elementDefinitions() {
return buildElementDefinitions([
globalElementLoader('ha-card'),
globalElementLoader('more-info-light'),
globalElementLoader('ha-switch'),
globalElementLoader('ha-icon'),
globalElementLoader('ha-slider'),
globalElementLoader('ha-color-picker'),
MwcSelect,
MwcListItem,
], LightEntityCard);
}
,.. |
The idea is that the custom card only has 1 custom registry, not every element. By passing the registry of the host down we make sure they all share the same registry. There really isn't much magic going on, and by creating a mixin like I did here #52 (comment) I don't think it is verbose at all? |
I see your point, but you are creating a "global" registry just for the card. There can still be conflicts between elements when you do that, if, for example, you use a couple of libraries. |
The changes you made to start using the scoped custom element registry (according to https://developers.home-assistant.io/blog/2022/02/18/paper-elements/ and https://developers.home-assistant.io/blog/2020/10/02/lazymoreinfo/#what-about-external-elements) are not bulletproof just yet. The idea is that your card relies entirely on its own bundled elements in order to work and here it is violated.
Let's take for example the
mwc-list-item
custom element you added. This element, internally, rendersmwc-ripple
. Since you didn't define your custom class as a ScopedRegistryHost, the internalmwc-ripple
element, will only be rendered if the global registry defines it (which is currently being done by HA). If HA suddenly drops the support for MWC, the element will not be rendered properly.This happens because ScopedRegistryHost is not recursive, and only applies to the elements directly rendered by it.
The solution I applied in other cards (artem-sedykh/mini-climate-card#67 for example), is to define each custom element as ScopedRegistryHost as well while setting its used elements as well (https://github.com/regevbr/mini-climate-card/blob/f61364816c5667370eebc5a44168713e5c36ea02/src/components/mwc/list-item.js#L7-L17)
If you are willing, I don't mind drafting a PR.
Another issue, that also relates to how HA sees custom cards -
Custom Cards using the elements used in the Home Assistant Frontend have never been supported
(taken verbatim from the above-mentioned blog posts). So it seems that using `ha-card- here should also be removed.@iantrich I would appreciate your input on the matter (I hope I was clear enough)
The text was updated successfully, but these errors were encountered: