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.

Sign up for GitHub

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

feat(deps): Remove check-types in favor of lodash #104

Merged
merged 1 commit into from
Jul 19, 2019
Merged

feat(deps): Remove check-types in favor of lodash #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Variable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
*always* yield true; regardless of the underlying value; because js.
**/

var check = require('check-types');
const _ = require('lodash');

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' );
Copy link
Member

@missinglink missinglink Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to check for the nil number value NaN here also?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

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

}
this.$ = val;
Expand Down
21 changes: 12 additions & 9 deletions lib/VariableStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@
note: you may inject your own variables when calling the constructor
**/

var check = require('check-types'),
Variable = require('./Variable');
const _ = require('lodash');

const Variable = require('./Variable');

function VariableStore( vars ){
this._vars = {};
if( check.assigned( vars ) ){
if( !_.isEmpty( vars ) ){
this.set( vars );
}
}
Expand All @@ -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) ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

_.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.

throw new Error( 'invalid query variable, key must be valid string' );
}
// init variable
if( !this._vars.hasOwnProperty( key ) ){
this._vars[ key ] = new Variable();
}
// setter
if( check.assigned( val ) ){

// set value if val parameter passed
if (val !== undefined) {
this._vars[ key ].set( val );
}

// getter
return this._vars[ key ];
};
Expand All @@ -50,7 +53,7 @@ VariableStore.prototype.var = function( key, val ){
check if a key has been set (has a value)
**/
VariableStore.prototype.isset = function( key ){
if( !check.nonEmptyString( key ) ){
if( _.isEmpty( key ) || !_.isString( key) ) {
throw new Error( 'invalid key, must be valid string' );
}
// key not set
Expand All @@ -65,7 +68,7 @@ VariableStore.prototype.isset = function( key ){
delete a variable by key
**/
VariableStore.prototype.unset = function( key ){
if( !check.nonEmptyString( key ) ){
if( _.isEmpty( key ) || !_.isString( key) ) {
throw new Error( 'invalid key, must be valid string' );
}
// key not set
Expand All @@ -81,7 +84,7 @@ VariableStore.prototype.unset = function( key ){
import variables from a plain-old-javascript-object
**/
VariableStore.prototype.set = function( pojo ){
if( !check.object( pojo ) ){
if( !_.isPlainObject( pojo ) ){
throw new Error( 'invalid object' );
}
for( var attr in pojo ){
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
},
"homepage": "https://github.com/pelias/query#readme",
"dependencies": {
"check-types": "^8.0.0",
"lodash": "^4.17.14"
},
"devDependencies": {
Expand Down