-
Notifications
You must be signed in to change notification settings - Fork 866
[fix] When normalize() receives null input, don't say it is an object #411
Conversation
@@ -12,6 12,11 @@ describe('normalize', () => { | |||
expect(() => normalize({})).toThrow(); | |||
}); | |||
|
|||
test('cannot normalize with null input', () => { | |||
const mySchema = new schema.Entity('tacos'); | |||
expect(() => normalize(null, mySchema)).toThrow(/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.
nit: toThrow('Unexpected input given to normalize')
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.
Just making sure it has null in the error string. Not trying to worry about the rest of the wording since there isn't an conditional in there.
src/index.js
Outdated
@@ -43,7 43,9 @@ export const schema = { | |||
|
|||
export const normalize = (input, schema) => { | |||
if (!input || typeof input !== 'object') { | |||
throw new Error(`Unexpected input given to normalize. Expected type to be "object", found "${typeof input}".`); | |||
throw new Error( | |||
`Unexpected input given to normalize. Expected type to be "object", found "${input && typeof input}".` |
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.
Maybe derive the type from the schema? array
if Array.isArray(schema) || schema instanceof schema.Array
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 don't believe an array would trigger this. typeof myarray === 'object' and all arrays are truthy.
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.
null, undefined, numbers, strings, symbols, functions (and thus static classes as well) are the only types that would trigger this. maybe some other primitive type I'm not aware of.
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 mean derive the type for the expectation to say: Expected type to be "${Array.isArray(schema) || schema instanceof schema.Array ? 'array' : 'object'}"
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.
ahh, gotcha. yes this is a good additional improvement
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.
but then shouldn't it fail if you get an array when expecting a plain object? currently this check will pass
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.
Should I add a respective test for array vs plain object in the if statement?
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.
Seems like array and objects are interchangable due to tests like:
test('normalizes Objects using their values', () => {
const userSchema = new schema.Entity('user');
const users = new schema.Array(userSchema);
expect(normalize({ foo: { id: 1 }, bar: { id: 2 } }, users)).toMatchSnapshot();
});
Realized it would be weird to output other falsy things like |
Problem
In Javascript typeof null === 'object'.
Solution
Since the check in normalize includes one for !input, it should say 'found null' when null is passed rather than a confusing message that it found object when it needed object.
TODO