mobx-state-tree
mobx-state-tree copied to clipboard
Array and model types are not inferred correctly when broken down into their components
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.
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.
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.
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.