slushjs / gulp-install

Automatically install npm and bower packages if package.json or bower.json is found in the gulp file stream respectively
MIT License
106 stars 46 forks source link

'end' event not firing #4

Closed ksmithut closed 10 years ago

ksmithut commented 10 years ago

Using the sample slushfile here, the .on('end', function () {...}) never gets called. When I take out the install plugin it works.

joakimbeng commented 10 years ago

The sample slushfile is currently wrong, see slush #6. The finish event should be used instead.

ksmithut commented 10 years ago

@joakimbeng I've tried that, too, but I'm wanting to detect when all of the dependencies have been installed. the 'finish' event gets fired before the any dependency can be downloaded.

This also might be a different issue, but it appears that if there is no change in the package.json, the install doesn't get triggered. I'm thinking that it's going to be an issue with gulp-conflict, although I believe the functionality is intentional because if there is no conflict, it should not pass the package.json down the pipeline and gulp-install will have no idea that there is even one there.

joakimbeng commented 10 years ago

@ksmithut what happens if you add a noop() to your pipe like this:

  //...
  .pipe(install())
  .pipe(gulpUtil.noop())
  .on('end', function () {...})

Is the end event triggered after the dependencies have finished installing then?

About the gulp-conflict together with gulp-install issue, that's meant to be. If the package.json or bower.json hasn't changed they won't get passed gulp-conflict and the install won't be triggered, which I think is a good behavior. Why run a new install if it's not needed?

ksmithut commented 10 years ago

@joakimbeng ya, it was kind of an edge case thing. I would remove my node_modules directory and slush it, but if someone is manually removing the node_modules directory, then they should know to reinstall their dependencies.

The end event didn't get triggered, but it worked as expected when putting in the 'finish' event.

Thanks for your help!

joakimbeng commented 10 years ago

Glad to hear that, thanks!

blai commented 10 years ago

'finish' is not emitted by all gulp plugins (including gulp.dest). The 'end'e event might not be all that important in gulp as a task manager, but in a generator, I might want to dynamically chain up an array of functions that each perform a portion of the scaffolding job, and then run this to execute them in sequence:

(function next () {
  var g = generators.shift();
  g && g().on('end', next);
})();

assume each item in generators is a function like this:

function generateTests(opts) {
  gulp.src([templateDir + '/test/**/*')
    .pipe(template(opts))
    .pipe(conflicts('./test')
    .pipe(gulp.dest('./test');
}

Would be nice if 'end' is supported. However, I found that this is not a bug of gulp-install itself, I replace .pipe(install()); with an generic through2 and the problem still exist. A workaround is to put pipe(install()) before .pipe(gulp.dest())

adam-lynch commented 10 years ago

This should really be added to the readme or something. I think it's why Travis keeps failing when building slush-slides.


I could be jumping to conclusions but since these are "dependencies" being installed, then it implies that you can't do anything for the user in their directory, using one of these dependencies. I mean when slush is called, you shouldn't put a gulpfile in the user's directory and run a task from it, instead you need to do anything like that in the slushfile.

joakimbeng commented 10 years ago

@adam-lynch the test seems to fail because Bower can't download the Prism repo:

bower ECMDERR       Failed to execute "git ls-remote --tags --heads git@github.com:LeaVerou/prism.git", exit code of #128

i will have to add information about the finish event to the readme as you suggest though.

adam-lynch commented 10 years ago

@joakimbeng hmm thanks! I wonder why that is since it does work fine locally :/ Might be a travis+git thing