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
Refactor defaults #1520
Refactor defaults #1520
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,22 +19,30 @@ var extend = require('util')._extend | |
, helpers = require('./lib/helpers') | ||
|
||
var isFunction = helpers.isFunction | ||
, constructObject = helpers.constructObject | ||
, filterForCallback = helpers.filterForCallback | ||
, constructOptionsFrom = helpers.constructOptionsFrom | ||
, paramsHaveRequestBody = helpers.paramsHaveRequestBody | ||
|
||
|
||
// organize params for patch, post, put, head, del | ||
function initParams(uri, options, callback) { | ||
callback = filterForCallback([options, callback]) | ||
options = constructOptionsFrom(uri, options) | ||
|
||
return constructObject() | ||
.extend({callback: callback}) | ||
.extend({options: options}) | ||
.extend({uri: options.uri}) | ||
.done() | ||
if (typeof options === 'function') { | ||
callback = options | ||
} | ||
|
||
var params | ||
if (typeof options === 'object') { | ||
params = extend({}, options) | ||
params = extend(params, {uri: uri}) | ||
} else if (typeof uri === 'string') { | ||
params = extend({}, {uri: uri}) | ||
} else { | ||
params = extend({}, uri) | ||
} | ||
|
||
return { | ||
uri: params.uri, | ||
options: params, | ||
callback: callback | ||
} | ||
} | ||
|
||
function request (uri, options, callback) { | ||
|
@@ -56,9 +64,9 @@ function request (uri, options, callback) { | |
|
||
var verbs = ['get', 'head', 'post', 'put', 'patch', 'del'] | ||
|
||
verbs.forEach(function(verb){ | ||
verbs.forEach(function(verb) { | ||
var method = verb === 'del' ? 'DELETE' : verb.toUpperCase() | ||
request[verb] = function(uri, options, callback){ | ||
request[verb] = function (uri, options, callback) { | ||
var params = initParams(uri, options, callback) | ||
params.options.method = method | ||
return (this || request)(params.uri || null, params.options, params.callback) | ||
|
@@ -73,77 +81,65 @@ request.cookie = function (str) { | |
return cookies.parse(str) | ||
} | ||
|
||
request.defaults = function (options, requester) { | ||
function wrap (method, options, requester) { | ||
|
||
if (typeof options === 'function') { | ||
requester = options | ||
options = {} | ||
} | ||
return function (uri, opts, callback) { | ||
var params = initParams(uri, opts, callback) | ||
|
||
var self = this | ||
var wrap = function (method) { | ||
var headerlessOptions = function (options) { | ||
options = extend({}, options) | ||
delete options.headers | ||
return options | ||
var headerlessOptions = extend({}, options) | ||
delete headerlessOptions.headers | ||
params.options = extend(headerlessOptions, params.options) | ||
|
||
if (typeof method === 'string') { | ||
params.options.method = (method === 'del' ? 'DELETE' : method.toUpperCase()) | ||
method = request[method] | ||
} | ||
|
||
var getHeaders = function (params, options) { | ||
return constructObject() | ||
.extend(options.headers) | ||
.extend(params.options.headers) | ||
.done() | ||
if (options.headers) { | ||
var headers = extend({}, options.headers) | ||
params.options.headers = extend(headers, params.options.headers) | ||
} | ||
|
||
return function (uri, opts, callback) { | ||
var params = initParams(uri, opts, callback) | ||
params.options = extend(headerlessOptions(options), params.options) | ||
if (isFunction(requester)) { | ||
method = requester | ||
} | ||
|
||
if (typeof method === 'string') { | ||
params.options.method = (method === 'del' ? 'DELETE' : method.toUpperCase()) | ||
method = request[method] | ||
} | ||
return method(params.options, params.callback) | ||
} | ||
} | ||
|
||
if (params.options.method === 'HEAD' && paramsHaveRequestBody(params)) { | ||
throw new Error('HTTP HEAD requests MUST NOT include a request body.') | ||
} | ||
request.defaults = function (options, requester) { | ||
var self = this | ||
|
||
if (options.headers) { | ||
params.options.headers = getHeaders(params, options) | ||
} | ||
if (typeof options === 'function') { | ||
requester = options | ||
options = {} | ||
} | ||
|
||
if (isFunction(requester)) { | ||
method = requester | ||
} | ||
var defaults = wrap(self, options, requester) | ||
|
||
return method(params.options, params.callback) | ||
} | ||
} | ||
var verbs = ['get', 'head', 'post', 'put', 'patch', 'del'] | ||
verbs.forEach(function(verb) { | ||
defaults[verb] = wrap(verb, options, requester) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we change how we call this function to simplify wrap's behavior? The complexity of supporting both functions and strings for the first argument would be unnecessary if you could always pass the function you want to wrap. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep I agree, crossed my mind, but I decided to keep it that way, though I haven't tested how the code will look like with it. So I'll give it a try. |
||
}) | ||
|
||
var defaults = wrap(self) | ||
defaults.get = wrap('get') | ||
defaults.patch = wrap('patch') | ||
defaults.post = wrap('post') | ||
defaults.put = wrap('put') | ||
defaults.head = wrap('head') | ||
defaults.del = wrap('del') | ||
defaults.cookie = wrap(self.cookie) | ||
defaults.cookie = wrap(self.cookie, options, requester) | ||
defaults.jar = self.jar | ||
defaults.defaults = self.defaults | ||
return defaults | ||
} | ||
|
||
request.forever = function (agentOptions, optionsArg) { | ||
var options = constructObject() | ||
var options = {} | ||
if (optionsArg) { | ||
options.extend(optionsArg) | ||
options = extend({}, optionsArg) | ||
} | ||
if (agentOptions) { | ||
options.agentOptions = agentOptions | ||
} | ||
|
||
options.extend({forever: true}) | ||
return request.defaults(options.done()) | ||
options = extend(options, {forever: true}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this just be written as |
||
return request.defaults(options) | ||
} | ||
|
||
// Exports | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
'use strict' | ||
|
||
var extend = require('util')._extend | ||
, jsonSafeStringify = require('json-stringify-safe') | ||
var jsonSafeStringify = require('json-stringify-safe') | ||
, crypto = require('crypto') | ||
|
||
function deferMethod() { | ||
|
@@ -12,40 +11,10 @@ function deferMethod() { | |
return setImmediate | ||
} | ||
|
||
function constructObject(initialObject) { | ||
initialObject = initialObject || {} | ||
|
||
return { | ||
extend: function (object) { | ||
return constructObject(extend(initialObject, object)) | ||
}, | ||
done: function () { | ||
return initialObject | ||
} | ||
} | ||
} | ||
|
||
function constructOptionsFrom(uri, options) { | ||
var params = constructObject() | ||
if (typeof options === 'object') { | ||
params.extend(options).extend({uri: uri}) | ||
} else if (typeof uri === 'string') { | ||
params.extend({uri: uri}) | ||
} else { | ||
params.extend(uri) | ||
} | ||
return params.done() | ||
} | ||
|
||
function isFunction(value) { | ||
return typeof value === 'function' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! +1 on removing this custom implementation and using normal javascript objects |
||
} | ||
|
||
function filterForCallback(values) { | ||
var callbacks = values.filter(isFunction) | ||
return callbacks[0] | ||
} | ||
|
||
function paramsHaveRequestBody(params) { | ||
return ( | ||
params.options.body || | ||
|
@@ -78,9 +47,6 @@ function toBase64 (str) { | |
} | ||
|
||
exports.isFunction = isFunction | ||
exports.constructObject = constructObject | ||
exports.constructOptionsFrom = constructOptionsFrom | ||
exports.filterForCallback = filterForCallback | ||
exports.paramsHaveRequestBody = paramsHaveRequestBody | ||
exports.safeStringify = safeStringify | ||
exports.md5 = md5 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap
is a little vague, it took me a while to understand what this function was doing. Is there a better name here that could provide a little more context?wrapDefaults
?wrapRequestMethods
?Also, some documentation on expected arguments would be equally helpful, especially since it looks like we support a lot of different ways to call this (method = string, function, or request, request is optional, etc).