Discussion: deprecating the prop API in favor of tProp

[evelant]

There's been some discussion of this in a couple places so I thought it might be useful to have a separate issue about it. #299 #303

Currently there are two APIs for defining model properties in mobx-keystone. prop allows defining properties that only have compile time typescript types. tProp allows defining properties that have compile time typescript types and optionally the ability to check types at runtime. tProp with runtime checking disabled is identical in behavior to prop.

Per previous discussions, there might be a number of benefits to removing the prop API since identical behavior can be achieved with tProp. So far I see the following benefits:

• Remove confusion surrounding two APIs with overlapping purposes
• Potentially remove the need for $modelType
• Simplify snapshot and model creation behavior, automatically construct nested models and avoid silent runtime failures where objects are returned instead of model instances
• Easier to add new functionality since only a single API needs to be considered and runtime type info could be used to support features (ex: Enable deep optional properties on nested objects)
• Closer to the MST API, easier for people coming from MST to understand

I think there are a number of potential downsides to removing the prop API as well:

• Existing users of the prop API would eventually need to transition to tProp. I think this might be very straightforward however since tProp is a superset of prop. Perhaps it could even be handled automatically by a codemod if it's a large impact?
• Potentially slower typescript performance? I'm not sure if there's any significant overhead in inferring prop types from calls to tProp vs a direct type definition on prop.

I'm sure I'm missing things however. What do you think @xaviergonz @sisp ?

[mashaalmemon]

I just happened upon this. Having adopted the prop approach I thought I might give my two cents. Of course the maintainer's views will ultimately dictate the direction of the library, so either way I'd have to trust @xaviergonz 's judgement

• Remove confusion surrounding two APIs with overlapping purposes

This might be resolved with improved documentation and division of examples for both approaches separately.

• Potentially remove the need for $modelType

What is the current need for $modelType? Is having/maintaining it a major problem?

• Simplify snapshot and model creation behavior, automatically construct nested models and avoid silent runtime failures where objects are returned instead of model instances

The runtime failure part, I'm kind of ok with only because I'm comfortable without runtime type checking - not migrating from MST and don't expect it. But for those very comfortable with MST I can see how having silent failures would be painful.

• Easier to add new functionality since only a single API needs to be considered and runtime type info could be used to support features (ex: Enable deep optional properties on nested objects)

Can't argue with the maintenance of a single API being more straight forward.

• Closer to the MST API, easier for people coming from MST to understand

This is a pro, but at the same time constraining ideas to MST API may be a hinderance and limiting. Depends on how you look at it.

• Existing users of the prop API would eventually need to transition to tProp. I think this might be very straightforward however since tProp is a superset of prop. Perhaps it could even be handled automatically by a codemod if it's a large impact?

As an existing user of prop with about 80 models at the moment (and potentially growing) a migration would be a major undertaking.

• Potentially slower typescript performance?

One of the reasons I chose the tProp approach is precisely to avoid the typescript performance downsides. I can see how for javascript users the runtime type checking would be more beneficial. But seeing as this library seems to be geared to making typescript support first-class compared to MST, relying on the underlying typescript vs having a forced run-time typechecking approach makes sense to me (my humble opinion).

One thing that is important to me in all of this is that I need the objects that come out of mobx-keystone to implement the same interface as the object going in. With prop I can do this (as discussed in 287). I haven't dug deep into the tProp approach but I'd hope that this would also be possible.

Again this is just my two cents.

[evelant]

@mashaalmemon I think it's important to note that using tProp is not forcing runtime type checking. IIRC the behavior is identical to prop in static typescript checking and at runtime in production mode where type checks are off by default.

You can disable the runtime checks in development mode as well if you choose. The only difference I can see then between prop and tProp is a slightly different API to define your model properties. Typescript and runtime behavior are identical between prop and tProp except that tProp lets you have runtime type checking optionally if you want it.

Translating a model between the prop and tProp API I think is as simple as replacing myProp: prop<string>() with myProp: tProp(types.string) for each property -- runtime and typescript behavior wouldn't change at all.

To clarify about be closer to the MST API I only meant the familiarity of defining props using types.string, types.optional etc. I don't think the MST API with the chaining of calls to actions, views, etc is very nice. IMO mobx-keystone does a lot better since it's written in typescript where MST is a js project with TS type definitions on top.

One thing that is important to me in all of this is that I need the objects that come out of mobx-keystone to implement the same interface as the object going in

There is no difference here between the behavior of tProp and prop AFAIK, behavior is identical in all respects except for optional runtime type checking.

I think at the core of this issue is that there are some useful features that seem difficult/impossible to support without models having runtime data about their own types.

As far as I can see $modelType and $modelId are already runtime type data that are necessary only to support the prop API. Unfortunately adding runtime type data to the snapshots makes interoperation with other systems significantly more difficult -- all systems that touch data need to be aware of mobx-keystone's requirements and anywhere mobx-keystone interfaces with another system checks need to be added for all nested models that those special properties are correctly defined otherwise silent runtime failures (crashes, bugs) happen.

[xaviergonz]

v0.64.0 added a feature to alleviate this problem.

Basically, now if you use a tProp to model then $modelType is not a requirement anymore in the input snapshot (it will still show in the getSnapshot result though)

eg.

@model(...)
class M1 extends Model({
  x: tProp(0) 
}) {}

@model(...)
class M2 extends Model({
  m: tProp(M1)
}) {}

const sn = { m: { x: 10 } }
// const m2 = fromSnapshot(sn) // this still won't work without $modelType
const m2 = fromSnapshot(M2, sn); // as long as we pass the type (M2 in this case) then ok

getSnapshot(m2) // { $modelType: "m2", m: { $modelType: "m1", x: 10 } }

[sisp]

That's awesome!