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
When using with LitElement/Webcomponent getting Illegal Constructor
error
#40
Comments
Hi @andreabosman16, thanks for finding this. I don't have experience with WebComponents, but my understanding is this is an issue related to extending
Please correct me if I'm wrong, but at first glance this doesn't look like a |
Thanks for responding this quick. In general the illegal constructor problem is solved with customelement.define() but this isn’t the case with me. I think it is a problem with an the mixin classes being an anonymous class or something like this. |
Alright, in that case I'm going to need some kind of minimal reproducible example in order to troubleshoot this. Alternatively PRs are welcome if you have an idea of how to resolve the issue! Can you also describe your use case a bit more so I have a better idea of why this is needed? |
I am going on vacation now i will give you a sample in two weeks if that is okay with you |
I'm in no rush. Enjoy your vacation 🙂 |
Hi @tannerntannern I think this issue is quite simple to explain even without a minimum reproducible example and I can try to explain it a little bit in depth since we faced the same. Immagine that you want to create a basic class in your code to add functionalities on top of the class MyElement extends HTMLElement {
public awesomeNewFeature(): string {
return 'Whoa!!!'
}
} Than you want to create many different WebComponents that should extend both this class ElementA extends Mixin(MyElement, OtherClass, AnotherClass) {
}
// And since we want to have it as a WebComponent we also add it to the customElements:
customElements.define('element-a', ElementA); Now the problem is that WebComponents cannot be instantiated using the new keyword, but rather you should use the The issue seems to be in the function We didn't investigated this more on our side, but I hope it is more clear now, if it is not we will try to provide a running sample. Thanks! |
Maybe the solution can be to create a function that will check recursively in the prototype chain if any ancestor of the constructor is of type To achieve the above one assumption should be mandatory, the fact that each class extending the class ElementB extends HTMLElement {
bar = 'bar';
}
ElementB.selector = "element-b";
customElements.define(ElementB.selector, ElementB); At this point in the code mentioned on the previous comment we might expect something like: function MixedClass(...args) {
for (const constructor of constructors) {
if (isExtendingHTMLElement(constructor)) {
copyProps(this, document.createElement(constructor.selector));
} else {
// @ts-ignore: potentially abstract class
copyProps(this, new constructor(...args));
}
if (initFunctionName !== null && typeof this[initFunctionName] === 'function') {
this[initFunctionName].apply(this, args);
}
}
} The above, at least in principles, should work since this code: Object.getOwnPropertyDescriptors(document.createElement(ElementB.selector)); Will print this: {
bar: {
value: 'bar',
writable: true,
enumerable: true,
configurable: true
}
} |
Hi @marmaak -- thank you very much for the clear description of the issue and a possible solution. I understand what's going on now, but I'm not necessarily sold on the solution. I'm hesitant to make I would almost rather allow Thoughts? Also FYI, my time is very limited for maintaining this library. I wouldn't implement this myself, but would happily review a PR if we agree on a general approach. |
Hi @tannerntannern, thanks for the quick reply. I think the solution you proposed is even better, and as you said this will give other devs the opportunity to do whatever they want in that phase. I might be able to help and contribute with a PR, not a big deal from my side. If we would like to agree on how the code should look like I also think that it will be important for other devs to have the ability to either override completely that phase (and therefore not calling import { settings } from 'ts-mixer';
settings.customCopyProps = (
self: object,
constructor: Class,
copyProps: (dest: object, src: object, exclude: string[]) => void,
...args: any[]
): void => {
// additional stuff and logic -- maybe
copyProps(self, new constructor(args), []);
// additional stuff and logic -- maybe
} And then inside the function MixedClass(...args) {
for (const constructor of constructors) {
if (settings.customCopyProps !== null && typeof settings.customCopyProps === 'function') {
settings.customCopyProps(this, constructor, copyProps, ...args);
} else {
// @ts-ignore: potentially abstract class
copyProps(this, new constructor(...args));
}
}
if (initFunctionName !== null && typeof this[initFunctionName] === 'function') {
this[initFunctionName].apply(this, args);
}
} Or do you have some better proposal? |
@marmaak Thanks for offering to help. I think what you describe isn't too far off what I imagined. Rather than allowing users to override a single export type Settings = {
initFunction: string | null,
staticsStrategy: 'copy' | 'proxy',
prototypeStrategy: 'copy' | 'proxy',
decoratorInheritance: 'deep' | 'direct' | 'none',
handleConstructors: (ctx: {
thisArg: object,
constructorArgs: unknown[],
constructors: (new (...args: any[]) => any)[],
copyProps: (dest: object, src: object, exclude: string[]) => void,
}) => void,
};
export const settings: Settings = {
initFunction: null,
staticsStrategy: 'copy',
prototypeStrategy: 'copy',
decoratorInheritance: 'deep',
handleConstructors: ({ constructors, constructorArgs, copyProps, thisArg }) => {
for (const constructor of constructors)
copyProps(thisArg, new constructor(...constructorArgs));
},
}; Then |
When mixin with Litelement/webcomponent classes you will get a
Illegal Constructor
error.The text was updated successfully, but these errors were encountered: