Add setup and teardown, or revert back to this.files. #606

Closed
cowboy opened this Issue Jan 7, 2013 · 8 comments

Projects

None yet

3 participants

@cowboy
Member
cowboy commented Jan 7, 2013

I just realized that if we're going to continue with this.file we will need setup and teardown in tasks for 0.4.

Chris showed me his "compress" code. The way it needs to work, fundamentally, is:

  1. create an archive.
  2. add files to the archive.
  3. finalize the archive.

Opening up an already-finalized archive isn't an option.

So, in order for this to work, we really need to get setup and teardown happening.

  • setup: create an archive.
  • eachfile: add the specified file to the archive.
  • teardown: finalize the archive.

With the current "only an eachfile function" implementation, the task function only knows about one src-dest at a time, and as such can't actually utilized the declarative expand mapping system. It'd have to parse it manually. Which isn't good, because then it limits the user to one declarative expand mapping per zipfile, which is really limiting. And also totally gross.

I'm really afraid this is going to push 0.4 off for another month, which is totally not an option. Either way, we need to figure out a solution for this as soon as possible!

Maybe a new .registerTask syntax that looks something more like:

var archiver = require('archiver');
grunt.registerTask({
  name: 'compress',
  description: 'Maybe the task format can be more like this?',
  setup: function() {
    this.archive = archiver.createZip(options);
    this.archive.on('error', function(err) {
      grunt.log.error(err);
      grunt.fail.warn('archiver failed.');
    });
  },
  eachfile: function() {
    var done = this.async();
    this.archive.addFile(fs.createReadStream(this.file.src), {name: this.file.dest}, function() {
      grunt.verbose.writeln('Add ' + srcFile + ' -> ' + destFile + '/' + internalFileName);
      done();
    });
  },
  teardown: function() {
    var done = this.async();
    this.archive.finalize(function(written) {
      grunt.verbose.writeln();
      grunt.log.writeln('File ' + destFile + ' created (' + written + ' bytes written).');
      done();
    });
  },
});

Although I'm afraid that this task format raises more questions than it answers. Like, how do we parse options, how do we persist state, fail tasks, etc.

I'm not sure we can actually do something like this in 0.4, and feel like we might need to revert back to calling the task function once with an already-expanded this.files array, like we did in an earlier 0.4.

@cowboy
Member
cowboy commented Jan 7, 2013

I'm very close to removing the current this.file implicit iteration, where the task function is called once for each file src-dest pair, and reverting back to the earlier 0.4 this.files array, where the task fn is called once and the task author has to iterate over it.

The this.files array will, of course, continue to be fully expanded and have all patterns resolved, which will reduce task boilerplate significantly from what it was when this.files was implemented before.

@cowboy
Member
cowboy commented Jan 7, 2013

And FWIW, @tkellen and I have already been prototyping a setup/eachfile/teardown system for 0.5. It's gonna be awesome.

@sindresorhus
Member

I think reverting back to this.files is probably the best options for now.

Though this will break almost every single task, since 0.3 didn't have this.files and all tasks that has upgraded to 0.4pre uses this.file...

Previously brought up in #546

@cowboy
Member
cowboy commented Jan 7, 2013

I can do what I did before, which is to set this.file = this.files[0];. I could possibly make this.file a getter method that warns that the task is using a deprecated API, as well.

@ctalkington
Member

im trying to think of times when tasks would care if it was run individually. mainly only when working with a single output file from what i've come up with. maybe we should build a list of more times when current logic will be a problem?

i think for v0.4 we could get away with a way for task define that it cant run multiple times per target. and in those cases grunt can just compile all the file pairs and pass it as an array for that task to handle. seems easiest route?

@cowboy cowboy added a commit to gruntjs/grunt-contrib-jshint that referenced this issue Jan 7, 2013
@cowboy cowboy Updating to support new this.filesSrc property per gruntjs/grunt#606. 1fb8000
@cowboy cowboy added a commit to gruntjs/grunt-contrib-nodeunit that referenced this issue Jan 7, 2013
@cowboy cowboy Updating to support new this.filesSrc property per gruntjs/grunt#606. 3c80570
@cowboy
Member
cowboy commented Jan 7, 2013

I have reverted the whole gh-524 file object implicit iteration abstraction, you can get the latest version here:

https://github.com/gruntjs/grunt/tree/file-mapping

I'll repeat my commit comments here:

  • The previous abstraction, while nice, didn't allow for practical per-task setup/teardown. Instead of building further abstraction, I am reverting. This will be addressed in 0.5.
  • Multi task functions are now run once per target, not once per file mapping object.
  • Inside tasks, this.files is an array of fully normalized/expanded file mapping objects, each with .src .dest .orig properties (as appropriate)
  • Inside tasks, this.filesSrc is a flattened, uniqued array of all this.files object .src properties.
  • Task authors will be responsible for iterating over this.files or this.filesSrc to process files in their multi task.

Tasks can iterate over this.files synchronously like this:

this.files.forEach(function(f) {
  doSomethingSyncWith(f.src, f.dest);
}, this);

Tasks can iterate over this.files asynchronously like this (forEachSeries could be used as well):

grunt.util.async.forEach(this.files, function(f, next) {
  doSomethingAsyncWith(f.src, f.dest, next);
}.bind(this), this.async());

Synchronous read-only tasks like jshint can do this:

this.filesSrc.forEach(function(filepath) {
  doSomethingSyncWith(filepath);
}, this);

Please review my commit ASAP so I can start updating plugins to work with the new (old) format, /cc @sindresorhus @shama @tkellen @ctalkington @tbranyen

@sindresorhus
Member

Lgtm

@cowboy
Member
cowboy commented Jan 8, 2013

Ok, these changes (and others, including the new declarative file mapping stuff) are now in master. Time to update some contrib plugins!

@cowboy cowboy closed this Jan 8, 2013
@cowboy cowboy added a commit to gruntjs/grunt-contrib-clean that referenced this issue Jan 8, 2013
@cowboy cowboy Updating to support new this.filesSrc property per gruntjs/grunt#606. c94b3be
@cowboy cowboy added a commit to gruntjs/grunt-contrib-concat that referenced this issue Jan 8, 2013
@cowboy cowboy Updating to support new this.files property per gruntjs/grunt#606.
* this.files.forEach iterates over all src-dest file pairs.
* f.src.filter() warns on and removes possible invalid source files (in case the user set nonull), but doesn't fail the task.
* Uncaught exceptions will fail the task.
1ac25da
@cowboy cowboy added a commit to gruntjs/grunt-contrib-qunit that referenced this issue Jan 8, 2013
@cowboy cowboy Updating to support new this.filesSrc property per gruntjs/grunt#606.
* Since grunt.file.expandFileURLs has been removed, URLs may now be specified via a "urls" option.
* URLs and src files will be concatenated and processed in-order.
d504b04
@cowboy cowboy added a commit to gruntjs/grunt-contrib-uglify that referenced this issue Jan 8, 2013
@cowboy cowboy Updating to support new this.files property per gruntjs/grunt#606.
* this.files.forEach iterates over all src-dest file pairs.
* f.src.filter() warns on and removes possible invalid source files (in case the user set nonull), but doesn't fail the task.
* Uncaught exceptions will fail the task.
db66d34
@vtsvang vtsvang referenced this issue in gruntjs/grunt-contrib-uglify Jan 9, 2013
Closed

Fixed support of new this.file property per gruntjs/grunt#606. #18

@shama shama referenced this issue in gruntjs/grunt-contrib-jshint Jan 9, 2013
Closed

Updating to support new this.files property per gruntjs/grunt#606. #11

@shama shama referenced this issue in gruntjs/grunt-contrib-nodeunit Jan 9, 2013
Closed

this.filesSrc not defined in v0.1.1 #6

@shama shama referenced this issue in gruntjs/grunt-contrib-nodeunit Jan 9, 2013
Closed

Updating to support new this.files property per gruntjs/grunt#606. #7

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-jasmine Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #17

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-copy Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #33

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-htmlmin Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #5

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-jst Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #12

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-stylus Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #21

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-yuidoc Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #4

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-requirejs Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #8

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-cssmin Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #8

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-less Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #14

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-imagemin Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #3

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-handlebars Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #11

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-compress Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #14

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-compass Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #4

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-pug Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #10

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-coffee Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #11

@cowboy cowboy referenced this issue in gruntjs/grunt-contrib-sass Jan 9, 2013
Closed

Update for compatibility with grunt 0.4.0rc5 #15

@shama shama added a commit to gruntjs/grunt-contrib-htmlmin that referenced this issue Jan 9, 2013
@shama shama Updating going back to this.files property per gruntjs/grunt#606. 28d838d
@shama shama added a commit to gruntjs/grunt-contrib-jst that referenced this issue Jan 9, 2013
@shama shama Switching to this.files property per gruntjs/grunt#606. f6d67bc
@shama shama added a commit to gruntjs/grunt-contrib-stylus that referenced this issue Jan 9, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. accc26c
@shama shama added a commit to gruntjs/grunt-contrib-coffee that referenced this issue Jan 10, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. 2a867a1
@shama shama added a commit to gruntjs/grunt-contrib-handlebars that referenced this issue Jan 10, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. 58be8df
@shama shama added a commit to gruntjs/grunt-contrib-pug that referenced this issue Jan 10, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. c0e6268
@shama shama added a commit to gruntjs/grunt-contrib-less that referenced this issue Jan 10, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. 34f5d55
@shama shama added a commit to gruntjs/grunt-contrib-cssmin that referenced this issue Jan 10, 2013
@shama shama Updating to support new this.files property per gruntjs/grunt#606. 20f9247
@dfernandez79 dfernandez79 added a commit to dfernandez79/grunt-simple-mocha that referenced this issue Jan 10, 2013
@dfernandez79 dfernandez79 Changed this.files.src to this.filesSrc per gruntjs/grunt#606 60f33ca
@shama shama added a commit to gruntjs/grunt-contrib-sass that referenced this issue Jan 10, 2013
@shama shama Switching to this.files property per gruntjs/grunt#606. Updating for …
…grunt 0.4.0rc5.
d6dfe52
@tony tony pushed a commit to tony/grunt-recess that referenced this issue Jan 17, 2013
Tony Narlock Updating to support new this.filesSrc property per gruntjs/grunt#606. 7c4912f
@tony tony added a commit to tony/grunt-recess that referenced this issue Jan 17, 2013
@tony tony Updating to support new this.filesSrc property per gruntjs/grunt#606. b894854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment