Add runtime type validation

[sisp]

This is an attempt at making a step forward towards supporting comprehensive runtime type validation. Closes #160.

So far, I've implemented support for specifying a (possibly complex) runtime type that can be passed to the model decorator. This approach, in contrast to specifying runtime types as model props, allows for more complex runtime type specifications, e.g. union types for a model (see #160 (comment) and the added test for a union of objects), as well as runtime type-checking of class instance properties and computed properties.

All changes are intentionally non-breaking for now at the cost of having an additional way of defining runtime types. If this idea receives positive feedback, I envision it to replace tProp props long-term. Then, model classes would use prop for all model props (so no runtime type-checking) and runtime type-checking could be added by passing a runtime type to the model decorator.

@xaviergonz and possibly others: What do you think?

TODO

• Support type-checking with types.model (i.e. model composition) - 86c1972
• Support collecting all type-checking errors in a tree as an alternative to bailing out at the first error - 5e46a60
• Support representing complex error relationships (both conjunctive and disjunctive errors) - 5e46a60
• Support propagating type-checking errors from a parent to a child, e.g. an error resulting from a refinement of a child defined by a parent should be accessible by the child - 870fefd

[terrysahaidak]

Hey, great work on it. Even with cryptic messages sometimes MST always points to the right spot and validates things. It helped a lot. Sometimes I miss it in keystone.

The only concern I have right away agains the current approach you took - the whole point of runtime type checks in MST was to be able to produce TS types from it. No need to write twice same thing.

I suppose mobx-keystone is trying to follow this mantra.

[sisp]

Thanks! 🙂

I share your concern about needing to specify prop types twice now, although they are not identical. To clarify the difference between (a) the runtime type provided via the model decorator and (b) the prop TS types, (a) spans all model props, class instance props, and computed props, and can express complex inter-dependencies among all of them while (b) is only about model props and the TS type of one prop is the union of all types this prop can have independent of the other props.

I think the main difference between what both MST and mobx-keystone are doing with runtime types and what I'm proposing is the difference between "parsing" and "validation". MST and mobx-keystone focus on parsing model props (i.e. decoding/checking props individually) whereas I'm aiming at validating entire models (including constraints across multiple model props, class instance props, and computed props).

[sisp]

I've completed a draft of how I think runtime type validation could be integrated into mobx-keystone. @xaviergonz What do you think?

[sisp]

@xaviergonz I know this PR is quite large and I'm not sure whether I've convinced you of the benefits I see in this feature yet. But I'm curious what you think about the current implementation. I'm quite happy with it at this point. Any feedback is much appreciated.

Something that's still missing is support for functional models. But I haven't found a consistent way of adding the validation type to a runtime model yet. I thought it might be better to get some feedback from you first before addressing functional models.

[sisp]

@xaviergonz Any chance you could check this PR and give feedback? I think it's such a great feature. 🙂

[evelant]

This feature looks great, anything blocking getting it merged?

[sisp]

Thanks, @AndrewMorsillo! 🙂

I think some of it is not bad, but there are some flaws in its approach that I have come to realize. For instance, type validation cannot simply reuse types.model(...) because both the validation type and the data type (i.e. the one that is specified via tProp) would be checked against even though, e.g., a validation shall be performed, and vice-versa. A variant of types.model(...) for validation could be added, but this would be confusing in my opinion. Or perhaps there is a way to distinguish between validation and data type checks using a context. Also, the fact that I haven't found any other library that uses logical expressions to combine errors makes me wonder whether I'm on the wrong track.

I think this PR requires some more conceptual work, but it could be a good starting point. If you have ideas how to improve it, I'd be happy to discuss and exchange ideas.

I think #271 is relevant, too, but I haven't found the time to finish a first draft yet.

[sisp]

How about adding a flag to types.model(...) like types.model(M, { instance: true }) or types.model(M, true) to indicate that the instance should be type-checked using the validation type instead of using the model data specified using tProp?

@model("Child", types.object(() => ({ value: types.integer })))
class Child extends Model({
  value: prop<number>(),
}) {}

@model("Parent", types.object(() => ({ child: types.model<Child>(Child, true) })))
class Parent extends Model({
  child: prop<Child>(),
}) {}

@xaviergonz Some feedback would be very helpful, so we can go forward here.

1. Are you generally interested in and happy with this feature?
2. Are you fine with having a data type error contain all errors instead of bailing out at the first error? This way, we can reuse the type checkers for both data and validation type checking.
3. Are you fine with the slight redundancy of specifying data types via tProp and validation types via the @model decorator? In fact, they are only the same in trivial cases, but I'd say in most cases the validation type will be more complex than the data type.

[evelant]

@sisp I like that this approach allows for both data and validation checking cases.

The redundancy of specifying types in both the decorator and the model might become tedious however. Imagine a model with 75+ properties, nested objects, etc. It would be quite easy to forget one and it likely wouldn't be easy to read.

This is just a quibble about the shape of the API however, not the functionality. Maybe there is a way to make it more ergonomic? I don't have a thorough enough understanding of the mobx-keystone code yet to know if it's possible. Would it be possible to specify the validation type along with the model type such as:

@model("M")
class M extends Model({
  value: prop<number>().withValidator(types.integer),
}) {}

If an API like this is possible it would eliminate the need to declare the fields/shape of the model twice.

[sisp]

Yes, with a large number of props the redundancy is suboptimal. At least TS checks that the validation types are compatible with the data types, so some errors are caught that way.

Your suggested API is unfortunately not sufficient. One of the test cases I added in this PR, "union - complex", tests validation of a discriminated union (without a discriminator though) which wouldn't work with per-prop validation types. Also, in my opinion the validation type should be able to check non-model props like computed props. That's why I propose to set the validation type via the @model decorator. It should be possible to "generate" model props from the validation type, however, I think validation is just an optional extension of a model, so the model spec should not rely on the presence of a validation type.

[evelant]

Ah I understand now. I think your approach is a good one. It seems like having this level of "redundancy" is likely not avoidable if we want full validation of complex types and non model props. I think it probably would be just fine like this in practice, the slightly increased verbosity would greatly pay off in having good validation capabilities.