Client Side Debugging

[prashn64]

issue #137

Based on Jimmy's notes:

https://github.com/jimmyhchan/dustjs/blob/debugger/lib/dust.js

[vybs]

@prashn64 how do we unit test these changes?

[prashn64]

@vybs one idea I just had was to set the logger to syncronous, run the code, and then we can look at the errorList's most recent element to see if it matches our expectation. This would likely be put in coreTests.

[jimmyhchan]

This will break in v8 and node where window is not defined.

in dustjs-helpers we just did:

/* make a safe version of console if it is not available
 * currently supporting:
 *   _console.log
 * */
var _console = (typeof console !== 'undefined')? console: {
  log: function(){
     /* a noop*/
   }
};

[brikis98]

Some feedback and questions:

1. Why are we rolling our own logging library? Have we done a thorough search of existing options to see if we can leverage their work? There are a few listed here: http://ajaxpatterns.org/Javascript_Logging_Frameworks
2. If for some strange reason, it turns out we're the only company on earth to do client-side logging, and we absolutely must build it from scratch, then why build it in the dust namespace? It seems that we would need a general client-side logging solution at LinkedIn. I already saw fz.js using one and I'm sure there are a half dozen others. This leads to inconsistency, duplication of effort, and increased payload. If we're going to build something, we should do it as a tiny, standalone, open source project and include it in all of our other JS code as a dependency.
3. Logging to console is a good start, but we need much more. For example, in dev mode, I'd prefer that any error gets thrown as an exception. We want to fail loudly to get devs to fix issues ASAP. In prod, we should generally fail silently, but logging to the console is not enough. We should instead fire an ajax call to a server, that in turn logs an error or fires a kafka event, would be far more useful. Perhaps we can re-use lite for this?
4. Stylistic comment: instead of log.(<a number>, <the message>), I think we should just have dedicated methods like log.debug(<message>), log.error(<message>), etc.

[prashn64]

@brikis98

1. That is one option, although I'm hesitant to have too many dependencies on other libraries. Additionally, for things like failing hard and throwing errors, it's useful to throw an error within the log call, if the logger has been configured to fail hard.
2. It's useful to have a logging library specific to dust, so that we can control when to turn off logging. If it was shared amongst all libraries, we would turn off logging for those libraries as well.
3. I agree with this. I think it would be part of a separate pull request in order not bloat this one too much, but we can have a task following this one for this feature. One issue to consider is making it generic enough to work outside of LinkedIn.
4. how about log.(warn, 'the message')? Basically a human readable parameter instead of a number which doesn't have much context. The reasoning is that if all logging routes through one function, it's cleaner to shut off logging as opposed to if there were four different functions.

[brikis98]

1. Depends on the size of the library. Also, you assume that one you create by hand, when it has all the bells and whistles and bug fixes, would actually be smaller.
2. Not if you create the library correctly. Most let you grab a named instance of the logger for use just for your code. For example:

var log = Logger.getLogger('dust');
log.info("This log instance only affects dust");

1. Yup, separate PR makes sense.
2. All 4 functions would call back to the same underlying one, so they could all be controlled in one place, not 4.

info = function(message) {
  this.log('INFO', message);
},

debug = function(message) {
  this.log('DEBUG', message);
},

log = function(level, message) {
  // Do actual logging
}

[jimmyhchan]

something is redundant here. you're pushing CORE into CORE_MIN below

[prashn64]

I think it's ok, it's just minifying the core. What I did find is the HEADER has always been overwritten by the minifier, so I'm fixing that in the next commit.