[Typescript] Overriden model props get 'never'/'multiple implementation' typings

[k-g-a]

Bug report

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

Sandbox link or minimal reproduction code

https://codesandbox.io/s/goofy-dewdney-u7sbo

Describe the expected behavior

Overriding prop via ModelType.props(NEW_PROPS) updates it's typings.

Describe the observed behavior

We get union type, which ends up in never for simple types (Problem #1 at sandbox) or multiple implementations for complex ones (Problem #2 at sandbox).

It seems like instead of doing (at model.ts):

props<PROPS2 extends ModelPropertiesDeclaration>(
        props: PROPS2
    ): IModelType<PROPS & ModelPropertiesDeclarationToProperties<PROPS2>, OTHERS, CustomC, CustomS>

We should do something like:

props<PROPS2 extends ModelPropertiesDeclaration>(
        props: PROPS2
    ): IModelType<Omit<PROPS, keyof PROPS2> & ModelPropertiesDeclarationToProperties<PROPS2>, OTHERS, CustomC, CustomS>

The Omit<...> part is added. But my quick experiment ended up with dozens of type errors.
I can make a PR if any solution is found.

[k-g-a]

I suspect we should do the same for volatile()/views()/actions()/extend()?

[mweststrate]

The sandbox doesn't seem to contain any code. But... I actually thought overriding props was forbidden 🤔 . Or at least it should be imho, as changing the type of a property breaks the contract of the type. (if X.y is declared as string, action X.z assumes it is a string, and then X.props() redeclares it as number, you'd be breaking X.z). So I think it is technically correct that you end up with a never...

[k-g-a]

I'm not sure why there is no code as I can see it (src/index.ts file).
Here's the picture:

And here's the code:

import { types } from "mobx-state-tree";

/* Problem #1: craetion type is 'never' for simple types */
const SimpleTypeBase = types.model({
  foo: types.string,
  bar: types.string
});

const SimpleType = SimpleTypeBase.props({
  bar: types.number,
  baz: types.number
});

// type error for 'bar' here
const simpleInstance = SimpleType.create({
  foo: "foo1",
  bar: 1,
  baz: 1
});

/* Problem #2: instance typings are ambigious for complex types */

const ComplexTypeChildString = types.model({
  foo: types.string
});
const ComplexTypeChildNumber = types.model({
  bar: types.number
});
const ComplexTypeBase = types.model({
  child: ComplexTypeChildString
});
const ComplexType = ComplexTypeBase.props({
  child: ComplexTypeChildNumber
});

const complexInstance1 = ComplexType.create({
  child: { bar: 1 }
});

// at this point child is ambigious
complexInstance1.child;

// moreover, we'll get runtime error here - although typesystem allows us such an assigment
const complexInstance2 = ComplexType.create({
  child: { foo: "foo1" }
});

In general, it seems reasonable to forbid overriding, but for now it's allowed.
I am not sure whether it's common case or not, but we use it in two ways:

• overriding generated code: A.linkedId = types.string (auto-generated from C# contract), B = A.props({linkedId: CUSTOM_REFERENCE}) - override, where CUSTOM_REFERENCE manages auto-loading of the linked entity on first access; the rest of the codebase actually never uses A;
• in case of large unions it's really hard to manage those as types.union (custom dispatcher manges runtime well, but types are really hard to follow), so we break those down, i.e.:

// union approach:

const Communication = types.model({
  type: type.enumiration(['call', 'sms', 'email', 'phone', 'messenger', ...]) // and some more types
  data: types.union(s => {
    switch(s.type) { 
      case 'call': return CallCommunicationData; 
      case 'sms': return SmsCommunicationData;
      // ... and so on
    }
  }, CallCommunicationData, SmsCommunicationData, EmailCommunicationData, ...)
})
interface ICommunication extends Instance<typeof Communication> {}

// becomes override:

const Communication = types.model({
  type: type.enumiration(['call', 'sms', 'email', 'phone', 'messenger', ...]) // and some more types
  data: BaseCommunicationData // this one holds the common properties between all the concrete CommunicationDatas, i.e. `timestamp`
});
interface ICommunication extends Instance<typeof Communication> {}

const CallCommunication = Communication.props({
  data: CallCommunicationData
});

interface ICallCommunication extends Instance<typeof CallCommunication> {}

function isCallCommunication(target: ICommunication): target is ICallCommunication {
  return target.type === 'call'
}

const SmsCommunication = Communication.props({
  data: SmsCommunicationData
});

interface ISmsCommunication extends Instance<typeof SmsCommunication> {}

function isSmsCommunication(target: ICommunication): target is ISmsCommunication {
  return target.type === 'sms'
}

// ...and so on

So we are able to use type guards to narrow down communication model to concrete variation later on, but it's typing is still ambigious.
It can be fixed by declaring the following interfaces:

// instead of:
interface ICallCommunication extends Instance<typeof CallCommunication> {}
// we use:
interface ICallCommunication extends Omit<Instance<typeof CallCommunication>, 'data'> {
  data: Instance<typeof CallCommunicationData>
}

but it's just some boilerplate, which needs to be written and sometimes is hard to understand by newcomers

Both cases seem viable, so my proposal is to embed the Omit<PROPS, keyof PROPS2> part into MST typesystem, but also add a warning about overriding existing props in dev mode. With such a warning we can keep MST flexible enough for complex cases, but prevent beginners from making mistakes.

[evelant]

I'm running into this while building models that are shared between a server and client project. I'd like each sub-project to be able to add project specific views/actions to the base shared models. This works but per above posts the types are wrong, we get an intersection of the redefined properties instead of overwriting them. For example:

//in shared project
const SharedModel1 = t.model( { foo: t.string })
const SharedModel2 = t.model({ bar: SharedModel1 })

//then in client project
const ClientModel1 = SharedModel1.views(self => .....)
const ClientModel2 = SharedModel2.props({ bar: ClientModel1 })

//now types are ambiguous even though it works fine at runtime
const m = ClientModel2.create({ bar: { foo: "asdf" }})
// type of m.bar is ClientModel1 & SharedModel1 when it should be ClientModel1

[k-g-a]

We use the following function to overcome the topic issue:

export type IOverridenModelType<BASE extends IAnyModelType, PROPS extends ModelProperties> = BASE extends IModelType<
  infer P,
  infer O,
  infer CC,
  infer CS
>
  ? IModelType<Omit<P, keyof PROPS> & PROPS, O, CC, CS>
  : never;

export function OverrideContractPorperties<BASE extends IAnyModelType, PROPS extends ModelProperties>(
  contact: BASE,
  props: PROPS
): IOverridenModelType<BASE, PROPS> {
  return contact.props(props) as IOverridenModelType<BASE, PROPS>;
}

Usage:

const FooBase = types.model('FooBase', {
  text: types.string,
  number: types.number,
  overriden: types.string,
});

const Foo = OverrideContractPorperties(FooBase, {
  overriden: types.array(types.string),
});

// just for example, to show the type output
const foo = {} as unknown as Instance<typeof Foo>;
foo.overriden; // IMSTArray<ISimpleType<string>> & IStateTreeNode<IArrayType<ISimpleType<string>>>

Hope this helps those who come here with the same problem.

[coolsoftwaretyler]

Hey @k-g-a - I really appreciate the in-depth answers and examples you put together here. This thread has been inactive for a while, so I'm inclined to close it out if I don't hear otherwise in the next two weeks or so.

Thanks, y'all!

[k-g-a]

Hey @coolsoftwaretyler! Thanks for asking.

Too my mind, we could mention somewhere in the docs the @mweststrate's note about the fact, that props overriding is not meant to be done. Alongside with that note we could mention one possible workaround from my last post.

A more ideal solution would be to leave the current approach "as is" and jsut add .override() alongside with .props() which explicitly states overriding (so the developer takes more informed decision) and of course fixes typings. Unfortunatelly, I'm not willing to make a PR atm :(

No matter the final decision, I don't mind closing this issue.

[coolsoftwaretyler]

Thanks for the follow up, @k-g-a. I'm going to toss a docs label on this for now, and perhaps when we swing back around to this item, we can also consider the additional functionality. I think one of the balancing acts we're trying to make with MST is how large our public API is, so I can't commit to adding more to it just yet, but I do agree that it's a good approach to not breaking things but giving folks more control.

I'm going to leave this open for now. I appreciate you getting back to me!