Breaking: Return undefined or null to signal failed validation #10
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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.
| @@ -10,39 +10,39 @@ describe('normalize', function() { | |||
| var type = 'string'; | |||
| var value = 'test string'; | |||
| var result = normalize(type, value); | |||
| expect(result).toEqual(value); | |||
| expect(result).toBe(value); | |||
There was a problem hiding this comment.
I had been wondering about this as well, we definitely want to do strict comparisons here.
There was a problem hiding this comment.
I prefer equals always but wasn't going to nitpick (this is from your changeset).
There was a problem hiding this comment.
Ah, sorry, yes, that's what got me wondering about this -- sorry, I made this a while back :-)
I respectfully disagree about equals, preferring strict comparison where applicable -- I guess that touches at the core of our differing ideas here, in a way.
In any event, for the change I was going for it was quite essential (testing that the rejection signal was properly changed from null to defined). But if the contract will be that null and undefined should be treated interchangably by the users of this lib, equals would be fine of course.
|
@erikkemperman what remains to be discussed here? |
|
Not much I guess! Sorry for being so verbose, I felt I hadn't been explaining myself properly and might have overcompensated slightly :-) |
|
No worries, I like that we can discuss the changes in-depth. I'll get this merged and released as 3.0 |
@erikkemperman I've made the
nullorundefinedchange I was talking about in #8 but I'm just now noticing that we don't actually do any null/undefined validation upon getting the result of a coercer. Was this intentional?