Allow function to be passed & executed if enabled

[ghost]

Title: Optimised debug information collection

Use Case: Sometimes producing debug output is extra intensive on resource. If it won't be displayed (debug environment var not set or doesn't match) there is no point in generating the debug info. Such heavy debug information could be binding prepared queries params to produce the actual query for debug, etc. For this reason, having the ability to wrap inside a callback would improve any such performance issue by only executing debug info when it will be displayed.

Example:

// Normal behaviour.
debug("Hello World"); 

// Only if this should be displayed will it execute the callback
debug(function() {
       return mysql.format(sql,values);
});

This was further discussed here: #370 (comment)

[coveralls]

Coverage increased (+2.8%) to 75.484% when pulling 9aae77f on VBAssassin:master into 3e1849d on visionmedia:master.

[TooTallNate]

Thank you for the patch. It's a 👎 from me personally because there have been plenty of times that I need to inspect a function object through debug.

Instead I'd recommend writing a custom formatter for your app. Something like:

// at the beginning of your app…
const createDebug = require('debug');
createDebug.formatters.q = ({ sql, values }) => {
  return mysql.format(sql, values);
};

// throughout your app code…
const debug = createDebug('your-namespace');
debug('here is a SQL query: %q', { sql, values });

[TooTallNate]

I would possibly be open to adding a built-in formatter (%e for eval perhaps?) that would execute the passed in function:

createDebug.formatters.e = fn => fn();

debug('%e', () => mysql.format(sql, values));

[ghost]

When ever I pass a function to debug for me it just says "[Function]" which isn't useful at all (at least not to me anyway). How have you been making it work differently for you?

[TooTallNate]

I decorate function objects with properties. See NodObjC, node-ctypes, node-function-class, for some examples.

[ghost]

Oh I see. I don't usually mess with those kind of things myself (although maybe I should). Neat trick being able to use formatters to solve a performance related issue 👍 I'm assuming the formatter wouldn't format the data and then discard it if the ns was disabled? I've not been through the code but will take your word on your answer there.

I don't believe most people decorate their functions (though I could be wrong)? I'm going by the fact that quite a few people have upvoted this suggestion in the RFC 3.0 discussion so I know they don't use debug with decorated functions in mind. Yet all of them will likely deploy to production systems and have performance in mind. Being a little cheeky here (no offence intended)... In which case maybe it would be best if your use case was done as a custom formatter and this patch applied by default as it applies to performance (nearly every use case), not formatting (for those who decorate functions)?

I hope I've not confused the use case of this by using the mysql formatter implying it's "formatting" related. Sorry, as I can see how upon scanning over this that that would be the first thing to imply. In fact, it's purely performance related. Some debug() information is expensive to obtain/track. Before this patch, I was having to wrap debug() inside boilerplate code like this:

if (debug.enabled) {
     // Do expensive operation to obtain debug data here...
    //...
    //...
     debug(...); // Now call debug to output the obtained debug data
}

It's cool either way as I've forked it but out of courtesy and interest in it from the RFC3.0 discussion I thought I'd ping it over to you in case you wanted it 👍

[TooTallNate]

Well like I said I'm open to a change here, so I want other maintainers opinions as well. Your proposal is a breaking change and we just did v3, but that shouldn't really matter towards anything, it's just an number. Do you not like my %e built-in formatter proposal?

[ghost]

If this was in a synchronous language like PHP or C I would have said yes, go with the formatters route. But with JS I'd possibly lean towards debug(function(){}) / ES6 syntax. I also believe it's less misleading in that it's related to performance and not formatting.

I can't deny the breaking change though. If your built in formatter option works as expected though. It really boils down to preference and the community.

Personally (although I'm obviously biased). I'm leaning towards the debug(function(){}) route as it looks cleaner to me, more JS like than C, the community appears to like it and it's instantly obvious that it's done for performance reasons and not formatting (I wouldn't have expected to find this in the formatting section of the docs).

Maybe we could have both (though I haven't considered the pros and cons of having both)? :)

As you said, let's see what other maintainers think and re-evaluate :)

[harrysarson]

Could this be solved by adding an eval function (or similar) to the debug object that would have this behavior. This would enable the desired behaviour without breaking code.

[ghost]

Hello Harry, do you mean a method like this?

debug.eval(function...)

[harrysarson]

Possibly, although I don't know how well eval makes the purpose of the function clear. Also, users may confuse it with the built in eval function.

logResultOf(function) is longer , but could be clearer.

[ghost]

I'm neutral regarding your suggestion Harry. I feel the performance side should impact everyone so making it default and at the forefront makes sense to me. But then debug.resultOf() would be better than nothing at all and as you said doesn't break anything, followed by the %e filter suggestion by Nate. 3 ways of doing the same thing... we now have options to consider 👍

[Qix-]

The only suggestion here I think belongs is the %e formatter.

What you're essentially doing is this:

if (debug.enabled) {
    debug(someFunction(...));
}

Wrapping the branch in a function, as is proposed here, is going to be a performance penalty and, in my opinion, makes the debug() function way too magical.

What @TooTallNate is suggestion makes sense; ultimately, debug is looking to print a string. A format specifier is extremely clear in that regard.

debug('%e', someFunction);
debug('Hello, %e!', ()=> 'World');

However, I'm also 👎 on the idea as a whole.

Debug output should be supplemental. This suggestion is basically trying to execute a function only if debugging is enabled. This is an anti-pattern; the behavior of your program shouldn't change based on debugging levels, only the output.

I foresee this being misused to great disadvantage if implemented in any capacity. If you can't execute the function right then, regardless of the debugging level, IMO that's instantly code smell.

Not sure we should encourage the use of an anti-pattern.

[ghost]

Hello Qix. You do realise this isn't about "formatting"? That in it's own right is misleading. If you believe there is an anti pattern here then using formatting for the purpose of performance must also fall somewhere in the anti pattern, code smell, some other thing that suggests it's bad. I know the way I suggested isn't perfect but given the use case of debug I believe it makes sense. In the same way that people know not to put tomatoes in the fruit bowl.

So it would appear, given your comment, the best way would actually be HarrySarsons suggestion. A new method wouldn't foul the behaviour or break compatibility with debug. It would allow those who decorate functions to still use it as expected. It would also be clear as to what it does, it's purpose and only do one thing. The only downside would be it's not quite as convenient. But given the alternatives this may be the lesser of the evils for the community.

[harrysarson]

I guess the concern is that the function you pass to debug could have side effects. A forced example:

var x;

debug.resultOf(() => (x = x || { run() {} })); 

x.run();

With debug.enabled === false a TypeError is thrown but with debug.enabled === true the code runs successfully.

So enabling debug would change the behaviour of the program which is why it could be an anti-pattern.

[Qix-]

That's exactly my point, @harrysarson. This functionality doesn't belong in Debug IMO. I'll let the other maintainers chime in of course, but this screams feature creep to me.

[ghost]

Yeah, I can see the concern. If a programmer is doing that, they have greater concerns though. I'm sure everyone in this conversation would use it as intended and not abuse it. It's like not giving someone a knife to cook with, out of fear they will kill someone.

After all, bad programmers can always scatter some code like this as it is now without the patch: debug((function(){ return ... //anti pattern based on debug.enabled })())

Qix, I think I may agree with you saying it screams "feature creep" and the main reason I'm using debug over the likes of say winston is the simplicity of debug. After all, debug displays data. Single responsibility.

This extra functionality may be better done in a wrapper that delegates to debug accordingly in which case, would be another project.

Personally, I still like the ability to simply switch it to a function (but then I know not to abuse it). So I'll happily leave it to you guys to decide on behalf of debug.

Great discussion and great to see an active community around this project 👍

[harrysarson]

@VBAssassin I made this for you.

https://www.npmjs.com/package/@harrysarson/eval-debug

[ghost]

Harry, I believe it's only an "anti pattern" if you use it that way (which you obviously shouldn't). The examples others have given here demonstrate the proposed anti pattern (which you can implement anywhere within a project). The examples I gave did not change state outside of the function and would not be considered an anti pattern. What people here have tried to suggest is that it simply opens the door for abuse by amateur developers. I'm not sure if we've all just got a little too pedantic over this.

Thanks for your time in producing that library Harry 👍

[harrysarson]

I put the warning in the readme to inform users of what could go wrong. Maybe it is worded a bit strongly-send a pr to me if you want to rewrite the warning :)

[ghost]

Personally. I would write what it's intended for i.e. performance. Give an example of how it should be used. Then, if you wish give an example of how it should not be used (the anti pattern that's been discussed here).

[Qix-]

I have actually changed opinions on this. I think the %e delimiter is a fine solution to this problem.

Thoughts @TooTallNate and @thebigredgeek?

[Qix-]

Also could you rebase @VBAssassin?

[nmccready]

I'm keeping an eye on this as this is one of the main reasons for (lazy eval and namespace spawning) https://www.npmjs.com/package/debug-fabulous being created.

[Qix-]

@818Nawaf You've been blocked for approving every single PR in the repository and wreaking havoc on my emails and notifications. In the future, I would advise you do not do that. It's turbo annoying.