Additional fields

[zetoke]

Hey, guys!
Thanks a lot for this great lib 👍

Can you clarify one question for me?
I have code:

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

const SomeScheme = createFactory({
  items: types.array(Item),
});

const someScheme = SomeScheme(blabla);

const response = await this.fetch('/api/');
someScheme.items.replace(response.data);

Sheme from API:

[
  {
    "id": 2,
    "name": "Name"
  }
]

Everything is ok.

But then at the API backend guys adding new non-breaking (not for us) field.
For example:

[
  {
    "id": 2,
    "name": "Name",
    "description": "Text"
  }
]

And we getting error and stack trace.

How can I avoid/handle this case?

Any solution for ignoring additional fields for the scheme?

[mweststrate]

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 ?

[zetoke]

How I can help with this issue?

[mweststrate]

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

[mattiamanzati]

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

[zetoke]

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?

[zetoke]

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

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

[mattiamanzati]

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

[mweststrate]

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

[zetoke]

@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).

[mattiamanzati]

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

[mweststrate]

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

[mattiamanzati]

@mweststrate Yes indeed

[mweststrate]

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

[zetoke]

@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({})) ?

[mattiamanzati]

@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

[zetoke]

@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?

[mweststrate]

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

[mattiamanzati]

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

[zetoke]

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