mobx-state-tree icon indicating copy to clipboard operation
mobx-state-tree copied to clipboard

Array and model types are not inferred correctly when broken down into their components

Open ada-waffles opened this issue 4 years ago • 2 comments

Bug report

  • [x] I've checked documentation and searched for existing issues
  • [x] I've made sure my project is based on the latest MST version
  • [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

https://codesandbox.io/s/great-sea-tpn6b?file=/src/index.ts

Describe the expected behavior

An array type should have its creation, snapshot, and instance types correctly inferred when passed to a function that infers types as their components.

Describe the observed behavior

The instance type is inferred as C | T due to a strange behavior in TypeScript.

I have already opened an issue with TypeScript here: https://github.com/microsoft/TypeScript/issues/43168

In the case of MST, the definition of IType#is at https://github.com/mobxjs/mobx-state-tree/blob/v5.0.1/packages/mobx-state-tree/src/core/type/type.ts#L101 causes this issue due to the interaction with the conditional type in STNValue. The issue doesn't occur for named interfaces unless they are subtyped, which is why this isn't affecting all types, just those such as arrays which subtype IType.

I'm opening this issue to raise awareness in case anyone wants to try working around the issue in MST.

For now I'm working around the issue by manually passing <Creation[] | undefined, Snapshot[], IMSTArray<typeof Type>> to the functions like union that need the components. It's ugly, but it works.

ada-waffles avatar Mar 09 '21 19:03 ada-waffles

This also affects models, since they also subtype IType.

I've found a less verbose workaround in the form of a helper function/interface:

export interface ITypeComponents<C, S, T> {
    readonly CreationType: C;
    readonly SnapshotType: S;
    readonly TypeWithoutSTN: T;
}

export const asIType = <C, S, T>(type: ITypeComponents<C, S, T>): IType<C, S, T> => type as IType<C, S, T>;

Passing the type inference through that interface avoids the issue: t.union(asIType(MyModel), asIType(t.array(MyOtherModel)))

I imagine an approach like this would be workable as an internal solution as well.

ada-waffles avatar Mar 13 '21 01:03 ada-waffles

Hey @ada-waffles - I'm sorry it took so long for anyone to get back to you here. Really appreciate the time you put into this.

Jamon and I are trying to give MST some of the attention it sorely deserves. One of the big items we're talking about is improved TS support, and I think this falls in that realm.

I don't have a path forward quite yet, but I'm going to label this issue and keep it open.

I know it's been literally years, but if you have a soft spot in your heart for MobX-State-Tree (or perhaps a business-critical need for it) and want to pitch in, we'd welcome the help. If not, no worries, just glad you took the time to put this issue together at all.

coolsoftwaretyler avatar Jun 28 '23 03:06 coolsoftwaretyler

Hey folks, we finally got around to this. Huge thanks to @thegedge for resolving it in https://github.com/mobxjs/mobx-state-tree/pull/2151.

Thanks for hanging in there with us on this one. We will release a preview build with the changes soon, although we may need to go slowly with a full release since this is one of those nuanced "is it a bug fix or a breaking change" kind of updates.

coolsoftwaretyler avatar Mar 01 '24 15:03 coolsoftwaretyler