Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

[fix] When normalize() receives null input, don't say it is an object #411

Merged
merged 3 commits into from
Jan 23, 2020

Conversation

ntucker
Copy link
Contributor

@ntucker ntucker commented Aug 6, 2019

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

  • Add & update tests
  • Ensure CI is passing (lint, tests, flow)
  • Update relevant documentation

@coveralls
Copy link

coveralls commented Aug 6, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling dff77ac on ntucker:normalize-null-input into b5f570a on paularmstrong:master.

@@ -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/);
Copy link
Owner

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')

Copy link
Contributor Author

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}".`
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Owner

@paularmstrong paularmstrong Aug 6, 2019

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'}"

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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();
    });

@ntucker
Copy link
Contributor Author

ntucker commented Aug 6, 2019

Realized it would be weird to output other falsy things like 0, so I explicitly only handle null case in special way now.

@ntucker ntucker merged commit 5992d34 into paularmstrong:master Jan 23, 2020
@ntucker ntucker deleted the normalize-null-input branch January 23, 2020 17:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants