Skip to content

Commit

Permalink
Breaking: Remove "merge" argument from .param method (Closes #227)
Browse files Browse the repository at this point in the history
The third "merge" argument in `.param( key, val, merge )` has been on
the chopping block for two months and is now being removed prior to the
1.0 release.

WPRequest instances are intended to be transient: It is much easier
to understand building a single array of values, and then call
`.param( 'key', valuesArr )` once at the end, than it is to mentally
keep track of calling `.param( 'key', value, true )` multiple times.

It is also one of the only mysterious boolean arguments in the entire
library, and we normally aim to avoid those.

It is also only used in one place (the `.parent` mixin), and is the only
place the `lodash.uniq` NPM dependency is used within node-wpapi.
Removing the argument allows us to simplify code in several places and
permits the removal of that dependency library, slightly decreasing the
size of the built browser UMD bundles.
  • Loading branch information
kadamwhite committed Dec 6, 2016
1 parent 5581ac0 commit 3d0778b
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 48 deletions.
36 changes: 6 additions & 30 deletions lib/constructors/wp-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

var qs = require( 'qs' );
var _union = require( 'lodash.union' );
var _unique = require( 'lodash.uniq' );
var extend = require( 'node.extend' );

Expand Down Expand Up @@ -375,12 +374,9 @@ WPRequest.prototype.validatePath = function() {
* @param {String|Object} props The name of the parameter to set, or an object containing
* parameter keys and their corresponding values
* @param {String|Number|Array} [value] The value of the parameter being set
* @param {Boolean} [merge] Whether to merge the value (true) or replace it (false, default)
* @return {WPRequest} The WPRequest instance (for chaining)
*/
WPRequest.prototype.param = function( props, value, merge ) {
merge = merge || false;

WPRequest.prototype.param = function( props, value ) {
if ( ! props || typeof props === 'string' && value === undefined ) {
// We have no property to set, or no value to set for that property
return this;
Expand All @@ -395,34 +391,14 @@ WPRequest.prototype.param = function( props, value, merge ) {
// Iterate through the properties
Object.keys( props ).forEach(function( key ) {
var value = props[ key ];
var currentVal = this._params[ key ];

// Simple case: setting for the first time, or not merging
if ( ! currentVal || ! merge ) {

// Arrays should be de-duped and sorted
if ( Array.isArray( value ) ) {
value = _unique( value ).sort( alphaNumericSort );
}

// Set the value
this._params[ key ] = value;

// Continue
return;
}

// value and currentVal must both be arrays in order to merge
if ( ! Array.isArray( currentVal ) ) {
currentVal = [ currentVal ];
}

if ( ! Array.isArray( value ) ) {
value = [ value ];
// Arrays should be de-duped and sorted
if ( Array.isArray( value ) ) {
value = _unique( value ).sort( alphaNumericSort );
}

// Concat the new values onto the old (and sort)
this._params[ key ] = _union( currentVal, value ).sort( alphaNumericSort );
// Set the value
this._params[ key ] = value;
}.bind( this ));

return this;
Expand Down
5 changes: 1 addition & 4 deletions lib/mixins/parameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,7 @@ parameterMixins.author = function( author ) {
* @param {Number} parentId The ID of a (hierarchical) taxonomy term
* @return The request instance (for chaining)
*/
parameterMixins.parent = function( parentId ) {
/* jshint validthis:true */
return this.param( 'parent', parentId, true );
};
parameterMixins.parent = paramSetter( 'parent' );

/**
* Specify the post for which to retrieve terms (relevant for *e.g.* taxonomy
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
"dependencies": {
"es6-promise": "^3.2.1",
"li": "^1.0.1",
"lodash.union": "^4.4.0",
"lodash.uniq": "^4.3.0",
"node.extend": "^1.1.5",
"parse-link-header": "^0.4.1",
Expand Down
14 changes: 3 additions & 11 deletions tests/unit/lib/constructors/wp-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,24 +192,16 @@ describe( 'WPRequest', function() {
expect( request._renderQuery() ).to.equal( '?context=edit&type=some_cpt' );
});

it( 'should set parameters by passing a hash object', function() {
it( 'should set multiple parameters by passing a hash object', function() {
request.param({
page: 309,
context: 'view'
});
expect( request._renderQuery() ).to.equal( '?context=view&page=309' );
});

it( 'should merge provided values if merge is set to true', function() {
request.param( 'type', 'post' );
request.param( 'type', 'page', true );
expect( request._params.type ).to.deep.equal( [ 'page', 'post' ] );
});

it( 'should merge, de-dupe & sort array values', function() {
request.param( 'type', [ 'post', 'page', 'post' ] );
expect( request._renderQuery() ).to.equal( '?type%5B%5D=page&type%5B%5D=post' );
request.param( 'type', [ 'page', 'cpt_item' ], true );
it( 'should de-dupe & sort array values', function() {
request.param( 'type', [ 'post', 'page', 'post', 'page', 'cpt_item' ] );
expect( request._renderQuery() ).to.equal( '?type%5B%5D=cpt_item&type%5B%5D=page&type%5B%5D=post' );
});

Expand Down
8 changes: 6 additions & 2 deletions tests/unit/lib/mixins/parameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,13 @@ describe( 'mixins: parameters', function() {
expect( getQueryStr( result ) ).to.equal( 'parent=42' );
});

it( 'merges values on subsequent calls', function() {
// TODO: Is this how the API actually functions?
it( 'replaces values on subsequent calls', function() {
var result = req.parent( 42 ).parent( 2501 );
expect( getQueryStr( result ) ).to.equal( 'parent=2501' );
});

it( 'can pass an array of parent values', function() {
var result = req.parent([ 42, 2501 ]);
expect( getQueryStr( result ) ).to.equal( 'parent[]=2501&parent[]=42' );
});

Expand Down

0 comments on commit 3d0778b

Please sign in to comment.