Additional fields about mobx-state-tree HOT 19 CLOSED

Imho behavior shoud be:

const Item = createFactory({
  id: types.number,
  name: types.string,
})

Item.is({}) // false, missing name and id
Item.is({ id: 3}) // false, missing name
Item.is({ id: 3, name: ""}) // OK
Item.is({ id: 3, name: "", description: ""}) // OK, but description will be 'lost' upon instantation

createFactory({
  id: types.number,
  name: types.string("")
}).is({ id: 3 }) // OK, name is lacking but has a default

from mobx-state-tree.

Good point, I think that should not be the behavior, I was tinkering about that already.

Currently Item.is({ id: 3, name: "test", stuff: false }) will yield false, I think this behavior should change to mimic how e.g. TypeScript does this, if the required set of attributes is present, there should be a match. What do you think @mattiamanzati ?

from mobx-state-tree.

I think this can best and easily be fixed once #65 is done, which features many type system improvements

from mobx-state-tree.

@zetoke Yes indeed, it yields false atm because it checks over the count of keys

from mobx-state-tree.

How I can help with this issue?

from mobx-state-tree.

Yeah, that should be correct @mwestrate! Btw is it a good thing to silently ingore those properties?

from mobx-state-tree.

I started an investigation of a problem for myself. Because I need this issue resolved :D
And after some time I wrote this master...zetoke:feature/object-is
The main problem of skipping fields inside isSnapshotValid:

const Factory = createFactory({ a: number });
Factory.is({ b: string })  // true like a Factory.is({})

What do you think about this?

from mobx-state-tree.

Currently Item.is({ id: 3, name: "test", stuff: false }) will yield false

@mweststrate But Item.is({ id: 4 }) will yield true. Am I right?

from mobx-state-tree.

@zetoke @mattiamanzati, eh no, should return false imho? name is missing in json but not declared maybe or withDefault

from mobx-state-tree.

@mweststrate @mattiamanzati but in the latest release (without my modifications) the "same" logic:

Factory = {
  id: number,
  name: string,
}
Factory.is({ id: 5 }) // true

It's bacause .is() is about a snapshot comparsion/validation, not a validation of instance of type (as I understood).

from mobx-state-tree.

@zetoke Factory.is({ id: 5, name: "hello", additionalKey: false }) // => false, because the number of keys is > than the allowed

from mobx-state-tree.

@zetoke I think Factory.is({ id: 5}) -> false, because Factory.create({ id: 5 }) should fail for lacking a name

from mobx-state-tree.

@mweststrate Yes indeed

from mobx-state-tree.

@mattiamanzati btw, should name: string("") be a shorthand for withDefault(string, "") or literal(""). I guess the first right?

from mobx-state-tree.

@zetoke Factory.is({ id: 5, name: "hello", additionalKey: false }) // => false, because the number of keys is > than the allowed

yep. But if number of keys < keys in snapshot - everything is ok for mobx-state-tree for now. That have been my point.

@zetoke I think Factory.is({ id: 5}) -> false, because Factory.create({ id: 5 }) should fail for lacking a name

Probably. What's about just {} (Factory.is({})) ?

from mobx-state-tree.

@mweststrate string("") is the same as doing "", so it will fallback to https://github.com/mobxjs/mobx-state-tree/blob/master/src/types/object.ts#L62-L66 and being a primitive!

So the only correct one is withDefault(string, "")!

With the new syntax that issue will be solved, because it will be string.create("") vs. withDefault(string, "") and that create will let you understand that you are crating an instance instead of passing a type

from mobx-state-tree.

@zetoke I think Factory.is({ id: 5}) -> false, because Factory.create({ id: 5 }) should fail for lacking a name

@mweststrate The main problem is applySnapshot using .is() as a function for validating snapshot. But this one could be "incremental". Or not?

from mobx-state-tree.

Only correction is the following

createFactory({
  id: types.number,
  name: types.withDefault(types.string, "")
}).is({ id: 3 }) // OK, name is lacking but has a default

from mobx-state-tree.

@mweststrate yep, that make sense. I have the same picture in my head.

from mobx-state-tree.