fix schema.nullable() to support non-strings #42891
Conversation
The current (new!) implementation of `schema.nullable()` ends up having a problem - it only works right for `schema.nullable(schema.string())`. For other types, like `schema.number()` and `schema.boolean()`, a `null` value passed in does not validate. I poked around a bit, but "it's complicated". So, went with the suggested approach of using a `schema.maybe([V, null])` validation, which works for number and boolean. Added some additional tests on the `nullable` type, could probably add more, if we think we need some.
💚 Build Succeeded |
pmuellr
added
the
Team:Core
|
Pinging @elastic/kibana-platform |
pmuellr
added
release_note:skip
| expect(type.validate(null)).toEqual(null); | ||
| }); | ||
|
|
||
| test('returns null if null for boolean', () => { |
There was a problem hiding this comment.
could you add a couple of tests validating it returns value for other types different from null?
const type = schema.nullable(schema.boolean());
expect(type.validate(true)).toBe(true);There was a problem hiding this comment.
added some additional tests - validating positively for more basic types, and negatively for a property in an object type
💔 Build Failed |
|
retest |
💚 Build Succeeded |
|
Thanks for the comments @restrry ; @azasypkin, any other thoughts? I'd like to get this merged by end of the week, next week, no big hurry ... |
|
|
||
| function nullable<V>(type: Type<V>): Type<V | null> { | ||
| return new NullableType(type); | ||
| return schema.oneOf([type, schema.literal(null)], { defaultValue: () => null }); |
There was a problem hiding this comment.
nit: can it just be defaultValue: null?
There was a problem hiding this comment.
I think I tried that first, it didn't work, and you suggested the function invocation version, which did work. Let me try again ...
There was a problem hiding this comment.
Huh, only reference I can find is you suggesting this change a long time ago to me. Weird thing is, no idea where the function version came from, I don't think I would have figured that one out myself; I have a feeling maybe it was needed in a previous version of the combinator.
So, commit incoming with that change ...
There was a problem hiding this comment.
Heh, yeah, I believe that was me who initially suggested that function syntax 🙈 Just didn't think long enough to suggest the simplest solution, sorry for the confusion.
|
Double-checked that the most recent version of |
💚 Build Succeeded |
The previous implementation of `schema.nullable()` ends up having a problem - it only works right for `schema.nullable(schema.string())`. For other types, like `schema.number()` and `schema.boolean()`, a `null` value passed in does not validate. I poked around a bit, but "it's complicated". So, went with the suggested approach of using a `schema.maybe([V, null])` validation, which works for number and boolean. Added some additional tests on the `nullable` type.
The previous implementation of `schema.nullable()` ends up having a problem - it only works right for `schema.nullable(schema.string())`. For other types, like `schema.number()` and `schema.boolean()`, a `null` value passed in does not validate. I poked around a bit, but "it's complicated". So, went with the suggested approach of using a `schema.maybe([V, null])` validation, which works for number and boolean. Added some additional tests on the `nullable` type.
4 participants
The current (new!) implementation of
schema.nullable()ends up havinga problem - it only works right for
schema.nullable(schema.string()).For other types, like
schema.number()andschema.boolean(), anullvalue passed in does not validate.
I poked around a bit, but "it's complicated". So, went with the
suggested approach of using a
schema.maybe([V, null])validation,which works for number and boolean.
Added some additional tests on the
nullabletype, could probablyadd more, if we think we need some.