Thanks for the proposal. I'll take a look at it this weekend.

Let me add a couple of thoughts.

• The relationship of errors (using a union type leads to disjunctive errors) should be retained in the error collection in order to be able to present users with correct feedback about alternative errors.

I think errors and their relationships could be represented along the lines of this:

enum TypeCheckKind {
  Error,
  And,
  Or
}

interface TypeCheckError {
  readonly kind: TypeCheckKind.Error
  readonly path: Path
  readonly expectedTypeName: string
  readonly actualValue: any
  readonly custom: any // How to properly deal with custom error data in a type-safe way?
}

interface TypeCheckExpression {
  readonly kind: TypeCheckKind.And | TypeCheckKind.Or
  readonly args: ReadonlyArray<TypeCheckError | TypeCheckExpression>
}

Most error expressions have the TypeCheckKind.And kind, but errors combined by the or type have the TypeCheckKind.Or relationship. Perhaps the TypeCheckError class can be reused/extended.

• Error information should be aggregated towards the root of the tree, so that the root model contains the complete collection of errors. This is useful, e.g., to determine whether a state tree contains any errors at all and to display all errors in a state tree in a dedicated view (e.g. think of VS Code's error panel).
• How to propagate errors from a parent model to its children models? Imagine a composition of models where a parent model adds additional constraints to (some of) its children. When a child model is used in the context of a parent model, the validation errors of the child model that are added by the parent model should also be included in the error collection of the child model.

Since parent models can impose additional constraints on their children, the complete set of errors is only available at the root of the state tree in the general case. But I'd like each model to have access to its complete set of errors (including the errors of its children), some of which may be coming from a parent model. Perhaps a context could be used:

const typeCheckContext = createContext<TypeCheckError | TypeCheckExpression | undefined>()

Whenever a model is in the context of typeCheckContext (i.e. typeCheckContext.getProviderNode(this) !== undefined because typeCheckContext.get(this) === undefined is ambiguous as undefined also means "no errors"), it means there is a parent model which provides (possibly more comprehensive) error information, so the model uses this error information instead of performing type-checking itself. The error information is propagated from a parent to its nearest children by setting the error context for each of these children, perhaps along the lines of this:

onInit() {
  onChildAttachedTo(
    () => this,
    child => {
      // not sure if there is a more efficient way of only reacting to attachment/detachment
      // of only the nearest children
      if (!isNearestParentOf(this, child)) { // imagine this function existed
        return
      }
      const path = getParentToChildPath(this, child)!
      typeCheckContext.setComputed(child, () => {
        // filter all errors whose paths begin with `path` and remove this prefix
        //
        // `filterTypeCheckErrors` is assumed to be tolerant of `undefined`, i.e.
        // `filterTypeCheckErrors(undefined, ...) -> undefined`
        return filterTypeCheckErrors(this.$errors, { // See below about `$errors`
          path,
          prefix: true, // `true` when `path` is a prefix, `false` when `path` is an exact match
          strip: true, // `true` when `path` should be left-stripped from the matching errors
        })
      )
      return () => typeCheckContext.unset(child)
    },
    { deep: true, fireForCurrentChildren: true }
  )
}

• How would the collected errors be exposed? As a builtin computed model property (e.g. model.$errors similar to model.$ with regard to naming)?

Perhaps like this:

@computed.struct
get $errors(): TypeCheckError | TypeCheckExpression | undefined {
  return typeCheckContext.getProviderNode(this)
    ? typeCheckContext.get(this)
    : this.typeCheck() // I guess ...?
}

On second thought, providing the errors without onChildAttachedTo may be even simpler:

onInit() {
  // set the error context:
  // (a) if the context does not exist or this node provides the context,
  //     perform error checking and provide the result
  // (b) if the context exists and is provided by another node, just pass
  //     it along
  typeCheckContext.setComputed(this, () => {
    const root = typeCheckContext.getProviderNode(this)
    return !root || root === this
      ? this.typeCheck() // I guess ...?
      : typeCheckContext.get(this)
  })
}

@computed.struct
get $errors(): TypeCheckError | TypeCheckExpression | undefined {
  // the provider node cannot be undefined because it's either this node
  // or a parent node
  const root = typeCheckContext.getProviderNode(this)!
  const path = getParentToChildPath(root, this)!
  return filterTypeCheckErrors(typeCheckContext.get(this), {
    path,
    prefix: true,
    strip: true,
  })
}

Here, again only the root model performs the type-checking for the entire tree and provides the errors via the context, but now each child model simply extracts the errors related to itself and its subtree using the path from the root to itself instead of using the path from its nearest parent to itself. It is a bit redundant that each model sets the context in onInit even though the context may already exist (which means all non-root models just pass the context along as is), but this way detaching a model should immediately lead to type-checking happening at this model because the error context is no longer provided by a parent, and attaching a model should immediately lead to using the existing error context from a parent.

I believe that the current runtime types perform caching, so changing one value in the state tree would not necessarily lead to type-checking the entire tree from scratch. This makes type-checking just at the root of the tree efficient.

I've been thinking about this a bit and with refinement types it seems like it's so close to being there, but the concept, allowing a value to exist with errors, seems to have a nasty way of "poisoning" the tree for lack of a better term. What I mean is once it's anywhere it kind of seems like it has to be everywhere.

For something to be really pervasively added, I think idea backing it needs to be fundementally sound/useful otherwise it would eat away at a purity that mobx-keystone feels like it has at the moment.

I think I would personally like to see it added as part of draft() or something very similar, something like...

// keep validation APIs in line with dirty-checking
// maybe make a new thing like "ValidatedDraft" or something along those lines
interface Draft {
  ...

  isValid: boolean
  isValidByPath(path: Path): boolean

  errors: TypeCheckError[];
  errorsByPath(path: Path): boolean
}

Not sure if this is possible, but I would then expect something of a custom cloning that "unrefines" all the refinement types in the original back to their base types, allowing the draft to contain the invalid, malformed, work-in-progress data. Your checkFn in the original model would be your validator.

isValid and your errors are just kind of saying "hey, if you tried to commit this draft right now it wouldn't work and this is the error you'd get from the refinement type".

I think Drafts being a one-stop-shop for wiring up anything form-y or input-y feels right, at least to me.

I like the idea of making comprehensive runtime validation a feature of draft. With the current type-checking in place, the original state would strictly enforce the type constraints while the draft would use the same type constraints but collect all errors instead that, e.g., can be provided as user feedback. It sounds like a clean design to me. I hadn't thought about it like this. In my app, errors are regular state because it is non-trivial to reach a valid state. Still, certain actions can only be performed when the state has no errors. In order to collect all errors, I couldn't use refinements, so I created a custom runtime type system similar to mobx-keystone's runtime types. Following your suggestion of making validation a draft feature, in my case I would operate exclusively on the draft in order to be able to collect and present errors.

I agree that the extended draft interface should follow the original one, but I think an array of TypeCheckError is not sufficient to represent the error structure. Based on your proposal and using the error interfaces I defined in #160 (comment), the extended draft interface could look like this:

interface Draft {
  ...

  isValid: boolean
  isValidByPath(path: Path): boolean

  errors: TypeCheckError | TypeCheckExpression | undefined;
  errorsByPath(path: Path): TypeCheckError | TypeCheckExpression | undefined
}

I noticed another requirement that the runtime validation should satisfy in my opinion. While mobx-keystone's current runtime type-checking only applies to model (state) properties, I think a comprehensive runtime validation should also be able to validate computed properties and volatile state. The design of the current runtime type-checking does not cover this case as the types are specified as part of the model (state) properties definition.

In addition, what happens if I need to refine the model type, e.g. in order to add a constraint that checks the combination of several props? The current runtime type system lets me refine types.model, but I can't refine Model().

I noticed another requirement that the runtime validation should satisfy in my opinion. While mobx-keystone's current runtime type-checking only applies to model (state) properties, I think a comprehensive runtime validation should also be able to validate computed properties and volatile state. The design of the current runtime type-checking does not cover this case as the types are specified as part of the model (state) properties definition.

In addition, what happens if I need to refine the model type, e.g. in order to add a constraint that checks the combination of several props? The current runtime type system lets me refine types.model, but I can't refine Model().

After thinking about it some more, I think that class models mix model prop definition and runtime type-checking in an unnecessarily strict way. Let me elaborate a bit more. A class model is created by inheriting from the class created by Model(...). The Model factory receives an object of props which define the state of the model. When a model is created with runtime types, the model state is defined along with a type definition that is checked at runtime whenever a prop is modified. Runtime types can only be specified for each prop individually which means a refinement across props does not seem possible. How about changing the signature of the Model factory like this?

class MyModel extends Model(/* which object props are model props? */, /* runtime type */) { ... }

For example:

class MyModel extends Model({
  kind: prop(),
  value: prop()
},
types.or(
  types.object(() => ({
    kind: types.literal('float'),
    value: types.number
  })),
  types.object(() => ({
    kind: types.literal('int'),
    value: types.integer
  }))
)) { ... }

The runtime type cannot be arbitrary of course. It must be a types.object and the object keys must be fixed even when it is a union of objects like in the example above. The first object { kind: prop(), value: prop() } indicates which of the object keys are model props and can optionally define default values. So it is possible to define a types.object which contains more keys than those specified in the first object. In this case, the remaining keys are ordinary class properties or getters, e.g. volatile state and computed props. I think this separation allows a complete runtime type specification of the model including both model (state) props, volatile state, and computed properties. It also allows to specify dependent model props like in the example above using, e.g., types.or.

Opinions?

First of all, sorry it took me this long to respond.

I tend to agree with @Sladav in that "application state" modelling and "user input" state are not exactly the same (although usually the user input is a less strict version of the application one).
For example, maybe for the application state it doesn't make sense to have two users with the same username, but as a user input it (temporarily until corrected) does.

In the first case it would be a hard constraint, something that if were to ever break would make the application state invalid. That can be already covered via throwing / returning an error before the state becomes invalid (e.g., throwing when a user with an already existing username is about to be added to the list of users), or a refinement type (which is basically a throw error check too).

For the second case (user input state) it is true that mobx-keystone doesn't directly offer a tool for this, but maybe in this case just following a pattern is enough to solve it?

For example, if the programmer followed a pattern like:

• given a custom error class

interface BaseError {
  path: Path // path to the child in which the error(s) is located
}

interface MyError extends BaseError {
  error: string; // might be a string that might be translated, already translated, only English, a code...
}

• model classes might (optionally) have a member that returns all current and children errors

@computed
get $errors(): Array<MyError> {
  const errors: MyError[] = []

  // local errors
  if (this.name.length > 30) errors.push("name too long", ["name"])

  // children errors, might be done custom (member by member) or by auto aggregation of children
  getChildrenObjects(this, { deep: true }).forEach(child => {
    const childErrors = child.$errors;
    if (childErrors) {
      childErrors.forEach(childError => {
        errors.push({
          ...childError,
          path: [ getParentPath(child).path, ...childError.path ]
        });
      });
      errors.push(...childErrors);
    }
  })
  return errors;
}

It is true however that one way to make this pattern easier would be to wrap the code from the second part (getChildrenObjects etc) into some kind of "getErrors" function and document the pattern to kind of make it more "official". Also I think this pattern would be easily applicable to drafts. Would that be enough?

I also agree with @Sladav and you about "application state" vs. "user input state", but I had only seen it this way for standard user input forms although the idea, of course, extends to more complex editing, too. Unless I'm missing something, I believe the "user input state" is always a relaxed version of the "application state", so as @Sladav suggested for validating "user input state" only the base types should be strictly enforced (an exception is thrown when they are violated) while refinement errors are merely collected for user feedback.

I'm not convinced that the pattern you sketched is sufficiently generic though:

1. Writing validation logic imperatively does not scale well in my experience; it becomes a pain when validation logic becomes complex. Schema-based validation using, e.g., mobx-keystone's runtime types or io-ts, is a much better approach in my opinion. Since mobx-keystone uses schema-based validation for model props already, I think it should also be used for validating entire models and trees.
2. Using an array of errors does not retain the logical relationship of errors ("and" vs. "or" - First-class support for comprehensive runtime validation  #160 (comment)) which may be relevant to accurately report errors. Instead, it implies only the "and" relationship which is correct in most cases (in my experience) but not always.
3. With schema-based validation, collecting errors only from children to parents is not sufficient because parent nodes may introduce additional constraints on their children. The solution may be to provide the errors at the root of the tree to its children via a context and each child extracts its corresponding errors.

Related to the above and in continuation of my thoughts in #160 (comment), I think specifying runtime types independently for each model prop is not sufficient. What if the "application state" had a coupling of props, e.g. according to the following schema?

types.or(
  types.object(() => ({
    kind: types.literal('float'),
    value: types.number
  })),
  types.object(() => ({
    kind: types.literal('int'),
    value: types.integer
  }))
)

When expressing kind and value as two props, the above type cannot be applied for type-checking. The only way to type-check this data structure is to have a single model prop whose value is this object. In addition, I think that validating not only model props (i.e. state) but also computed properties is needed in the general case. For instance, think of the typical shopping cart example where the total price is computed in a computed property. If, for instance, there was a budget that may not be exceeded by a customer, which could be a model prop, then validation should also check that the budget is not exceeded. Of course, the budget type could be refined to check whether the budget value exceeds the sum of the item prices and their quantities, but this formula is already implemented in the computed property and would need to be reimplemented in the type refinement (yes, the function could be extracted and called in both places, but I hope you get my point).

How about this: Let's give the model decorator a second (optional) argument being the type/schema of the entire model. It could replace tProp entirely or just serve as an additional, more comprehensive type. (I realized that Model is not the right place for providing this type.)

@model(
  "UnionModel",
  types.or(
    types.object( // needs to become tolerant of additional properties
      () => ({
        kind: types.literal("float"),
        value: types.number,
      })
    ),
    types.object( // needs to become tolerant of additional properties
      () => ({
        kind: types.literal("int"),
        value: types.integer,
      })
    )
  )
)
class UnionModel extends Model({
  kind: prop<"float" | "int">(),
  value: prop<number>(),
}) {}

@model(
  "ComputedPropertyModel",
  types.object(() => ({ value: types.number })) // needs to become tolerant of additional properties
)
class ComputedPropertyModel extends Model({}) {
  get value(): number {
    return 10
  }
}

I've successfully implemented this feature (to some extent) to see if it works (also in terms of Typescript typings). In my opinion, this feature would be valuable on its own already without the distinction between "application state" and "user input state". But with this more comprehensive type/schema in place, I believe we would be one step closer to supporting error collection for "user input state" (only when type-checking is performed in a draft).

@sisp
About the example above, couldn't you do it like this instead?

@model("Float")
class Float extends Model({
   kind: tProp(types.literal("float")),
  value: tProp(types.number),
}) {}

@model("Int")
class Int extends Model({
   kind: tProp(types.literal("int")),
  value: tProp(types.integer),
}) {}

@model("Root")
class Root extends Model({
  // choose one or the other
  value: tProp(types.or(types.model<Float>(Float), types.model<Int>(Int)))
}) {}

or assuming they don't need to be full models

const floatType = types.object(() => {
   kind: types.literal("float"),
  value: types.number,
})

const intType = types.object(() => {
   kind: types.literal("int"),
  value: types.integer,
})

const floatOrIntType = types.or(floatType, intType)

@model("Root")
class Root extends Model({
  value: tProp(floatOrIntType)
}) {}

@xaviergonz: Yes that's right, although in both cases the prop value is needed just to be able to express the "or" relationship. If this was the only problem, I could live with it, but this approach doesn't allow me to validate a computed property easily either.

Let's look at a shopping cart, for instance:

const quantityType = types.refinement(types.integer, (n) => n > 0)
const moneyType = types.refinement(types.number, (n) => n >= 0)

@model("ShoppingItem")
class ShoppingItem extends Model({
  quantity: tProp(quantityType),
  unitPrice: tProp(moneyType),
}) {}

@model("ShoppingCart")
class ShoppingCart extends Model({
  items: tProp(types.array(types.model(ShoppingItem))),
  budget: tProp(moneyType),
}) {
  @computed
  get totalPrice() {
    return this.items.reduce((total, item) => total + item.quantity * item.unitPrice, 0)
  }
}

I'd like to be able to check (using the same declarative runtime type system) that totalPrice does not exceed budget and get an error (ideally for totalPrice) when it exceeds budget. This requires runtime type-checking beyond model props. With the API sketch from above where a runtime type is passed to the model decorator, it could look something like this:

const shoppingCartType = types.refinement(
  types.object({
    items: types.array(types.model(ShoppingItem)),
    budget: moneyType,
    totalPrice: moneyType
  }),
  (cart) => cart.totalPrice <= cart.budget,
)

@model("ShoppingCart", shoppingCartType)
class ShoppingCart extends Model({
  items: tProp(types.array(types.model(ShoppingItem))),
  budget: tProp(moneyType),
}) {
  @computed
  get totalPrice() {
    return this.items.reduce((total, item) => total + item.quantity * item.unitPrice, 0)
  }
}

Do you see my point?

If we added a typeCheck() optional method to models (that gets invoked as part of the runtime type checking), would it achieve the same?

@model("ShoppingCart")
class ShoppingCart extends Model({
  items: types.array(types.model(ShoppingItem)),
  budget: moneyType,
}) {
  // would get called as part of the runtime validation
  typeCheck() {
    if (this.totalPrice <= this.budget) {
      // TypeCheckError could have a new overload where you can provide an error message instead of type/actualValue
      return new TypeCheckError(["totalPrice"], "above budget")
    }
  }

  @computed
  get totalPrice() {
    return this.items.reduce((total, item) => total + item.quantity * item.unitPrice, 0)
  }
}

and for non class model types then "refinement" would still be used (although could be enhanced with an optional error message)

@xaviergonz: Yes that's right, although in both cases the prop value is needed just to be able to express the "or" relationship.

Actually you can make that value even a root store:

const valueModel = fnModel(floatOrIntType, "FloatOrIntModelName")
const value = valueModel.create({ kind: "float", value: 3.14 })
registerAsRootStore(value) // if needed

// or if no runtime type checking is need

type FloatOrInt = { kind: "float", value: number } | { kind: "int", value: number }
const valueModel = fnModel<FloatOrInt>("FloatOrIntModelName")
const value = valueModel.create({ kind: "float", value: 3.14 })
registerAsRootStore(value) // if needed

// or even just as a plain object
type FloatOrInt = { kind: "float", value: number } | { kind: "int", value: number }
const value: FloatOrInt = toTreeNode({ kind: "float", value: 3.14 })
registerAsRootStore(value) // if needed
typeCheck(floatOrIntType, value) // if desired

If we added a typeCheck() optional method to models (that gets invoked as part of the runtime type checking), would it achieve the same?

Well, for this particular refinement, it would be the same I guess. But in order to validate a more complex computed property, e.g. an array of back-references, I think it would be nice to use the declarative type system instead of writing imperative validation logic. A workaround may be to declare the type separately and call the typeCheck function in your sketched new typeCheck method manually using this type to type-check this (beyond the model props). But that's pretty much the same as my suggested new model decorator argument if that type-check got invoked by model.typeCheck() too, e.g. by intersecting the two types using a types.and(...) type. The model decorator argument could probably also be moved to the Model/ExtendedModel function instead if that makes more sense.

Related to the above, I've been having some trouble experimenting with these ideas when trying to type-check a model when using types.model(SomeModel) (which implicitly happens when calling model.typeCheck()) which only applies the type-checker of the model props to model.$ instead of model, so only the props (before transformations) can be checked.

Or how about something like this?

@model("ShoppingCart")
class ShoppingCart extends Model(
  // TS types are inferred from the runtime types below.
  // Default values and transforms can be provided here.
  {
    items: prop(),
    budget: prop(),
  },
  // Optional runtime types. If omitted entirely or no runtime
  // type is available for a model prop above, TS type needs
  // to be provided to that `prop` above explicitly.
  types.refinement(
    types.object({
      items: types.array(types.model(ShoppingItem)),
      budget: moneyType,
      totalPrice: moneyType,
    }),
    (cart) => cart.totalPrice <= cart.budget
  )
) {
  @computed
  get totalPrice() {
    return this.items.reduce((total, item) => total + item.quantity * item.unitPrice, 0)
  }
}

Model class inheritance would correspond to intersecting the runtime types of the base class and the child class (using a types.and type). The TS types for the model props would be inferred using the same approach that I've taken in #195 b8e24b1 (using Unionize to determine the type union for each model prop separately). I haven't tested it yet though.

What do you think? I really think that keeping type-checking declarative as much as possible is a good idea.

The last idea seems to require TS types for abstract class properties derived from a mapped type which I don't think is (currently) possible. When the runtime type is provided as the second argument of the Model function, any non-model property needs to be typed as an abstract class property in the abstract class returned by Model(...) to ensure that the class that extends from that abstract class implements those properties.

Unless I'm missing something, I'd say that using a decorator to provide the validation runtime type is the best option.

@xaviergonz Not sure if you've missed my comments in #195 or didn't have time yet or perhaps absolutely dislike that PR, but is there any chance you could check it and give some feedback? I really think this is a great feature and that the implementation integrates nicely with the rest of mobx-keystone. I'm still a bit uncertain whether setting the validation type in the @model decorator is the best solution, but it seems to work well for class models. I haven't found an equivalent place to add the validation type to functional models though. I'd very much appreciate your feedback. 🙂