Skip to content
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

Breaking: Return undefined or null to signal failed validation #10

Merged
merged 1 commit into from
Jul 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 7 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ var normalize = require('value-or-function');
var isEnabled = normalize('boolean', true);
// isEnabled === true

// Values not matching type return null
// Values not matching type return undefined
var isEnabled = normalize('boolean', 1);
// isEnabled === null
// isEnabled === undefined

// Functions are called
var isEnabled = normalize('boolean', function() {
Expand Down Expand Up @@ -49,7 +49,9 @@ var isEnabled = normalize(['string', 'boolean'], true);
// Provide a function as first argument to do custom coercion
var now = new Date();
var enabledSince = normalize(function(value) {
return value.constructor === Date ? value : null;
if (value.constructor === Date) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return value.constructor === Date ? value : undefined; ? And come to think of it, wouldn't instanceof Date be better in case someone extends that class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like ternaries and undefined is the return value if we don't return anything. The only reason the docs had the ternary was due to requiring a null return.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, that's not something I've heard about ternaries before? Anyway, returning undefined explicitly is slightly odd. In documentation I think it might help to point out what's expected though, but nitpicking (again).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikkemperman sorry, I didn't mean it about ternaries; I meant that the result of a function call is undefined if it doesn't return anything explicitly. I just don't like the look/mental parse-ability of ternaries (especially ones that return undefined as the "else" case.

return value;
}
}, now);
// enabledSince === now

Expand All @@ -60,7 +62,6 @@ var result = normalize.string('');
var result = normalize.symbol(Symbol());
var result = normalize.boolean(true);
var result = normalize.function(function() {});
var result = normalize.undefined(undefined);
var result = normalize.date(new Date());
```

Expand All @@ -71,7 +72,7 @@ var result = normalize.date(new Date());
Takes a coercer function `coercer` to transform `value` to the desired type.
Also optionally takes any extra arguments to apply to `value` if `value` is a function.

If the return value of `coercer(value)` is not `null`, that value is returned.
If the return value of `coercer(value)` is not `null` or `undefined`, that value is returned.
Otherwise, if `value` is a function, that function is called with any extra arguments
supplied to `normalize`, and its return value is passed through the coercer.

Expand All @@ -80,7 +81,7 @@ and the appropriate default coercer is invoked, optionally first reducing `value
to a primitive type with `.valueOf()` if it is an Object.

If `coercer` is an array, each element is tried until one returns something other
than `null`, or it results in `null` if all of the elements yield `null`.
than `null` or `undefined`, or it results in `undefined` if all of the elements yield `null` or `undefined`.

#### `normalize.object(value[, ...appliedArguments])`

Expand All @@ -106,10 +107,6 @@ Convenience method for `normalize('boolean', ...)`.

Convenience method for `normalize('function', ...)`.

#### `normalize.undefined(value[, ...appliedArguments])`

Convenience method for `normalize('undefined', ...)`.

#### `normalize.date(value[, ...appliedArguments])`

Convenience method for `normalize('date', ...)`.
Expand Down
7 changes: 2 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ var types = [
'boolean',
'date',
'function', // Weird to expose this
'undefined', // And this?
];


Expand Down Expand Up @@ -40,10 +39,10 @@ function coerce(ctx, coercer, value) {
}

// Array of coercers, try in order until one returns a non-null value
var result = null;
var result;
coercer.some(function(coercer) {
result = coerce(ctx, coercer, value);
return result !== null;
return result != null;
});

return result;
Expand Down Expand Up @@ -76,15 +75,13 @@ coerce.date = function(value) {
if (typeof value === 'number' && !isNaN(value) && isFinite(value)) {
return new Date(value);
}
return null;
};


function typeOf(type, value) {
if (typeof value === type) {
return value;
}
return null;
}


Expand Down