-
Notifications
You must be signed in to change notification settings - Fork 8.5k
fix schema.nullable() to support non-strings #42891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
68f48c4
5098cfb
33cc703
0bf7c08
0366466
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,19 +19,66 @@ | |
|
|
||
| import { schema } from '..'; | ||
|
|
||
| test('returns value if specified', () => { | ||
| test('returns string value when passed string', () => { | ||
| const type = schema.nullable(schema.string()); | ||
| expect(type.validate('test')).toEqual('test'); | ||
| expect(type.validate('test')).toBe('test'); | ||
| }); | ||
|
|
||
| test('returns null if null', () => { | ||
| test('returns number value when passed number', () => { | ||
| const type = schema.nullable(schema.number()); | ||
| expect(type.validate(42)).toBe(42); | ||
| }); | ||
|
|
||
| test('returns boolean value when passed boolean', () => { | ||
| const type = schema.nullable(schema.boolean()); | ||
| expect(type.validate(true)).toBe(true); | ||
| }); | ||
|
|
||
| test('returns object value when passed object', () => { | ||
| const type = schema.nullable( | ||
| schema.object({ | ||
| foo: schema.number(), | ||
| bar: schema.boolean(), | ||
| baz: schema.string(), | ||
| }) | ||
| ); | ||
| const object = { | ||
| foo: 666, | ||
| bar: true, | ||
| baz: 'foo bar baz', | ||
| }; | ||
|
|
||
| expect(type.validate(object)).toEqual(object); | ||
| }); | ||
|
|
||
| test('returns null if null for string', () => { | ||
| const type = schema.nullable(schema.string()); | ||
| expect(type.validate(null)).toEqual(null); | ||
| expect(type.validate(null)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null if null for number', () => { | ||
| const type = schema.nullable(schema.number()); | ||
| expect(type.validate(null)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null if undefined', () => { | ||
| test('returns null if null for boolean', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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);
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added some additional tests - validating positively for more basic types, and negatively for a property in an object type |
||
| const type = schema.nullable(schema.boolean()); | ||
| expect(type.validate(null)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null if undefined for string', () => { | ||
| const type = schema.nullable(schema.string()); | ||
| expect(type.validate(undefined)).toEqual(null); | ||
| expect(type.validate(undefined)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null if undefined for number', () => { | ||
| const type = schema.nullable(schema.number()); | ||
| expect(type.validate(undefined)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null if undefined for boolean', () => { | ||
| const type = schema.nullable(schema.boolean()); | ||
| expect(type.validate(undefined)).toBe(null); | ||
| }); | ||
|
|
||
| test('returns null even if contained type has a default value', () => { | ||
|
|
@@ -41,7 +88,7 @@ test('returns null even if contained type has a default value', () => { | |
| }) | ||
| ); | ||
|
|
||
| expect(type.validate(undefined)).toEqual(null); | ||
| expect(type.validate(undefined)).toBe(null); | ||
| }); | ||
|
|
||
| test('validates contained type', () => { | ||
|
|
@@ -56,6 +103,27 @@ test('validates basic type', () => { | |
| expect(() => type.validate(666)).toThrowErrorMatchingSnapshot(); | ||
| }); | ||
|
|
||
| test('validates type in object', () => { | ||
| const type = schema.object({ | ||
| foo: schema.nullable(schema.string({ maxLength: 1 })), | ||
| bar: schema.nullable(schema.boolean()), | ||
| }); | ||
|
|
||
| expect(type.validate({ foo: 'a' })).toEqual({ foo: 'a', bar: null }); | ||
| expect(type.validate({ foo: null })).toEqual({ foo: null, bar: null }); | ||
| expect(type.validate({})).toEqual({ foo: null, bar: null }); | ||
| expect(type.validate({ bar: null })).toEqual({ foo: null, bar: null }); | ||
| }); | ||
|
|
||
| test('validates type errors in object', () => { | ||
| const type = schema.object({ | ||
| foo: schema.nullable(schema.string({ maxLength: 1 })), | ||
| bar: schema.nullable(schema.boolean()), | ||
| }); | ||
|
|
||
| expect(() => type.validate({ foo: 'ab' })).toThrowErrorMatchingSnapshot(); | ||
| }); | ||
|
|
||
| test('includes namespace in failure', () => { | ||
| const type = schema.nullable(schema.string({ maxLength: 1 })); | ||
|
|
||
|
|
||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can it just be
defaultValue: null?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.