When using with LitElement/Webcomponent getting `Illegal Constructor` error

[andreabosman16]

When mixin with Litelement/webcomponent classes you will get a Illegal Constructor error.

[tannerntannern]

Hi @andreabosman16, thanks for finding this. I don't have experience with WebComponents, but my understanding is this is an issue related to extending HTMLElement in general, not ts-mixer specifically.

You will get this error if you forget to call customElements.define and try to instantiate the component. [...] To fix it, define it first: customElements.define(...);

Please correct me if I'm wrong, but at first glance this doesn't look like a ts-mixer issue to me

[andreabosman16]

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.

[tannerntannern]

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?

[andreabosman16]

I am going on vacation now i will give you a sample in two weeks if that is okay with you

[tannerntannern]

I'm in no rush. Enjoy your vacation 🙂

[marmaak]

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 HTMLElement class (exactly the same that LitElement is doing):

class MyElement extends HTMLElement {
    public awesomeNewFeature(): string {
         return 'Whoa!!!'
    }
}

Than you want to create many different WebComponents that should extend both this MyElement class and maybe many others. To do this you'll start using ts-mixer also to avoid using that cumbersome notation for creating mixins (a function that return an anonymous class or similar) and you'll end up with some code like 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 document.createElement('element-a') otherwise you will get an Illegal Constructor error (the same mentioned in the title).

The issue seems to be in the function MixedClass (mixins.ts, line 223) where the code try to copy properties from ancestors by instantiating them though the new keyword, but when it will come to MyElement (and as a consequence to HTMLElement) it will fire that error at runtime.

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!

[marmaak]

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 HTMLElement and only in this scenario instead of calling the new operator use the document.createElement.

To achieve the above one assumption should be mandatory, the fact that each class extending the HTMLElement should have a static property where the selector is stored, something like:

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
    }
}

[tannerntannern]

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 ts-mixer aware of web components at all because it's meant as a front-end/back-end agnostic library. I'm also hesitant to further add the restriction that web component classes have a static selector property.

I would almost rather allow ts-mixer users to hook into that constructor loop shown above with a custom handler function. This way, if you need the behavior described above, it could be achieved without forcing ts-mixer to be aware of web components or enforce arbitrary restrictions on them. This handler function could theoretically be added to the global settings object described in the README. You could even publish a package specifically for this custom handler function to allow other ts-mixer + web components users to benefit. A "ts-mixer web components adapter" of sorts.

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.

[marmaak]

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 copyProps at all) or just add something before/after it, so I guess we should provide a way for them to call somehow the original copyProps function, maybe it can be passed as a parameter to the handler:

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 MixedClass function just check for the existence of the handler and call it, otherwise we will stick to the previous code:

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?

[tannerntannern]

@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 copyProps call however, I think we'd have more flexibility by simply overriding the entire loop. I haven't tested it, but I imagine something like this in the settings.ts could work:

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 MixedClass would simply invoke settings.handleConstructors with the proper dependencies in place of the constructor loop. Thoughts on this approach?