Fix thunk and computed alias

[arielhs]

This is a followup to this PR.

Suppose you had a model where you had factored out the Thunk and Computed types into their own aliases like this:

type MyModelAction<TPayload = void> = Action<MyModel, TPayload>
type MyModelComputed<TResult> = Computed<MyModel, TResult>
type MyModelThunk<TPayload = undefined, TResult = any> = Thunk<MyModel, TPayload, any, {}, Promise<TResult>>

type MyState = {
    value: string
}

type MyThunkPayload = { data: string }

interface MyModel {
    myState: MyState

    myComputed: MyModelComputed<number>

    setMyState: MyModelAction<string>

    myThunk: MyModelThunk<MyThunkPayload, boolean>

}

const myModel: MyModel = {
    myState: {
        value: 'initial'
    },

    setMyState: action((state, payload) => {
        state.myState = {
            value: payload
        }
    }),

    myComputed: computed(state => state.myState.value.length),

    myThunk: thunk(async (actions, payload) => {
        actions.setMyState('a')
        // some kind of await
        return true
    })
}

Since the Thunk and Computed types don't actually use the Model type parameter in their resulting type, it is "lost" when we make a type alias wrappers. This means the computed() and thunk() generic functions infer their type parameters like this when attempting to use them on the model instance:

computed<{}, any, {}, StateResolvers<{}, {}>>

thunk<{}, MyThunkPayload, any, {}, Promise<true>>

Whereas we would expect them to be inferred like this:

computed<MyModel, number, {}, StateResolvers<MyModel, {}>>

thunk<MyModel, MyThunkPayload, any, {}, Promise<true>>

The result on the model instance is:

// Property 'myState' does not exist on type 'StateMapper<_Pick<{}, never>>'.
myComputed: computed(state => state.myState.value.length),

and

//Property 'setMyState' does not exist on type 'Actions<{}>'
actions.setMyState('a')

This PR addresses this by adding type markers which forces TypeScript to capture these type variables, fixing the target typing on the model instance.

This has also been used to capture the StoreModel and Injections type parameters.

A concern I have is if someone was using these types in an unconventional way e.g.

Before:

// "type" | "result"
type Before = keyof Computed<any, any>;

After:

// typeof ModelTypeMarker | typeof StoreModelTypeMarker | "result" | "type"
type After = keyof Computed<any, any>;

So in this sense it's a breaking change.

Fixes: #634

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
easy-peasy	✅ Ready (Inspect)	Visit Preview	Nov 13, 2022 at 0:08AM (UTC)

[jmyrland]

Hi @arielhs ! Thanks for this contribution 👏

I added a typescript test to cover this issue, but I was unable push it to your repository due to insufficient permissions. Could you add this test yourself? (I just copied your example into a external-type-defs.ts file in the tests/typescript folder)

This PR addresses this by adding type markers which forces TypeScript to capture these type variables, fixing the target typing on the model instance.

Awesome!

A concern I have is if someone was using these types in an unconventional way e.g.

Good point. I don't expect this to be heavily utilized, but if so, it is an easy fix for the consumer.

[jmyrland]

Sadly, when testing this on a large code base, there are some issues. This does not seem to be working well for composed models. I'll try to reproduce this in a small example & post back here.

[jmyrland]

Sadly, when testing this on a large code base, there are some issues. This does not seem to be working well for composed models. I'll try to reproduce this in a small example & post back here.

I think I was able to reproduce this - it is atleast one of the issues:

import { thunk, Thunk } from 'easy-peasy';

type Injections = any;

interface IModelActions<T> {
  someThunk: Thunk<this, T, Injections, IStoreModel>;
}

interface IModel<T> extends IModelActions<T> {
  someValue: T;
}

interface IStoreModel {
  a: IModel<string>;
  b: IModel<number>;
}

const createModelActions = <T>(): IModelActions<T> => {
  return {
    someThunk: thunk(() => {}),
  };
};

const storeModel: IStoreModel = {
  /*
    Type '{ someThunk: Thunk<IModelActions<string>, string, any, IStoreModel, any>; someValue: string; }' is not assignable to type 'IModel<string>'.
      Types of property 'someThunk' are incompatible.
        Type 'Thunk<IModelActions<string>, string, any, IStoreModel, any>' is not assignable to type 'Thunk<IModel<string>, string, any, IStoreModel, any>'.
          Property 'someValue' is missing in type 'IModelActions<string>' but required in type 'IModel<string>'.ts(2322)

    example.ts(10, 3): 'someValue' is declared here.
    example.ts(15, 3): The expected type comes from property 'a' which is declared here on type 'IStoreModel'
  */
  // ❌ Results in the error above
  a: {
    someValue: 'hello',
    ...createModelActions<string>(),
  },

  // ✅ works fine
  b: {
    someValue: 123,
    someThunk: thunk(() => {}),
  },
};

If I remove the marker types in the definition, the problem is resolved:

export type Thunk<
  Model extends object,
  Payload = undefined,
  Injections = any,
  StoreModel extends object = {},
  Result = any,
> = {
  type: 'thunk';
  payload: Payload;
  result: Result;
  // 👇 When these are "removed", the issue above dissappears 🤔
  // [ModelTypeMarker]?: Model;
  // [InjectionsTypeMarker]?: Injections;
  // [StoreModelTypeMarker]?: StoreModel;
};

I'm not sure how to resolve this issue. We should be able to compose stores, as the example above with the a-store.

[arielhs]

@jmyrland I'll look at the example now and see if i can find another way that works. I've also added you as a collaborator to my fork. And thank you for the example, should make chasing this down much easier

[jmyrland]

@jmyrland I'll look at the example now and see if i can find another way that works. I've also added you as a collaborator to my fork. And thank you for the example, should make chasing this down much easier

Sweet - I pushed the tests 👍

[arielhs]

@jmyrland I found a way to capture the type parameters without creating the assignability issues that would break the composed models like you described in your example. I've just pushed, let me know if this works against the real example - the large codebase you described.

I am confident it is a safe technique, see the "Methodish" example here if you are interested

[jmyrland]

@jmyrland I found a way to capture the type parameters without creating the assignability issues that would break the composed models like you described in your example. I've just pushed, let me know if this works against the real example - the large codebase you described.

I am confident it is a safe technique, see the "Methodish" example here if you are interested

Awesome! You're a TS wizard! 😁

That fixed the specific issue - but this is whack-a-mole, and another one popped up (generalized output):

Interface 'IStoreModel' cannot simultaneously extend types 'IModelA<IStoreModel, IModelB<IStoreModel, any>>' and 'IModelC'.
Named property 'someAction' of types IModelA<IStoreModel, IModelB<IStoreModel, any>>' and 'IModelC' are not identical.

I'll see if I can construct a test to reproduce this issue, unless you immediately see the fix.

[arielhs]

@jmyrland I found a way to capture the type parameters without creating the assignability issues that would break the composed models like you described in your example. I've just pushed, let me know if this works against the real example - the large codebase you described.
I am confident it is a safe technique, see the "Methodish" example here if you are interested

Awesome! You're a TS wizard! 😁

That fixed the specific issue - but this is whack-a-mole, and another one popped up (generalized output):

Interface 'IStoreModel' cannot simultaneously extend types 'IModelA<IStoreModel, IModelB<IStoreModel, any>>' and 'IModelC'.
Named property 'someAction' of types IModelA<IStoreModel, IModelB<IStoreModel, any>>' and 'IModelC' are not identical.

I'll see if I can construct a test to reproduce this issue, unless you immediately see the fix.

Lol I'm happy to keep whack-a-moling, please do make the example

[jmyrland]

Lol I'm happy to keep whack-a-moling, please do make the example

I've added a new testcase for complex store inheritance, extrapolated from existing code. It is quite convoluted - but if this passes, so should the existing code.

[jmyrland]

Elegant solution @arielhs 👏

I've tested the new types against the "Large Codebase ™️" and all is good ✅

[arielhs]

Lol I'm happy to keep whack-a-moling, please do make the example

I've added a new testcase for complex store inheritance, extrapolated from existing code. It is quite convoluted - but if this passes, so should the existing code.

Thanks again for creating an example. I feel a bit silly, the solution ended up being much simpler... just change Thunk and Computed to interfaces! It actually makes sense because the whole cause of this issue is that newer versions of TypeScript are more aggressive at simplifying type aliases, but interfaces are never simplified away.

[jmyrland]

Thanks again for creating an example. I feel a bit silly, the solution ended up being much simpler... just change Thunk and Computed to interfaces! It actually makes sense because the whole cause of this issue is that newer versions of TypeScript are more aggressive at simplifying type aliases, but interfaces are never simplified away.

My pleasure - I very much appreciate the simple solution of using interface 👍

Let's get this released when @ctrlplusb has a moment to spare! 🚀

[jmyrland]

Thanks again for your contribution @arielhs ! 👏 This will be included in the upcoming release this week.