Static Factory Methods #26

Closed
kitsonk opened this Issue Mar 16, 2016 · 8 comments

Projects

None yet

2 participants

@kitsonk
Member
kitsonk commented Mar 16, 2016

I realised we don't have an API to add static methods to a factory, which makes it difficult to actually specify those when typing factories. I would see something like this:

interface Static {
    <F extends ComposeFactory<T, O>, T, O, S>(factory: F, staticProperties: S): F & S;
}

And integrating that into the compose API like the other methods, and we would be able to do something like this:

interface Foo {
  bar: number;
}

interface FooFactory extends ComposeFactory<Foo, any> {
  foo(): string;
}

const createMy: FooFactory = compose({
    bar: 1
  })
  .static({
    foo() { return 'foo'; }
  });

createFactory.foo();

(also, wouldn't that be our preferred method for overriding the mixin descriptor too? Possibly a more clean way of dealing with other bits we don't want in the prototype)

@kitsonk kitsonk added the enhancement label Mar 16, 2016
@maier49 maier49 was assigned by kitsonk Mar 16, 2016
@kitsonk kitsonk added this to the beta.1 milestone Mar 16, 2016
@maier49
Contributor
maier49 commented Mar 18, 2016

Do you have any thoughts on how we should handle passing an existing compose factory with static properties to mixin or create? Should the new factory get the old one's static properties automatically, or should I have to explicitly add them. By default it would be the latter, but we could potentially add signatures like these:

create<T, O, P, S>(base: ComposeFactory<T, O> & S, initFunction?: ComposeInitializationFunction<T, P>): ComposeFactory<T, O & P> & S;

mixin<T, O, U, P, R, S>(
    base: ComposeFactory<T, O> & R,
    mixin: ComposeMixinable<U, P> & S
): ComposeFactory<T & U, O & P> & R & S;

Without having tried it out yet, I feel like this would be the more intuitive behavior.

@maier49
Contributor
maier49 commented Mar 18, 2016

This has been added to the tersemixin branch, and test cases/documentation have been updated as well. Currently the functionality described in my previous comment has not been implemented, so a separate call to static would need to be made to pass the static properties from one ComposeFactory to another. I can put the code on a separate branch if we want to isolate this feature from the rest of the stuff on the tersemixin branch.

@kitsonk
Member
kitsonk commented Apr 1, 2016

I think the expectation would be that if I extend a factory, or mixin in a factory, I would get the same sort of static resolution as other resolution (last one wins). I agree the latter would be more intuitive.

@kitsonk kitsonk modified the milestone: April 2016, beta.1 Apr 8, 2016
@maier49
Contributor
maier49 commented Apr 8, 2016

I've updated the API for the static method to reflect our discussions on it. It now looks like this:

export interface ComposeFactory<T, O> {
    static<C extends this, S>(staticProperties: S): C & S;
}

As we discussed, the end result is that static properties can be added to a ComposeFactory using its static method, but chaining will only maintain the last set of static properties. This can be remedied by casting the factory to an interface at intermediate steps or passing a generic type at each step. I added a section in the README to elaborate on this.

mixin and extend will maintain the static properties on a factory, but not the type. The problem with using a signature like the one for static is that the final type needs to have an intersection of the generics in the factory and not an intersection of the factories.

export interface ComposeFactory<T, O> {
    mixin<C extends this, D extends ComposeFactory<U, P>, U, P>(mixin: D): C & D // Doesn't have correct types for ComposeFactory generics
}

export interface ComposeFactory<T, O> {
    mixin<U, P>(mixin: ComposeFactory<U, P>): ComposeFactory<T & U, O & P>// Loses static properties
}
@kitsonk
Member
kitsonk commented Apr 14, 2016

Ok, having look at this, it is the best we can do. I think documenting it up should be the situation.

Maybe what we do is always "drop" static methods when moving from one factory to another, so the only way to add static properties is on a "final" version of the factory? So we only have static<S>(staticProperties: S): this & S;. They would "disappear" in the types post that, and we could ensure they "disappear" when we clone the factory.

@maier49
Contributor
maier49 commented Apr 14, 2016

I agree with that assessment. I've made some updates to the documentation on the tersemixin branch. The implementation was already in line with this(the changes I mentioned in my last comment were on my fork). In light of our previous discussion on the issue I've made on further change, which is to use a weak map to track the static properties for a given compose factory. This allows you to do something like composeFactoryA.static(composeFactoryB); without it trying to copy over properties on composeFactoryB that are normally handled internally, such as mixin, extend, etc.

@kitsonk
Member
kitsonk commented Apr 15, 2016

Ok, I will take one last look, but I think we have done what we can for now and it would be good to merge this in with master and do a release, which I will do later on.

@kitsonk
Member
kitsonk commented Apr 15, 2016

Closed via 77042c0

@kitsonk kitsonk closed this Apr 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment