Remove circular dependency from debugging code

[nylen]

This circular dependency is giving me problems with browserify, see
browserify/browserify#1068.

Move the debug property from request to request.Request, and
define a property at the old location to pass through to the new
location for backwards compatibility.

[nylen]

Review @simov @seanstrom @FredKSchott ?

[seanstrom]

Looks good, though are we forced to have the debug module inlined into request.js ?

[nylen]

I was having trouble thinking of a clean way to pass the needed flag. I guess something like this would work though:

lib/debug.js

module.exports = function(Request) {
  Request.debug = // do process.env check
  return function debug(msg) {
    if (Request.debug) {
      // do stuff
    }
  }
}

and in request.js

var debug = require('./lib/debug')(Request)

Do you think this is better than just having it inlined?

[seanstrom]

Ehm, It's probably better to have it inline. Now that I see the alternative.

[FredKSchott]

nit: I'd expect to see the debug function up with the other inlined functions, instead of here in the Request prototype methods

[FredKSchott]

+1 lgtm (with 1 small comment)

[nylen]

Yeah that's one of the reasons I wanted some review on this. It's here because the linter complains about use of Request before it's defined otherwise, and moving it to the bottom makes it complain about the same thing with debug.

[nylen]

@FredKSchott I guess I could put var debug at the top, then debug = function()... at the bottom, would that be better?

[FredKSchott]

Ah yea, I see that now. In that case I'm fine with what you have written, it makes sense given the layout. Although it also means our constructor could never call debug, which is a shame.

Not sure if defining var debug at the top buys us much, that would confuse me even more :)

The only other solution I can think of would be to define some var isDebugEnabled to hold the flag instead, and then expose some Request.getDebug/setDebug methods for index.js to hook into that check that flag. But... that's not too much cleaner.

[nylen]

Agreed, and I'd rather avoid adding more machinery here. Somebody besides me want to merge this?

Minor issues in the scheme of things, but I've been reminded of this article a lot lately: http://stilldrinking.org/programming-sucks

[FredKSchott]

Haha yea, but I'm glad these are our problems now, vs "what the hell is this code doing please halp"

Also fwiw, you can always enable/disable eslint rules within certain code blocks:

/*eslint-disable no-alert, no-console */

alert('foo');
console.log('bar');

/*eslint-enable no-alert */

[nylen]

thanks @seanstrom for the separation of duties :)