Setting defaults for props on nested objects

[evelant]

A model may contain may small nested objects, some of which might need defaults for their fields (optional at creation or snapshotIn time). Currently there is no way to handle that except for making each nested object its own top level uniquely named model which comes with another set of problems.

Example scenario:

@model("MyModel")
export class MyModel extends Model({
    foo: tProp(types.string, "bar"), //top level defaults are fine
    someNestedStuff: tProp(types.object(() => ({
          requiredNestedProp: types.string,
          optionalNestedProp: types.number // ??? there's no way to set a default for this -- it should be 4 if not specified           
    })))
}) {}

I think what's needed is a types.optional where we can specify a default in nested objects. The above example would then be:

@model("MyModel")
export class MyModel extends Model({
    foo: tProp(types.string, "bar"), //top level defaults are fine
    someNestedStuff: tProp(types.object(() => ({
          requiredNestedProp: types.string,
          optionalNestedProp: types.optional(types.number, () => 4) 
    })))
}) {}

Without this capability it becomes necessary to either create extra top level models or to remember what the defaults should be at creation time which is error prone.

[sisp]

Just a brief comment: Deeply nested default values require a schema like you've shown above, which only exists when using tProp and runtime types. It wouldn't work without runtime types, i.e. when using prop. That said, I think it would be fine to support deeply nested default values only when using runtime types. Just my opinion though. 😉

[xaviergonz]

Proposal - What about something like this (so it works also in non runtime type checked scenarios):

someNestedStuff: prop<whatever>({ default value if undefined}).mergeWith(() => ({
  optionalNestedProp: 4
}))

where mergeWith would be deeply merged and applied only if the value is an object (not undefined, null, etc)

[evelant]

@xaviergonz I think that might work but wouldn't be the most ergonomic API since default properties would need to get specified "twice". ex:

    someNestedStuff: prop<{x: number, y: string}>().mergeWith(() => ({y: "foo"}))

I think that also makes it unclear what the type of someNestedStuff.y is. Is it y: string or y?: string? It should be y?: string when creating the model but y: string when reading it.

[xaviergonz]

yeah, I agree it is less ergonomic, but I'd rather create an API that works for both cases (some people don't use tProps at all, like @sisp if I remember correctly)

You can get the type while creating by using ModelCreationData<Model> (it would be y?: string), and the type while reading by using ModelData<Model> (it would be y: string).

This is already true for props with default values, for example, prop<string>("s") has a ModelCreationData of y?: string and a ModelData of y: string

[sisp]

Although I don't use tProps at the moment, I see no alternative to using tProps in this case, and I don't think the suggested mergeWith(...) method would be sufficient. Think of a conditional default value like this (with a hypothetical types.optional that supports a default value):

class M extends Model({
  x: tProp(
    types.or(
      types.object(
        () => ({
          kind: types.literal("int"),
          value: types.optional(types.integer, 0),
        })
      ),
      types.object(
        () => ({
          kind: types.literal("string"),
          value: types.optional(types.string, "some default string")
        })
      )
    )
  )
}) {}

The default value depends on the kind field which requires conditional logic. With runtime types, this shouldn't be a problem. It's actually very similar to how JSON Schema libraries use the default keyword to fill in missing values.

Something similar could be considered for deeply nested transformations. Think of the recently added new prop transform feature but for deeply nested values. Again, JSON Schema libraries typically support transforming string fields annotated with the format keyword. Perhaps runtime types could support transformations as well? This would be a topic for discussion in a separate issue though.

A wild idea: How about removing prop all together, so tProp is always used? The actual runtime validation can be disabled anyway, so the overhead should be small for people like me who use prop right now.

[evelant]

@sisp Ah, good catch on that use case.

As I've experimented further with transitioning some of my models from MST to mobx-keystone I think that the idea of removing prop in favor of tProp maybe is not a wild one. tProp with runtime validation disabled should be equivalent to prop shouldn't it?

It seems like keeping 2 different APIs is making it difficult to enable some functionality like validation and optionals. Also having two different APIs with overlapping functionality was confusing to me upon first encountering mobx-keystone, especially coming from MST.

[xaviergonz]

Couldn't you do that use case if mergeWith had as parameter the original snapshot?

e.g.

prop<
{
  kind: 'int'; value: number;
}
| {
  kind: 'string'; value: string;
}
>({ default value if undefined}).mergeWith((obj) => {
  if (obj.kind === 'int') return { ...obj,  value: 0}
  return { ...obj, value: '' }
})

Although maybe in this case mergeWith should be actually called 'preprocess' or something similar

[xaviergonz]

A wild idea: How about removing prop all together, so tProp is always used? The actual runtime validation can be disabled anyway, so the overhead should be small for people like me who use prop right now.

I've thought about it (to remove the need of the model type and model id props), but I think at this point if I break the API that much any people using it might be (rightly) annoyed by the decision...

[sisp]

Couldn't you do that use case if mergeWith had as parameter the original snapshot?

e.g.

prop<
{
  kind: 'int'; value: number;
}
| {
  kind: 'string'; value: string;
}
>({ default value if undefined}).mergeWith((obj) => {
  if (obj.kind === 'int') return { ...obj,  value: 0}
  return { ...obj, value: '' }
})

Although maybe in this case mergeWith should be actually called 'preprocess' or something similar

Yes, that would work. I just imagine it would get hard to read with more deeply nested objects or nested "conditionals", whereas I think the declarative syntax reads nicely and scales better.

I've thought about it (to remove the need of the model type and model id props), but I think at this point if I break the API that much any people using it might be (rightly) annoyed by the decision...

I completely understand that. It's such a core part of the API which would break many people's code. But the project is still relatively young, so I think it can be justified with a generous deprecation time. Would you consider a divergence of the capabilities of the two APIs in favor of tProp, i.e. it becomes possible to declare deeply nested defaults declaratively using tProp via something like types.optional while this feature either doesn't exist in the prop API or exists via mergeWith as you suggested? Then, prop would be marked deprecated with, e.g., a 1 year period until removal. What do you think?

[evelant]

I think there might be a lot of benefit to dropping the prop API but I definitely see the concern about such a big change. I agree a deprecation warning would probably work fine.

Since the tProp API can be equivalent might it be possible to make a codeshift to migrate existing code automatically? I imagine even a manual migration is fairly simple since the behavior is equivalent. Are there any cases where it might be non-trivial (beyond simply replace prop with tProp and translating the type definition of each field) to migrate a model?