feat(deps): Remove check-types in favor of lodash #104
Conversation
Since the creation of this module we have largely settled on lodash as our Javascript helper library of choice. This module was now using both, which seemed excessive and inconsistent. This change replaces all use of `check-types` with equivalent lodash functionality. One notable conceptual change is regarding what is valid to store in a `Variable` object used as placeholder values in queries. Initially, only scalar values were supported: non-empty strings, numbers, and booleans. However, over time we found it useful to store first arrays and then even objects as variables, meaning its now clearer to enumerate what a `Variable` _doesn't_ support storing: empty strings, `null`, and `undefined`
orangejulius
force-pushed
the
remove-check-types
branch
from
8f75861 to
0910125
Compare
|
It's hard to get my head around this as the two libraries have quite different method names. One thing I noticed is the use of We need to be a little careful, the old syntax was actually much clearer in some cases IMHO and I'm a bit worried this refactor will introduce new bugs. |
|
|
||
| function Variable(){ | ||
| this.$ = ''; | ||
| } | ||
|
|
||
| Variable.prototype.set = function( val ){ | ||
| if( !check.nonEmptyString(val) && !check.number(val) && !check.boolean(val) && !check.array(val) && !check.object(val)){ | ||
| if( val === '' || val === null || val === undefined ){ | ||
| throw new Error( 'invalid value, value must be valid js Variable' ); |
There was a problem hiding this comment.
Do we need to check for the nil number value NaN here also?
There was a problem hiding this comment.
Good question. I was trying to preserve the existing behavior as much and as simply as possible, but NaN is definitely in the same boat as null and undefined as generally not useful.
On the other hand, and especially considering #13, which could be fixed by removing the restriction on storing an empty string, its kinda worth considering if we should filter out any values at all at this point. null and undefined at least would both cleanly serialize (to nothing) in JSON, so why not allow that if we allow so much else?
There was a problem hiding this comment.
I'm going to merge this as is but I'm definitely up for re-visiting allowed/disallowed values. I think we should allow '' to fix #13, figure out if we can stop allowing objects (which just sounds like a painful way to introduce weird behavior), and revisit null/undefined/NaN etc.
| @@ -31,17 +32,19 @@ function VariableStore( vars ){ | |||
| vs.var('foo').set('bar'); | |||
| **/ | |||
| VariableStore.prototype.var = function( key, val ){ | |||
| if( !check.nonEmptyString( key ) ){ | |||
| if( _.isEmpty( key ) || !_.isString( key) ) { | |||
There was a problem hiding this comment.
I don't believe this is correct, my understanding of _.isEmpty is that it's for collections, not strings.
You might have to write it isString(x) && !!x.length
There was a problem hiding this comment.
_.isEmpty ends up checking .length, so it does work: https://github.com/lodash/lodash/blob/4.17.14/lodash.js#L11483-L11487
I agree that reading through the docs, it sounds like it shouldn't work, unless the definition of a "collection" is anything that has a length property.
|
Acceptance tests all pass so I think we are safe :) |
Since the creation of this module we have largely settled on lodash as our Javascript helper library of choice. This module was now using both lodash and
check-types, which seemed excessive and inconsistent.This change replaces all use of
check-typeswith equivalent lodash functionality.One notable conceptual change is regarding what is valid to store in a
Variableobject used as placeholder values in queries.Initially, only scalar values were supported: non-empty strings, numbers, and booleans.
However, over time we found it useful to store first arrays and then even objects as variables, meaning its now clearer to enumerate what a
Variabledoesn't support storing: empty strings,null, andundefined