mobxjs / serializr

Serialize and deserialize complex object graphs to and from JSON and Javascript classes
MIT License
766 stars 52 forks source link

update documentation for usage of optional with custom #131

Closed akphi closed 4 years ago

akphi commented 4 years ago

See discussion in #128

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 89.961% when pulling 05e03fe3fd0babf0ef3386a89c933d053053dcef on blacksteed232:master into cd4c35ed0afe2d1b931379d1bef71334b8e76e91 on mobxjs:master.

NaridaL commented 4 years ago

Does optional(custom()) make sense at all?

In any case, I think your example calls value => value.name with value undefined, which results in an error.

Maybe it makes more sense to write that if you're using custom, you don't need optional.

custom(v => v.?name ?? SKIP, v => {})

akphi commented 4 years ago

Does optional(custom()) make sense at all?

In any case, I think your example calls value => value.name with value undefined, which results in an error.

Maybe it makes more sense to write that if you're using custom, you don't need optional.

custom(v => v.?name ?? SKIP, v => {})

@NaridaL yes! that's definitely my use case, and so I thought I could use optional to be more declarative. For example:

const todoSchema = createSimpleSchema({
  name: optional(custom(..)) // more declarative than 'custom( ... return SKIP ...)
})

Then I tried optional(custom()) and found out did not work quite the way I expected.

@pyrogenic thanks!

NaridaL commented 4 years ago

image

image

akphi commented 4 years ago

@NaridaL yes, I get that. I was not careful about giving the example in the doc but that's exactly the problem I faced when I used optional(custom()). I thought optional will happen first. I can correct the example to:

user: optional(custom(value => value?.name, () => SKIP))

akphi commented 4 years ago

@NaridaL I have corrected the example in #132