feedback

[jonschlinkert]

@cowboy / @tkellen the code in master should be viewed as "placeholder" code.

I'm going to add another issue to get your thoughts on a couple of approaches I'm exploring for event logging. First though, it occurred to me while trying to patch the event logger code into file and options (just to try to do it in more than one lib) that we should consider making all legacy libs constructors. This will make the API uniform and consistent, and when libs like this one are patched in, we can follow the same exact pattern across all of them.

However, if we do this, we'll also need to implement a facade around option, config and any other top-level exports that are currently exposed and used as functions. thoughts?

[cowboy]

I envision this library exporting these functions:

• LogMethodsToEvents - provide the same API that grunt.log currently provides, but instead of logging directly to the console, emit events (maybe on a .event property of the returned instance)
• LogEventsToMethods - takes a reference to to grunt logger grunt.log as the first argument, along with a list of all event emitters to observe. Any time an event is emitted, the corresponding actual grunt.log method is called.

And being used as-follows:

// Inside grunt-legacy-file, for example
var LogMethodsToEvents = require('grunt-event-logger').LogMethodsToEvents;
function File(options) {
  this.log = new LogMethodsToEvents();
  // etc
}
File.prototype.doSomething = function() {
  // exact same API as before but instead of calling .verbose directly call .log.verbose
  this.log.write('foo').ok();
  this.log.verbose.writeln('--verbose').or.writeln('not --verbose');
}

// Inside grunt
var grunt = {};
grunt.log = new require('grunt-legacy-log').Log({grunt: grunt});
grunt.file = new require('grunt-legacy-file').File({grunt: grunt});
grunt.option = new require('grunt-legacy-option').Option({grunt: grunt});
grunt.config = new require('grunt-legacy-config').Config({grunt: grunt});
grunt.fail = new require('grunt-legacy-fail').Fail({grunt: grunt});
// etc

var LogEventsToMethods = require('grunt-event-logger').LogEventsToMethods;
LogEventsToMethods(grunt.log, [
  grunt.file.log.events,
  grunt.option.log.events,
  grunt.config.log.events,
  // etc
]);
// not sure about fail yet, but I think this could work
LogEventsToMethods(customFailHandler, [
  grunt.fail.log.events
]);

So instead of actual log method -> write to stdout, it's proxy log method -> emit event -> handle event -> actual log method -> write to stdout

Does this make sense?

[cowboy]

Also, I think we should rename this library to grunt-legacy-log-events or similar, since I want it to be absolutely clear that this only exists to help us break legacy libs out of Grunt, and won't be used otherwise.

[cowboy]

Or grunt-legacy-event-logger.

[jonschlinkert]

rename this library

consider it done. In general I'll just put a best-guess in place then we can rename to whatever makes sense.

[jonschlinkert]

k

[jonschlinkert]

Any time an event is emitted, the corresponding actual grunt.log method is called.

ok, I had a bullet list of questions I was about to paste up here but that gets rid of most of them...

[tkellen]

I chatted with @cowboy about this this morning and I was going to reply but it looks like he covered the main points of our conversation in the comments above. If you need anything else, just say the word @jonschlinkert!

[cowboy]

Maybe it could something like this (completely untested):

var methods = [
  'write',
  'writelns',
  'ok',
  'error',
  // etc
];

var modes = [
  null,
  'verbose',
  'notverbose'
];

exports.LogMethodsToEvents = function() {
  var log = {
    event: new EventEmitter(),
    verbose: {},
    notverbose: {}
  };
  log.verbose.or = log.notverbose;
  log.notverbose.or = log.verbose;
  methods.forEach(function(name) {
    modes.forEach(function(mode) {
      var namespace = mode ? log[mode] : log;
      namespace[method] = function() {
        log.event.emit(method, {
          mode: mode,
          args: arguments,
        });
        return namespace;
      };
    });
  });
  return log;
};

exports.LogEventsToMethods = function(log, emitters) {
  emitters.forEach(function(emitter) {
    methods.forEach(function(method) {
      emitter.on(method, function(data) {
        var namespace = data.mode ? log[data.mode] : log;
        namespace[method].apply(namespace, data.args);
      });
    });
  });
};

[cowboy]

It might make sense to just emit a generic log event and have the method be a property of the data object, it would make for a lot less event handlers!

[jonschlinkert]

yesterday where we left off was that we were going to create two libs: one as a generic lib for creating the event loggers (some kind of factory), and the other as a facade to grunt legacy libs. I still stand by that as the way to go. it allows us to be generic, keep it simple, and IMHO it will be just as easy to maintain if not easier than creating one lib. Are we still doing the second lib?

If so, the last code snippet is a little closer to what I was thinking because it's a better pattern for us to create a facade around - it's similar to the code I showed you in our call yesterday, except for the namespaces.

today I started working on this (unfinished) (edit: this is from the last hour or so, I did some stuff earlier this morning that was appealing b/c it's considerably simpler, but all of these approaches have drawbacks):

var util = require('util');
var Emitter = require('events').EventEmitter;

/**
 * Create an instance of `EventLogger`
 *
 * @api public
 */

function EventLogger() {
  Emitter.call(this);
  this.methods = [];
}
util.inherits(EventLogger, Emitter);

/**
 * Emit a log event.
 *
 * @param  {String} `name` the name of the log event to emit
 * @param  {String} `message` Message intended to be logged to the console.
 * @return {Object} `EventLogger` for chaining
 * @api public
 */

EventLogger.prototype.base = function(name, message) {
  var event = this.methods.length === 0 ? name : this.methods.join('.');
  this.methods = [];
  this.emit(event, message);
  return this;
};

/**
 * Create a logger with the given `name`.
 *
 * @param  {String} `name` the name of the log event to emit
 * @return {Object} `EventLogger` for chaining
 * @api public
 */

EventLogger.prototype.create = function(name) {
  Object.defineProperty(EventLogger.prototype, name, {
    enumerable: true,
    configurable: true,
    get: buildLogger.call(this, name)
  });
};

/**
 * Create a chainable, logger getter object
 *
 * @param  {String} `name` the name of the log event to emit
 * @return {Function} getter function, used in `defineProperties`
 */

function buildLogger (name) {
  var self = this;
  var getter = function() {
    self.methods.push(name);
    var logger = function (message) {
      return self.base(name, message);
    };
    logger.__proto__ = EventLogger.prototype;
    return logger;
  };
  return getter;
}

/**
 * Expose `EventLogger`
 */

module.exports = EventLogger;

/**
 * example usage
 */

var logger = new EventLogger();

logger.on('info', console.log);
logger.on('log', console.log);
logger.on('info.log', console.log);
logger.on('log.info', console.log);
logger.on('verbose.info', console.log);
logger.on('verbose.error', console.log);

// in the "facade" we would just wrap this and pass an array of methods to create
logger.create('info');
logger.create('log');
logger.create('warn');
logger.create('error');
logger.create('verbose');

logger.verbose.error('verbose.error message');
logger.log.error('info message');
logger.log.warn('log message');
logger.log.info('log.info message');
logger.verbose.info('whammy');

IMHO whether it's something along the lines of @cowboy's last example, or the one I just pasted, I'd like to see this done as two libs to make it clean. If that's not consensus, no worries we can still get all of that implemented, but I don't think it's as cookie-cutter as it seems.

[cowboy]

@jonschlinkert I might be misunderstanding here, but your example seems far more complex than what is necessary to just decouple the legacy libs from grunt.log. Maybe I just don't understand what you're getting at. Can you contextualize it in a grunt-specific example?

Also, @tkellen do you have any suggestions here?

[jonschlinkert]

It make make sense to just emit a generic log event

yeah that definitely has appeal, it's simple. I think we would just need to decide on the args passed to the emitter so that code context can be correctly interpreted in the listeners.. (edit: which you have a good example of in the last code snippet: log.event.emit(method, {mode: mode, args: arguments});)

Can you contextualize it in a grunt-specific example?

give me a sec, I'll try to come up with something.

[tkellen]

I think both approaches are valid. One question I would ask that might be useful in guiding which to choose is this: do we expect this interface to only be used privately or do we plan to have the public logging API actually function using emitters at some point?

[tkellen]

Taking a that a bit further, I think it would be useful to concretely identify the ultimate goal from a functionality standpoint and then allow @jonschlinkert to implement the solution he thinks makes sense.

It seems to me that the complexity of what we're describing here is small enough that our time would be better spent actually building prototypes we can interact with vs discussing what they might look like.

[jonschlinkert]

Example (pseudo-code):

logger.on('info', grunt.log.info);
logger.on('verbose.info', grunt.verbose.info);
logger.on('verbose.warn', grunt.verbose.warn);

[jonschlinkert]

scratch that last example... completely agree with @tkellen's point. e.g. the code isn't what I'm really getting at...

[tkellen]

I just chatted with @cowboy on the phone about this. We think you should run with your idea @jonschlinkert.

As an aside, I also think we should set a deadline/goal for when grunt.file lands back in Grunt. This might seem arbitrary to say here, but I believe we're out at this level of abstraction because it's needed for grunt.file to work? It would be great if we could be focused about what goal we're trying to serve vs getting lost in the semantics of how event logging might potentially work.

If I were picking a date from thin air, I'd say let's try to make this happen by June 5th. If that's not a reasonable date, no worries, set another. Whatever the case, let's have a date!

Excited to see what you come up with @jonschlinkert!

[jonschlinkert]

sounds like a plan, @tkellen thanks!

[cowboy]

Thanks @tkellen. June 5 sounds great, and also, I'm far more interested in getting the legacy grunt stuff done than bikeshedding!

[cowboy]

(Even though it may be impossible to tell)

[jonschlinkert]

lol, just saw that...