sindresorhus / gulp-filter

Filter files in a `vinyl` stream
MIT License
315 stars 37 forks source link

Filter without restoration exits gulp before task finishes #54

Closed adammockor closed 8 years ago

adammockor commented 8 years ago

My gulp task:

gulp.task('useref', 'Bundle CSS and JS based on build tags and copy to `dist/` folder', function () {
  // run useref only in build
  if (build.isBuild()) {
    var assets = useref.assets(config.useref.assetsCfg);

    var jadeFilesOnly = filter(['**/*.jade'], {restore: true});
    var excludeJade = filter(['**','!**/*.jade'], {restore: true});

    return gulp.src(config.useref.src)
      .pipe(assets)
      .pipe(gulpif('*.js', gulpif(config.uglifyJs, uglify()))) // uglify JS
      .pipe(gulpif('*.css', gulpif(config.minifyCss, minifyCss()))) // minify CSS
      .pipe(gulpif(config.cacheBust, rev()))
      .pipe(assets.restore())
      .pipe(useref())
      .pipe(gulpif(config.cacheBust, revReplace({replaceInExtensions: ['.jade', '.css', '.js']})))
      .pipe(jadeFilesOnly)
      .pipe(gulp.dest(config.useref.destJade))
      .pipe(jadeFilesOnly.restore)
      .pipe(excludeJade) 
      .pipe(gulp.dest(config.useref.dest))
      .pipe(gulpif(config.cacheBust, rev.manifest(config.useref.revManifestCfg))) // create rev-manifest.json 
      .pipe(gulp.dest(config.useref.dest));
  } else {
    return;
  }
});

This .pipe(excludeJade) silently exits gulp flow without error. If I add.pipe(excludeJade.restore)` in the end of stream, everything works as expected. (OSX 10.11/Windows 7, Node 0.12.7/4.2.0)

Restoration is optional according the docs.

nfroidure commented 8 years ago

The restore option is optionnal but you have to declare it in options by setting restore to false.

adammockor commented 8 years ago

So when restore option is removed from:

var excludeJade = filter(['**','!**/*.jade']);

it works without .pipe(excludeJade). Thank you.

Before it was strange, because sometimes it works sometimes not.

paxperscientiam commented 8 years ago

I think this issue should be re-opened.

The problem, I think, is that when the "restore" option is set to true, the task will fail silently (no return signal?) if the filter method is not actually used. ie, this will fail:

var jsFilter = G.filter('**/*.js',{restore: true});

gulp.task('ttt', ['clean:all'],function() {
    "use strict";
    return gulp.src(G.MBF(footFiles))
    .pipe(jsFilter)
    .pipe(G.debug());
});

While, this will not:

var jsFilter = G.filter('**/*.js',{restore: true});

gulp.task('ttt', ['clean:all'],function() {
    "use strict";
    return gulp.src(G.MBF(footFiles))
    .pipe(jsFilter)
    .pipe(G.debug())
    .pipe(jsFilter.restore);
});

Use-case: debugging.

nfroidure commented 8 years ago

Sadly, the task doesn't really fail, it endlessly waits for the restore stream to be consumed. I see no proper way to avoid people messing up with the restore option that hadn't already be done (like the restore option set to false per default).

Opting in the restore option means you know what you're doing.

To be honnest, i see a way to help people debugging it but i find it pretty ugly:

paxperscientiam commented 8 years ago

Hey nfroidure, while I lack the insight to comment on the inner workings of Gulp and Node, I do agree that some sort of descriptive error message would be helpful. At the very least, such a warning should be added to the documentation.

nfroidure commented 8 years ago

What about adding this to the README restore option description ?

By enabling the restore stream, you declare your intent to effectively consume the restore stream. Since this module respect back pressure, your filter stream may never end if you do not consume the restored one.

paxperscientiam commented 8 years ago

I will add that now :+1: