Skip to content

Commit

Permalink
Clean up debug code
Browse files Browse the repository at this point in the history 
Make debug a prototype method.  This has a couple of advantages:

- It's no longer necessary for the debug code to be at a specific place
  in request.js, see request#1354
- It's easier for helper methods in other files to generate debug
  messages, see request#1360
  • Loading branch information
@nylen
nylen committed Jan 29, 2015
1 parent 2c87bbf commit ffd4851
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 27 deletions.
5 changes: 3 additions & 2 deletions lib/auth.js
View file
Expand Up @@ -8,8 +8,9 @@ var md5 = helpers.md5
, toBase64 = helpers.toBase64


function Auth () {
function Auth (request) {

This comment has been minimized.

Sign in to view

Copy link
@simov

simov Jan 29, 2015

I think it's better to have this logic inside each module that needs it.

var debug = process.env.NODE_DEBUG && /\brequest\b/.test(process.env.NODE_DEBUG)

function debug () {
  if (debug) {
    console.error('REQUEST %s', util.format.apply(util, arguments))
  }
}

There is really not that much related to Request, nor to the Request's instance. Request.debug is a static var, so Request.prototype.debug is not per instance either.

So if that was the only reason to pass the entire request instance here, I would suggest not to do that.

This comment has been minimized.

Sign in to view

Copy link
@nylen

nylen Jan 29, 2015

Author Owner

I don't think we should repeat the logic, but if someone enables/disables debugging for request then that choice should carry through to all helper libraries that want to send debug info.

This change isn't urgent, I'm fine with closing this PR if we can't agree on something. But, I'm going to quote you from request#1360 (comment):

About debug that would do the job 👍

:trollface: 💬

This comment has been minimized.

Sign in to view

Copy link
@simov

simov Jan 29, 2015

Yeah, you should tell me how you find data around github so quick. Ok, I completely forgot about the fact that we modify Request.debug in index.js Actually that's part of the reason why I don't want to modify instances in places where you don't expect, and once you pass it around it's just a matter of time before someone start doing it.

This comment has been minimized.

Sign in to view

Copy link
@nylen

nylen Jan 30, 2015

Author Owner

My secret is this: I check my email obsessively unless I'm working on something else :)
// define all public properties here
this.request = request
this.hasAuth = false
this.sentAuth = false
this.bearerToken = null
Expand Down Expand Up @@ -116,7 +117,7 @@ Auth.prototype.response = function (method, path, headers) {

var authHeader = c.get('www-authenticate')
var authVerb = authHeader && authHeader.split(' ')[0].toLowerCase()
// debug('reauth', authVerb)
self.request.debug('reauth', authVerb)

switch (authVerb) {
case 'basic':
Expand Down
54 changes: 29 additions & 25 deletions request.js
View file
Expand Up @@ -261,14 +261,6 @@ function Request (options) {

util.inherits(Request, stream.Stream)

// Debugging
Request.debug = process.env.NODE_DEBUG && /\brequest\b/.test(process.env.NODE_DEBUG)
function debug() {
if (Request.debug) {
console.error('REQUEST %s', util.format.apply(util, arguments))
}
}

Request.prototype.setupTunnel = function () {
// Set up the tunneling agent if necessary
// Only send the proxy whitelisted header names.
Expand Down Expand Up @@ -338,7 +330,7 @@ Request.prototype.init = function (options) {
self.qsLib = (options.useQuerystring ? querystring : qs)
}

debug(options)
self.debug(options)
if (!self.pool && self.pool !== false) {
self.pool = globalPool
}
Expand Down Expand Up @@ -507,7 +499,7 @@ Request.prototype.init = function (options) {
}

// Auth must happen last in case signing is dependent on other headers
self._auth = new Auth()
self._auth = new Auth(self)

if (options.oauth) {
self.oauth(options.oauth)
Expand Down Expand Up @@ -894,7 +886,7 @@ Request.prototype.start = function () {
var reqOptions = copy(self)
delete reqOptions.auth

debug('make request', self.uri.href)
self.debug('make request', self.uri.href)
self.req = self.httpModule.request(reqOptions)

if (self.timeout && !self.timeoutTimer) {
Expand Down Expand Up @@ -957,9 +949,9 @@ Request.prototype.onRequestError = function (error) {

Request.prototype.onRequestResponse = function (response) {
var self = this
debug('onRequestResponse', self.uri.href, response.statusCode, response.headers)
self.debug('onRequestResponse', self.uri.href, response.statusCode, response.headers)
response.on('end', function() {
debug('response end', self.uri.href, response.statusCode, response.headers)
self.debug('response end', self.uri.href, response.statusCode, response.headers)
})

// The check on response.connection is a workaround for browserify.
Expand All @@ -968,7 +960,7 @@ Request.prototype.onRequestResponse = function (response) {
response.connection.once('error', connectionErrorHandler)
}
if (self._aborted) {
debug('aborted', self.uri.href)
self.debug('aborted', self.uri.href)
response.resume()
return
}
Expand All @@ -987,7 +979,7 @@ Request.prototype.onRequestResponse = function (response) {
if (self.httpModule === https &&
self.strictSSL && (!response.hasOwnProperty('client') ||
!response.client.authorized)) {
debug('strict ssl error', self.uri.href)
self.debug('strict ssl error', self.uri.href)
var sslErr = response.hasOwnProperty('client') ? response.client.authorizationError : self.uri.href + ' does not support SSL'
self.emit('error', new Error('SSL Error: ' + sslErr))
return
Expand Down Expand Up @@ -1033,7 +1025,7 @@ Request.prototype.onRequestResponse = function (response) {
var redirectTo = null
if (response.statusCode >= 300 && response.statusCode < 400 && response.caseless.has('location')) {
var location = response.caseless.get('location')
debug('redirect', location)
self.debug('redirect', location)

if (self.followAllRedirects) {
redirectTo = location
Expand All @@ -1059,7 +1051,7 @@ Request.prototype.onRequestResponse = function (response) {
}

if (redirectTo && self.allowRedirect.call(self, response)) {
debug('redirect to', redirectTo)
self.debug('redirect to', redirectTo)

// ignore any potential response body. it cannot possibly be useful
// to us at this point.
Expand Down Expand Up @@ -1146,7 +1138,7 @@ Request.prototype.onRequestResponse = function (response) {
// Since previous versions didn't check for Content-Encoding header,
// ignore any invalid values to preserve backwards-compatibility
if (contentEncoding !== 'identity') {
debug('ignoring unrecognized Content-Encoding ' + contentEncoding)
self.debug('ignoring unrecognized Content-Encoding ' + contentEncoding)
}
dataStream = response
}
Expand Down Expand Up @@ -1197,14 +1189,14 @@ Request.prototype.onRequestResponse = function (response) {
}
})
self.on('end', function () {
debug('end event', self.uri.href)
self.debug('end event', self.uri.href)
if (self._aborted) {
debug('aborted', self.uri.href)
self.debug('aborted', self.uri.href)
return
}

if (buffer.length) {
debug('has body', self.uri.href, buffer.length)
self.debug('has body', self.uri.href, buffer.length)
if (self.encoding === null) {
// response.body = buffer
// can't move to this until https://github.com/rvagg/bl/issues/13
Expand All @@ -1226,7 +1218,7 @@ Request.prototype.onRequestResponse = function (response) {
response.body = JSON.parse(response.body, self._jsonReviver)
} catch (e) {}
}
debug('emitting complete', self.uri.href)
self.debug('emitting complete', self.uri.href)
if(typeof response.body === 'undefined' && !self._json) {
response.body = self.encoding === null ? new Buffer(0) : ''
}
Expand All @@ -1237,14 +1229,14 @@ Request.prototype.onRequestResponse = function (response) {
else{
self.on('end', function () {
if (self._aborted) {
debug('aborted', self.uri.href)
self.debug('aborted', self.uri.href)
return
}
self.emit('complete', response)
})
}
}
debug('finish init function', self.uri.href)
self.debug('finish init function', self.uri.href)
}

Request.prototype.abort = function () {
Expand Down Expand Up @@ -1523,7 +1515,7 @@ Request.prototype.httpSignature = function (opts) {
method: self.method,
path: self.path
}, opts)
debug('httpSignature authorization', self.getHeader('authorization'))
self.debug('httpSignature authorization', self.getHeader('authorization'))

return self
}
Expand Down Expand Up @@ -1655,6 +1647,18 @@ Request.prototype.destroy = function () {
}
}


// Debugging

Request.debug = process.env.NODE_DEBUG && /\brequest\b/.test(process.env.NODE_DEBUG)

Request.prototype.debug = function() {
if (Request.debug) {
console.error('REQUEST %s', util.format.apply(util, arguments))
}
}


Request.defaultProxyHeaderWhiteList =
defaultProxyHeaderWhiteList.slice()

Expand Down

0 comments on commit ffd4851

Please sign in to comment.