m19c / gulp-run

Pipe to shell commands in gulp
ISC License
151 stars 25 forks source link

Added stream end to the transform stream to determine when execution is done #17

Closed bleadof closed 9 years ago

bleadof commented 9 years ago

I had a problem that the gulp never knew when this particular stream ended, so here's a solution.

I wanted to fix the tests to instead of call(done) to listen end event and then call done, but for some reason the compare stream also never emits end. I added push(null) to bunch of places but gave up after an hour of fiddling with them. The code is semi hard to read with multiple transform streams.

I'd write the tests so that I would use highland instead of the transform streams. With highland you can use doto to do the actual expects so you won't have to deal with transform streams. I would look into this at some point, but currently I'm too busy. So here's my drive-by PR. :grin:

cbarrick commented 9 years ago

Thanks for the PR!

Before I merge, I have a few questions. First, is it our responsibility to end the stream? Other plugins, like gulp-rename, don't appear to close the stream. gulp.dest appears to close the stream, but it's not very clear to me if we also share this responsibility.

Second, what does this mean when we have multiple input files? If we end the stream in the first call to _transform (even asynchronously), couldn't we conceivably be closing before all files have been passed through?

I know this is an old thread, and I apologize for that. If this is still an issue, could you post your usage so I can check this out with more detail?

taylor1791 commented 9 years ago

I am running into the same problem. If gulp-run does not close the steam how would you do it in a simple example. Take this small bit for example.

gulp.task( 'create-hash',  function() {

  var run = require( 'gulp-run' );

  return run( 'git rev-parse HEAD' )
    .exec()
    .pipe( gulp.dest( '.' ) ) );
} );
cbarrick commented 9 years ago

Hmmm... Other than the syntax error, the example you gave above works for me.

Specifically, the following gulp task calls the command git rev-parse HEAD and put the resulting hash in a file called ./git.

var gulp = require('gulp');
var run = require( './' ); // called from within the gulp-run repo

gulp.task( 'create-hash',  function() {
  return run( 'git rev-parse HEAD' )
    .exec()
    .pipe( gulp.dest( '.' ) );
} );

That doesn't mean that nothing's broken, but the issue must be dependent on a more complex task. Or am I missing something?

taylor1791 commented 9 years ago

Sorry, I didn't see your response or the syntax error. When I run gulp create-hash I get

[18:27:50] Using gulpfile ~/src/scrap/Gulpfile.js
[18:27:50] Starting 'create-hash'...
[18:27:50] $ git rev-parse HEAD 
9433xxxb20

I do not see [18:34:35] Finished 'create-hash' after 39 ms, since there wasn't a stream end. This can prevent a subsequent task from running if it has create-hashas a dependency.

Here is an illustration of the example.

$ gulp test
[18:41:45] Using gulpfile ~/src/scrap/Gulpfile.js
[18:41:45] Starting 'create-hash'...
[18:41:45] $ git rev-parse HEAD 
94336bc45e38d203e04fbb240f6e93618c4b4b20

Where

Gulpfile.js

var gulp = require('gulp');
var run = require( 'gulp-run' ); // called from within the gulp-run repo

gulp.task( 'create-hash',  function() {
  return run( 'git rev-parse HEAD' )
    .exec()
    .pipe( gulp.dest( '.' ) );
} );

gulp.task( 'test', [ 'create-hash' ], function() {
  return gulp.src( 'git' )
    .pipe( gulp.dest( 'git.hash' ) );
} );
taylor1791 commented 9 years ago

@cbarrick Did you get a chance to try out my new example?

cbarrick commented 9 years ago

I just pushed a possible fix to the branch called 17.

gulp-run now properly closes the stream when used with .exec, like @bleadof's fix, but it also properly handles being in the middle of a pipeline, i.e. gulp-run will not close the stream if it didn't start it.

I'll get some proper tests and merge this into master asap

taylor1791 commented 9 years ago

This is great news! Thanks for continuing to investigate this issue.

cbarrick commented 9 years ago

I've merged my branch and published as v1.6.7