Terse API for Mixin

[kitsonk]

The review of dojo/compose by the SitePen/dgrid (@maier49, @kfranqueiro, and @edhager) as highlighted that the mixin API should/could be significantly more terse, especially when dealing with complex type compositing. Ideally the API would work more like this:

const fooFactory = compose({
    mixins: [ aFactory, BConstructorFunction, CClass, dObjectLiteral ],
    aspect: {
        before: { 'foo': aspectBeforeFunction },
        around: { 'bar': aspectAroundFunction },
        after: { 'baz': aspectAfterFunction }
    },
    init: initFunction
});

const foo = foo({ foo: 'bar' });

I expect the interface would need to be something like this:

interface ComposeAspectDefinition {
    before?: { [ method: string ]: AspectBeforeMethod };
    around?: { [ method: string ]: AspectAroundMethod };
    after?: { [ method: string ]: AspectAfterMethod };
}

interface ComposeBaseFactoryDefinition {
    aspect?: AspectDefinition;
    init?: InitFunction | InitFunction[];
}

interface ComposeSingleFactoryDefinition<A extends Object, O> extends ComposeBaseFactoryDefinition {
    mixins: ComposeFactory<A, O> | GenericClass<A, O> | A | [ComposeFactory<A, O> | GenericClass<A, O> | A];
}


interface ComposeTwoFactoryDefinition<A extends Object, B extends Object, O, P> extends ComposeBaseFactoryDefinition {
    mixins: [ComposeFactory<A, O> | GenericClass<A, O> | A, ComposeFactory<B, P> | GenericClass<B, P> | B];
}
// etc...

interface Compose {
    <A extends Object, O>(ComposeSingleFactoryDefinition<A, O>): ComposeFactory<A, O>;
    <A extends Object, B extends Object, O, P>(ComposeTwoFactoryDefinition<A, B, O, P>): ComposeFactory<A & B, O & P>;
    // etc...
}

[kitsonk]

Sorry @maier49 didn't see your PR #21. I will review that.

[maier49]

I'm wondering about the implications of the API example you provided. It looks like the main compose function, under this proposal, would now expect an object with mixin, aspect, and init functions. If this is the case, would I still be able to call compose with a plain object? I'm assuming the typing could be overloaded so that an object matching the mixin, aspect, init formula would go down one path, and any other object would go down the other. But this might get tricky because we now have to enforce via typing whether the user meant to create a type that has a mixin property or pass a mixin. Keeping the base compose function as is, and moving this functionality to the mixin function seems like it would avoid those issues and leave a clearer API. I'm not sure how serious of a concern this is, but I wouldn't want to complicate the more straightforward use cases too much in our effort to accommodate more complex ones.

The second thought I had is in relation to providing an array of mixins, and my thinking is definitely being influenced by dgrid here so I may well be overlooking other use cases. It seems more likely that somebody would want to pass a single mixin, aspect, and init function (or some combination thereof), rather than passing several mixins with one aspect object and one init function. I can see the usefulness of providing the overloads for passing multiple mixins though.

On the tersemixin branch, currently the types are as follows:

/* Mixin API */
export interface ComposeClassMixin<O, P> {
    base?: GenericClass<P>;
    initializer?: ComposeInitializationFunction<O, P>;
    aspectAdvice?: AspectAdvice;
}

export interface ComposeFactoryMixin<O, P, T> {
    base?: ComposeFactory<T, P>;
    initializer?: ComposeInitializationFunction<O, P>;
    aspectAdvice?: AspectAdvice;
}

export interface ComposeFactory<O, T> {
    mixin<U, V>(mixin: ComposeClassMixin<V, U>): ComposeFactory<O, T & U>;
    mixin<P, U, V>(mixin: ComposeFactoryMixin<V, U, P>): ComposeFactory<O & P, T & U>;
}

export interface Compose {
    mixin<O, A, P, B>(base: ComposeFactory<O, A>, mixin: ComposeClassMixin<P, B>): ComposeFactory<O, A & B>;
    mixin<O, P, A, B, C>(base: ComposeFactory<O, A>, mixin: ComposeFactoryMixin<C, B, P>): ComposeFactory<O & P, A & B>;
}

function mixin<O, A, P, B>(base: ComposeFactory<O, A>, mixin: ComposeClassMixin<P, B>): ComposeFactory<O, A & B>;
function mixin<A, B, O, P, T>(base: ComposeFactory<A, O>, mixin: ComposeFactoryMixin<T, P, B>): ComposeFactory<A & B, O & P>;

}

In order to handle the equivalent to the ComposeTwoFactoryDefinition, what do you think about extending that along these lines?

export interface Compose {
        // The existing signatures
    mixin<O, A, P, B>(base: ComposeFactory<O, A>, mixin: ComposeClassMixin<P, B>): ComposeFactory<O, A & B>;
    mixin<O, P, A, B, T>(base: ComposeFactory<O, A>, mixin: ComposeFactoryMixin<T, B, P>): ComposeFactory<O & P, A & B>;
        // Overloads for multiple mixins
       mixin<O, A, P, B, Q, C>(base: ComposeFactory<O, A>, mixin: ComposeClassMixin<P, B>, secondMixin: ComposeClassMixin<Q, C>): ComposeFactory<O, A & B & C>;
    mixin<O, P, A, B, Q, C, T, U>(base: ComposeFactory<O, A>, mixin: ComposeFactoryMixin<T, B, P>, secondMixin: ComposeFactoryMixin<U, C, Q>): ComposeFactory<O & P & Q, A & B & C>;
}

[kitsonk]

Sorry, back into things again. Your points are valid...

If this is the case, would I still be able to call compose with a plain object?

I was thinking outloud versus being pendantic... I would say though let's not be afraid to change the main API if it deals with the main use case of the library. The feedback as I understood was that to we were finding the get X done, it was too verbose and actually we would like to throw something specific at it which just sort of "works" that was as "approachable" as Dojo1's declare.

I would suggesting thinking of it though from how you would like to write the code and then we can try to figure out how to make it work with type inference. What I think you are saying is you feel it works better like this:

const fooFactory = compose({
  foo: 'bar'
}, fooInit);

const fooBarFactory = mixin(fooFactory, {
  mixins: [ Bar, bazFactory, { qat: 2 } ],
  aspect: { /* ... */ },
  init: initFunction
});

/* or */

const fooBarFactory = compose({
        foo: 'bar'
    })
    .mixin({
        mixins: [ Bar, bazFactory, { qat: 2 } ],
        aspect: { /* ... */ },
        init: initFunction
    });

If we think that the majority of the operations are going to be just taking an object literal with an init function and creating a factory, then I would say, let's not touch the compose() API, but if we think that was we are going to have a mixture of base factory creation and extending already existing factories, then I think splitting that API out makes more sense as you are suggesting.

[maier49]

I do think we will still want to be able to support the base case of an object and an optional init function with the compose function. Especially because in theory your base class shouldn't need to aspect its own functions, which leaves only a class/prototype object and possibly an initialization function to worry about.

The only part where there's still a difference between the current implementation and your latest code example is where multiple mixins are passed.

const fooBarFactory = compose({
        foo: 'bar'
    })
    .mixin({
        mixins: [ Bar, bazFactory, { qat: 2 } ],
        aspect: { /* ... */ },
        init: initFunction
    });

Would currently be

const fooBarFactory = compose({
        foo: 'bar'
    })
    .mixin(
      {
        base: Bar,
        aspect: { /* ... */ },
        init: initFunction
      },
      {
        base: bazFactory
      },
      {
        base: { qat: 2 } 
      }
   );

This is obviously a little more verbose for this case, but if all you want to do is add multiple objects to modify the type, extend would still probably be the appropriate part of the API to use. The idea for mixin as I'm thinking about it is to handle the case where your object/factory/class that you want to mixin also has some other behavior in the form of an initalizing function or aspects that need to go along with it. To get the same convenience in the case that you do just want to extend the prototype with multiple objects, extend could be modified to take multiple arguments. If you think it's much more readable though, I can modify the signature to use the mixins array as in your examples

Edit: Fixed a typo in the example code

[kitsonk]

@maier49 and I discussed this verbally, summary of the conversation was:

• The suggestion of .mixin() makes sense, though the property base could have something better syntactically.
• The suggestion of clarifying .extend() to be more useful.
• We will document that if you are mixing in a Compose Factory, compose will integrate any init functions, but if a constructor function or an ES6 class is passed, any logic contained in the constructor will not be absorbed. Related to Using ES6 and TypeScript classes as a base #5.
• The support for Ambient Decorators in TypeScript 2.0 (see Add Support for design-time decorators microsoft/TypeScript#2900) might provide easier ways to deal with edge cases around typing.
• Generics are still challenging with compose, though F-bound polymorphism might help to a degree (see Type parameters as constraints microsoft/TypeScript#5949).
• Dealing with conflict resolution is still a challenge, as we can't control the type inference.

[kitsonk]

TL;DR your suggestion around mixin works!

@maier49 in using compose, I am finding often when I have a mixin, I want/need to provide an init function, because there is a feature in that mixin which I wan't to use. I don't know why before this we hadn't noticed it, but it is really awkward.

For example, if I have something like this:

import compose from 'compose';

const createDestroyable = compose({
    own(handle: Handle): void {
        /* own a handle */
    },
    destroy(): void {
        /* destroy handles */
    }
}, () => { /* init handle array */ });

const createSomething = compose({
        foo: null
    })
    .mixin(createDestroyable);

I want to be able to create something with a handle and have it owned during the construction of createSomething. I can currently put the init function in the root compose, but the instance will only be typed as the root compose and won't have the mixin, so no "own" property, so I have to do any casting to get it to work, which seems silly.

[kitsonk]

@maier49 first, really good work!

Some feedback on the tersemixin branch.

For the initializer function in a mixin, you don't get the right type inferred for options, but I would expect to be able to. For example:

interface AOptions {
  foo?: string;
}

const createB = compose({ bar: 1 });

const createA = compose<AOptions, { foo: string; }>({ foo: 'bar' },
    (instance, options) => { /* instance and options correct */ })
  .mixin({
    mixin: createB,
    initializer(instance, options) {
      /* options will be of type {} */
      /* instance will be of type { foo: string; } & { bar: number; } */
      /* options should be of type AOptions & {} */
    }
  });

Also, I know we talked offline about the ordering of the init functions and the only one we had left up in the are is the base initializer. Taking a look at using this, I realised that while mixins should generally be isolated from each other, I found a use case where my methods in my base depended on features from mixins and in my init function, I called some of those methods. The expectation would then be that those methods it would call would not fail, therefore it would have expected that base initializer would have run last.

It would be "nice" to have ComposeMixin take an array/tuple... I know it becomes a pain without (Variadic Kinds)[https://github.com/microsoft/TypeScript/issues/5453]. Consider the following, currently with tersemixin:

const createWidget: WidgetFactory = compose(createEvented)
    .mixin({
        mixin: createDestroyable
    })
    .mixin({
        mixin: createRenderable
    })
    .mixin({
        mixin: createStateful
    });

Or the following:

const createWidget: WidgetFactory = compose(createEvented)
    .mixin({
        mixins: [ createDestroyable, createRenderable, createStateful ]
    });

I am not suggesting getting rid of the mixin property on the ComposeMixin, but I am suggesting some sort of mixins property. I think I found a way to avoid the silliness of overloading, take a look at this playground:

interface ComposeFactory<O, T> {
    (options?: O): T;
}

interface GenericClass<T> {
    new (...args: any[]): T;
    prototype: T;
}

interface ComposeInitializationFunction<O, T> {
    (instance: T, options?: O): void;
}

interface AspectAdvice { }

type ComposeMixinItem<O, T> = GenericClass<T> | T | ComposeFactory<O, T>;

interface ComposeMixin<O, P, Q, R, S, T, U, V, W, X> {
    mixins?: [ ComposeMixinItem<P, U>, ComposeMixinItem<Q, V> ]
        | [ ComposeMixinItem<P, U>, ComposeMixinItem<Q, V>, ComposeMixinItem<R, W> ]
        | [ ComposeMixinItem<P, U>, ComposeMixinItem<Q, V>, ComposeMixinItem<R, W>, ComposeMixinItem<S, X> ]
    mixin?: ComposeMixinItem<P, U>;
    initializer?: ComposeInitializationFunction<O & P & Q & R & S, T & U & V & W & X>;
    aspectAdvice?: AspectAdvice;
}

function mixin<O, P, Q, R, S, T, U, V, W, X>(mixin: ComposeMixin<O, P, Q, R, S, T, U, V, W, X>): ComposeFactory<O & P & Q & R & S, T & U & V & W & X> {
    return; /* who cares about details*/
}

const createSomething = mixin({
    mixins: [ { foo: 'bar' }, { bar: 12 }, { baz: false }, { qat: / / } ],
    initializer(instance, options) {
        instance.foo;
        instance.bar;
        instance.baz;
        instance.qat;
    }
});

const something = createSomething();

something.foo;
something.bar;
something.baz;
something.qat;

In using the tuples, the type inference appears to collapse and the resulting type is inferred correctly.

One final though... I am realising that the generic ordering we have in compose is a bit silly... Originally I had that <O /*mapped to options*/, T /*mapped to instance type*/> made a whole lot of sense, but in usage, it is confusing! Maybe we should just kill that off and it should be <T /*mapped to instance type*/, O /*mapped to options*/>.

[maier49]

These issues should be addressed now.
I did a sweep over all the generics to try to be more consistent in the use of T and O for the type and options, and swapped the order whenever they were reversed.

The argument to mixin can now also have a mixins property that can be provided instead of mixin and can contain 2-5 types to be mixed in.

The type of the options argument is now being properly inferred from the base and mixin. One thing that I noticed though, is that the unspecified generics are taking the empty object type, so for example with something like:

compose({
    foo: ''
}, function (instance: { foo: string }, options: { foo?: string } ) {
    if (options.foo) {
        instance.foo = options.foo;
    }
}).mixin({
    mixin: { bar: number },
    initializer: function (instance, options) {
    }
});

The inferred type of the instance and options arguments of initializer would be, respectively:
{ foo: string } & { bar: number } & {} and { foo?: string } & {}.

Allowing only one mixin to be passed at a time helps, but doesn't completely resolve this problem. It helps because as long as you pass a compose factory then the instance and options type can be inferred from that and there are no extra generic types to collapse to {}. Where it doesn't help is when a generic class or a plain object is used as the mixin instead of a ComposeFactory. In that case the options type will still end up being inferred to something & {}. Although, in that case I think it's much more acceptable. If the initializer type has not been specified, and is not explicitly provided in the function definition then it's probably not as significant that it defaults this way.

Going back to overloads would eliminate this problem, but then we are back at the problem of that pushing complexity onto users of the library.

[kitsonk]

@maier49 first, Happy Birthday! 🎂 🍰

Second, I took a look and agree that type inference on options is always collapsing to any. I suggest that if we are "happy" with the features of the API, we just let that rest for now and wait for variadic types, which I think will be the only way we can solve the problem.

I started to add some JSDoc to the compose.ts to make intellisense make a bit of sense.

I think the type ComposeMixinItem could actually be leveraged else where in the module (and might have a better name) since that is our foundational argument in most cases.

I ran into a problem when passing factories in the mixins property. While object literals worked fine, factories seemed to blow up the type inferrence. Other than adding a test to re-create the issue, I couldn't figure out exactly what was going on. It maybe, like in other functions, the mixing of ComposeFactories with object literals and GenericClasses is causing issues, though just chaining the mixins like before works perfectly fine. Anyways, consider it a birthday present for you! 😄

[maier49]

Based on our discussions on the issue, I removed the mixins argument in favor of using a single call and modifying that for multiple arguments if/when variadic kinds are introduced to TypeScript.

Also based on our discussion, I overloaded the mixin signature to allow for anything with a factoryDescriptor method in addition to the already existing object type. While method overloading seems to work fine for this, I ran into some type inference issues when trying to express both types as a union. I don't think that's a big problem though, this only represents one overload and it's for an actually different method signature.

The ComposeFactory now provides a default implementation of factoryDescriptor which returns the factory itself as the mixin property, allowing ComposeFactorys to be consumed directly via the mixin function. This will hopefully make the mixin function the defacto method for consuming 'compose modules', when those modules are not being used on their own.

I also went a step further and made the ComposeFactory implementation of factoryDescriptor check the ComposeFactory prototype for _aspectAdvice and _initializer properties, and add those to the mixin as aspectAdvice and initializer, respectively. I'm not completely sold on this idea, but it could potentially be useful.
The factoyDescriptor function no longer does anything special with _aspectAdvice or _initializer properties.

[kitsonk]

Closed via 77042c0