tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

Mixin Parent Class does not receive Child Class's "this". #20

Closed Judahh closed 4 years ago

Judahh commented 4 years ago

When using regular heritance:

class Parent{
  constructor() {
    console.log('Parent name:'+ this.constructor.name)
  }
}

class Child extends Parent{
  constructor() {
    super()
    console.log('Child name:'+ this.constructor.name)
  }
}

let child = new Child();

Outputs:

Parent name:Child
Child name:Child

But when I use Mixin from ts-mixer:

import { Mixin } from 'ts-mixer';
class Parent {
  constructor() {
    console.log('Parent name:' + this.constructor.name);
  }
}

class Child extends Mixin(Parent) {
  constructor() {
    super();
    console.log('Child name:' + this.constructor.name);
  }
}

const child = new Child();

Outputs:

Parent name:Parent
Child name:Child

So it's not getting the correct "this"

tannerntannern commented 4 years ago

Hi @Judahh, I probably should have made this more explicit in the caveats section of the README, but this is expected behavior. Unfortunately, ES6 makes it impossible to call constructors as regular functions, which is necessary to get the "correct" behavior.

Something I've considered previously (but haven't seen the demand for it until now) is adding a configuration option to ts-mixer that causes it to automatically call a designated init function, which can be called with the proper this. I was thinking it would look something like this:

import { Mixin, Settings } from 'ts-mixer';

Settings.initFunctionName = '__init__';  // `null` would disable this behavior

class Foo {
    private __init__() {
        // do stuff that requires proper `this` during "construction"
    }
}

class Bar {
    public constructor(a: string, b: number) {
        // do stuff that doesn't require proper `this`
    }

    // could even forward args from constructor if needed
    private __init__(a: string, b: number) {
        // do stuff that requires proper `this`
    }
}

const FooBar = Mixin(Foo, Bar);

const fooBar = new FooBar('foo', 42);
// order of events under the hood:
// * Foo instance is constructed and properties copied over to new fooBar object
// * Bar instance is constructed and properties copied over to new fooBar object
// * __init__ from Foo is .apply()'d to new fooBar object (with 'foo' and 42, but they are ignored)
// * __init__ from Bar is .apply()'d to new fooBar object with 'foo' and 42

Obviously, this is pretty clunky, but I've spent many hours wrestling with this problem and I don't think there's another way around it, aside from keeping your constructors pure.

Is this be a feature you'd be interested in?

Judahh commented 4 years ago

Yes It would help me a lot

Judahh commented 4 years ago

One question, is there a beta version for this release or is not so simple to implement?

tannerntannern commented 4 years ago

Hi @Judahh, thanks for your patience. I've started working on this but I haven't gotten a chance to push anything out to npm yet. I'm hoping to get more time this evening. I will post a link here when I have a beta version ready to go!

lucastorress commented 4 years ago

I need this too. Waiting for release of this beta version! @tannerntannern

tannerntannern commented 4 years ago

Thanks for your patience everyone. The beta release is available. You will have to use npm install ts-mixer@5.2.0-beta.0 in order to get the beta rather than 5.1.0. I've also added a section in the docs explaining a bit more about the feature.

Please let me know what you think! If there are no issues, I'll promote it to an actual release in a few days.

Judahh commented 4 years ago

Looks like when you have more than 3 levels of heritance 'this' starts to stop working

Judahh commented 4 years ago

Hi @Judahh, I probably should have made this more explicit in the caveats section of the README, but this is expected behavior. Unfortunately, ES6 makes it impossible to call constructors as regular functions, which is necessary to get the "correct" behavior.

Something I've considered previously (but haven't seen the demand for it until now) is adding a configuration option to ts-mixer that causes it to automatically call a designated init function, which can be called with the proper this. I was thinking it would look something like this:

import { Mixin, Settings } from 'ts-mixer';

Settings.initFunctionName = '__init__';  // `null` would disable this behavior

class Foo {
    private __init__() {
        // do stuff that requires proper `this` during "construction"
    }
}

class Bar {
    public constructor(a: string, b: number) {
        // do stuff that doesn't require proper `this`
    }

    // could even forward args from constructor if needed
    private __init__(a: string, b: number) {
        // do stuff that requires proper `this`
    }
}

const FooBar = Mixin(Foo, Bar);

const fooBar = new FooBar('foo', 42);
// order of events under the hood:
// * Foo instance is constructed and properties copied over to new fooBar object
// * Bar instance is constructed and properties copied over to new fooBar object
// * __init__ from Foo is .apply()'d to new fooBar object (with 'foo' and 42, but they are ignored)
// * __init__ from Bar is .apply()'d to new fooBar object with 'foo' and 42

Obviously, this is pretty clunky, but I've spent many hours wrestling with this problem and I don't think there's another way around it, aside from keeping your constructors pure.

Is this be a feature you'd be interested in?

Is there an ecma version that solves this problem?

tannerntannern commented 4 years ago

Looks like when you have more than 3 levels of heritance 'this' starts to stop working

Thanks for catching. I will try to look into this later tonight. If you could provide a minimal code snippet that reproduces the issue, it will help me incorporate it into my test suite faster. 👍

Is there an ecma version that solves this problem?

Sort of, but it probably won't help you here. Previous versions of ts-mixer used to behave differently depending on whether your compile target was set to es5 or es6. The reason for this was that TypeScript transforms class expressions into the older function style when targeting pre-ES6. For example:

class Foo {
    method() {}
}

// when compiling to es5...
function Foo() {}
Foo.prototype = {
    method: function() {}
};

Unlike ES6 classes, this older style can be used for calling constructors without new. So ts-mixer would detect this and apply this the "correct" way for these old style classes. However, having split behavior for different compile targets didn't seem like a good idea to me, especially now that ES6 is 5 years old and fewer and fewer people are going to be targeting pre-ES6. So this behavior was removed.

Judahh commented 4 years ago

Thanks for catching. I will try to look into this later tonight. If you could provide a minimal code snippet that reproduces the issue, it will help me incorporate it into my test suite faster. 👍


import { Mixin, settings } from 'ts-mixer';
settings.initFunction = 'init';

const assignObject = (type, object) => { return Object.assign(new type(), object); };

abstract class ClassA { public name;

constructor() { this.name = this.constructor.name; } init() { this.name = this.constructor.name; } }

class ClassB extends ClassA { public name1; init() { super.init(); this.name1 = this.name + 1; } }

class ClassC extends ClassA { public name2; init() { super.init(); this.name2 = this.name + 2; } }

class ClassD extends ClassA { public name3; init() { super.init(); this.name3 = this.name + 3; } }

class ClassE extends Mixin(ClassB, ClassC) {}

class ClassF extends Mixin(ClassE, ClassD) {}

test('test Mixin', async (done) => { const b = new ClassB(); const c = new ClassC(); const d = new ClassD(); const e = new ClassE(); const f = new ClassF();

expect(b).toStrictEqual(assignObject(ClassB, { name: 'ClassB' })); expect(c).toStrictEqual(assignObject(ClassC, { name: 'ClassC' })); expect(d).toStrictEqual(assignObject(ClassD, { name: 'ClassD' })); expect(e).toStrictEqual( assignObject(ClassE, { name: 'ClassE', name1: 'ClassE1', name2: 'ClassE2', }) ); expect(f).toStrictEqual( assignObject(ClassF, { name: 'ClassF', name1: 'ClassF1', // RECEIVING name1: 'ClassE1' name2: 'ClassF2', name3: 'ClassF3', }) );

done(); });


Another problem is that is not possible to override the init method

class ClassD extends ClassA { public name3; init(blob) { super.init(); this.name3 = this.name + 3 + blob; } }

Judahh commented 4 years ago

As soon as fixed, please post a beta version. Thanks

lucastorress commented 4 years ago

Yes, waiting for it. Thanks.

tannerntannern commented 4 years ago

I apologize for keeping you waiting... I have been looking into this, but I haven't quite narrowed down why it's happening yet. In the meantime, could you provide a little more context about why you need this particular pattern? I still plan on fixing the bug, but it would be nice to know more about how people are using this library.

tannerntannern commented 4 years ago

v5.2.0-beta.1 is now available. I believe this fixes the issue you raised @Judahh. The bug was related to mixing classes that were already mixed by ts-mixer. This is something the library should be able to handle just fine (now that the bug is fixed), but it is a bit of an anti-pattern.

To illustrate, this is a similar hierarchy to what you posted in your test snippet:

But ts-mixer can mix more than two classes at a time, so you may find it better to skip the intermediate E class and do this instead:

Of course, you may have had a good reason for using the intermediate E class, but I just wanted to point out the possibility.

Judahh commented 4 years ago

Thanks, but for my problem is not possible to skip, because is a two project problem. There are two projects. One used by the other, and just a fraction is visible to the other and both use ts-mixer. So it's not possible to skip a class.

tannerntannern commented 4 years ago

Hi all, once again let me know if you run into any issues. If not I'll promote it to a full release in a few days!

tannerntannern commented 4 years ago

5.2.0 has been released. Closing for now.