Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

New Task Format Proposal #497

Closed
tkellen opened this Issue · 19 comments

6 participants

@tkellen
Owner

This proposal outlines a task format that can be used with the current grunt API (0.4+), enables meaningful unit testing, and could eventually provide robust support for calling task functionality from within other tasks.

Please poke holes in my example below (a refactoring of grunt-contrib-concat). I'd love to see all contrib tasks re-written in this form but I'm sure I've missed a few things.

'use strict';

module.exports = function(grunt) {

  // Internal lib.
  var comment = require('./lib/comment').init(grunt);

  // Prep task export
  var task = {};
  task.name = 'concat';
  task.description = 'Concatenate files.';

  // The option processor for a task should be a discreet
  // function which can be tested externally.
  task.processOptions = function (input) {
    var options = input({
      separator: grunt.util.linefeed,
      banner: '',
      stripBanners: false,
      process: false
    });
    // Normalize boolean options that accept objects.
    if (options.stripBanners === true) {
      options.stripBanners = {};
    }
    if (options.process === true) {
      options.process = {};
    }
    return options;
  };

  // Reading a file is a discreet action most file-based tasks perform.
  // Any task-specific or convention based transformations should take
  // place here.  Logging can also happen here.  If this is not defined,
  // grunt could provide a default method when the task is registered.
  task.readFile = function (filepath, options) {
    // Warn if a source file/pattern was invalid.
    if (!grunt.file.exists(filepath)) {
      grunt.log.error('Source file "' + filepath + '" not found.');
      return '';
    }
    // Read file source.
    var src = grunt.file.read(filepath);
    // Process files as templates if requested.
    if (options.process) {
      src = grunt.template.process(src, options.process);
    }
    // Strip banners if requested.
    if (options.stripBanners) {
      src = task.stripBanner(src, options.stripBanners);
    }
    return src;
  };

  // Writing a file is a discreet action that most file-based tasks perform.
  // Any task specific or convention based transformation should take
  // place here.  Logging can also happen here.  If this is not defined,
  // grunt could provide a default method when the task is registered.
  task.writeFile = function (filepath, src, options) {
    return grunt.file.write(filepath, src);
  };

  // The main task processor.  This is what tasks could eventually
  // call if they want to utilize functionality from other tasks.
  // Takes an optional done callback for async handling.
  task.pipe = function (input, options, done) {
     // Process banner if needed.
    var banner = grunt.template.process(options.banner);

    // The source files to be concatenated. The "nonull" option is
    // used to retain invalid files/patterns so they can be warned
    // about.
    var files = grunt.file.expand({nonull: true}, input);

    // Concat banner + specified files.
    var concat = files.map(function (filepath) {
      return task.readFile(filepath, options);
    }).join(grunt.util.normalizelf(options.separator));

    // return processed output
    return banner+concat;
  };

  // The main task entry point, eventually called by grunt instead
  // of the shim below.  Takes an options object and an optional set
  // of input files.
  task.invoke = function(options, input) {
    // process options
    options = task.processOptions(options);
    // get result
    var result = task.pipe(input.src, options);
    // write it
    task.writeFile(input.dest, result, options);
  };

  // Formerly an external lib
  task.stripBanner = function(src, options) {
    if (!options) { options = {}; }
    var m = [];
    if (options.line) {
      // Strip // ... leading banners.
      m.push('(?:.*\\/\\/.*\\n)*\\s*');
    }
    if (options.block) {
      // Strips all /* ... */ block comment banners.
      m.push('\\/\\*[\\s\\S]*?\\*\\/');
    } else {
      // Strips only /* ... */ block comment banners, excluding /*! ... */.
      m.push('\\/\\*[^!][\\s\\S]*?\\*\\/');
    }
    var re = new RegExp('^\\s*(?:' + m.join('|') + ')\\s*', '');
    return src.replace(re, '');
  };

  // shim new task format
  grunt.registerMultiTask(task.name, task.description, function () {
    // if there is a `files` object, grunt could invoke the task
    // once for each key/value pair, eliminating this level of
    // nesting.
    this.files.forEach(function(fileObj) {
      // invoke task with same context so we can access things like
      // this.async();
      task.invoke.apply(this, [this.options, fileObj]);
    }, this);

  });

  return task;
};
@tkellen
Owner

Just a point of clarification: I'm not suggesting we try to make this happen before 0.4!

@shama
Owner

+1
I especially want to do this to the watch task as I'm currently duplicating functionality in grunt-hub.

@cowboy
Owner

Please don't actually do this yet, anyone. We're just throwing ideas around!

@tkellen
Owner

Seriously. This needs major revision.

@shama
Owner

No worries; just liking the format and the possibility of stream based tasks. I will have some ideas to throw your guy's way soon. :)

@millermedeiros

One thing that I think is a design error is to inject grunt to the task method, it won't be future proof since you might have conflicting grunt versions (imagine grunt v2.0 in a couple years). It would be better if each task required grunt by themselves, so they could depend on different grunt versions (npm is great at handling conflicting dependencies).

I also don't agree with tasks doing any call to grunt.log or grunt.verbose. It would be better if each task emitted events, that way the tasks could be called from other runners and still provide helpful info about the process.

I would probably expose multiple methods for each task and add all the task configuration as properties of the module:

// require grunt explicitly (only if you need) so we could potentially have
// tasks that depend on different versions of grunt. It would also allow tasks
// to be used outside of grunt which is a HUGE win. Many tasks don't need grunt
// at all.
var grunt = require('grunt');

// expose info about the task
exports.name = 'awesome';
exports.description = 'Do awesome things';
exports.type = 'multi'; // "multi" or "single" (case insensitive)

// events so we avoid the need of calling `grunt.log` directly
var EventEmitter = require('events').EventEmitter;
exports.emitter = new EventEmitter();

// we expose multiple methods so other tasks can call them directly and it will
// also reduce the need of using strings to identify methods so no more need
// for `grunt.helper('helperName')`, you can simply `require('taskName')` as if
// it was a regular node.js dependency.
exports.readFile = function(filePath, options){
    if (!grunt.file.exists(filepath)) {
        // we don't log things directly, we delegate the logging to the task
        // runner, will also make it easier to reuse tasks
        exports.emitter.dispatch('error', 'Source file "' + filepath + '" not found.');
        return '';
    }
    // ...
};
exports.writeFile = function (filepath, src, options) {
    // we can reuse grunt methods if it's helpful
    return grunt.file.write(filepath, src);
};

// entry point used by grunt or any other tool
// (if invoke contains 3 parameters it is considered async)
exports.invoke = function(options, input, done){
    // ...
};

There is no reason for the task to call grunt.registerTask and grunt.registerMultiTask if you already call grunt.loadTasks and grunt.loadNpmTasks to load them. Leave the registerTask only if you want multiple tasks inside the same file (inside your grunt file) and discourage its usage. In the long run it will be more flexible.

I would also remove all the magic properties added to this inside tasks. Very confusing and it's hard to predict what a task will need. Better to pass everything as an argument to invoke() and do all the requiresConfig and requires logic as a configuration inside the gruntfile or as a property of the module itself.

Every single grunt task is tightly coupled with the runner, the current approach isn't scalable and will lead to headaches in the future. I hope you guys consider some of the suggestions. The earlier these problems are addressed the better - less people affected and less code to be updated/ported.

Cheers.

@cowboy
Owner

I've actually considered emitting log events instead of logging directly. Interesting, thanks for the input.

@tkellen
Owner

I agree 100% with everything you've posted Miller. My proposal was deliberately conservative, in the hopes of moving things forward in a backward-compatible way.

Just FYI, the primary reason for passing around the instance of grunt is that it contains a bunch of state about how it was invoked (debugging / dry run, etc). Assuming we went this direction, we'd still have to pass some context around so that the individual grunts ran their functionality in the right state.

@millermedeiros

@tkellen I think it should formalize the arguments that we expect on invoke (and keep it as brief as possible), maybe something like:

task.invoke({
  flags : { foo : true, dryRun: true, debug: false },
  options : { ... },
  args : [ ... ]
});

@cowboy It would be extremely useful if all tasks could be executed as regular node modules, that way we could reuse a lot of stuff on other programs (not only command-line interfaces). Maybe it should use EventEmitter2 instead of the native EventEmitter since grunt uses it (just saw issue #328) and it's way more flexible (wildcards, onAny()). I also think that Grunt should bubble the events if it is being run from another node.js program and suppress logs (grunt would have a separate logger module that is only enabled if running from command-line or called explicitly - could be a separate npm package).

@tkellen
Owner

BOOM! I just secured the 'task' package on npm for this project. Coming soon (okay, coming in like, many months) to a gruntplugin near you, var task = require('task');

@tkellen
Owner

If anyone wants to see where were are headed for 0.5, check this out:
http://github.com/tkellen/node-task

@geddski

@tkellen after a brief glance I didn't see how one would define a multitask (or other types like parallel task).

Also I really wish the grunt project would do releases more like node/angular/others, where even is stable and odd is unstable. Working with the "devel" branch of 0.4 was a royal pain, having to reference specific commits rather than semver releases. The current setup makes it difficult for us to test the upcoming stable release (something node and other projects find invaluable). Please consider.

@tkellen
Owner

Tasks which conform to the node-task spec will return a promise which, when resolved, will signal the runner to continue. In order to support parallel tasks, the current plan is to allow the assignment of "alias" tasks with a flag stating that the resolution of the promise is not required to continue. Basically, running in parallel is not a task-level configuration--that is controlled by the runner executing the task(s). The concept of a multitask will more or less be going away, but the specifics of the implementation are not yet decided.

I hear you on the release pain. We've handled 0.4 very poorly and it won't happen again. I'm not familiar with the even/odd convention, but I'd be happy to read any blog posts or other documentation that outlines what you consider a best-practice for release management.

@tkellen
Owner

The basic architecture we're shooting for at the moment is this:

  1. Tasks as npm modules that can be required and run independent of any task runner (if you want to manually build a compliant config object to execute it). You will also be able to pipe data between multiple tasks (think coffescript transpilation + uglify in a single step).
  2. A library for parsing configurations (template expansion, glob expansion etc) from the current Gruntfile format, into a valid form for running node-task compliant modules. Will support user-defined middleware for controlling config output.
  3. A task runner which uses said config parsing library to execute node-task compatible modules (can be used programmatically, or via cli)
  4. All task output emitted as events which can be listened to by any compatible logger (the default being a console logger that produces output very similar to what we currently have).
@geddski

The even/odd convention is awesome. Node.js for example is on version 0.8.16. 0.8.x is the stable branch, receiving no new features, only bug fixes. Meanwhile, work on the 0.9.x branch is well underway, with new features and improvements (like streams2). Any relevant bug fixes in the unstable branch (0.9.x) are cherry picked into stable (0.8.x). As soon as the unstable branch is deemed ready, it becomes the next stable release (0.10.x) and a new unstable branch begins (0.11.x).

This is great because the stable branch continues to harden from bug fixes, and working with the unstable branch is easy because there are frequent semver releases (0.9.2, 0.9.3, etc).

For an example npm module that uses this convention well see testacular. As for their repo they use 'master' branch for 'unstable' and 'stable' branch for stable releases.

@sindresorhus
Collaborator

I've always found even/odd releases weird, and moot with semver. 0.4.0-alpha.1 is IMHO much more succinct. With even/odd you have to know about it to understand that eg. Node 0.9 is an unstable version.

@geddski

@sindresorhus it's a convention that's been used successfully on large projects with many contributors, like Node, Ruby, and the Linux kernel. But you're right, the end results are what matter and can be achieved with a number of conventions. What counts is that there are very frequent (npm) releases on the alpha branch, bugs get back ported to stable, and the project is easy to test/contribute to.

If using the prerelease section of semver it may be helpful to attach more semantics: 0.4.0-alpha.0.2.3 (so you can still track major, minor, and patch changes within the context of upcoming 0.4). But I haven't verified that node-semver compares these correctly.

EDIT: it does compare correctly.

semver.gt('0.4.0-alpha.0.1.2', '0.4.0-alpha.0.1.1'); //true
semver.gt('0.4.0-alpha.0.1.2', '0.4.0-alpha.0.1.4'); //false
@sindresorhus sindresorhus referenced this issue in IndigoUnited/automaton
Closed

Support Gruntjs tasks #7

@tkellen
Owner

Great discussion here. I think it can continue over at http://github.com/tkellen/node-task or at http://github.com/gruntjs/grunt/wiki/Grunt-0.5-Roadmap

@tkellen tkellen closed this
@millermedeiros millermedeiros referenced this issue in node-task/spec
Closed

emitter #5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.