Not working anymore since 4.0.0 #67

sinedied · 2016-04-21T07:31:58Z

I have this gulp task that works perfectly with gulp-filter@3.x:

gulp.task('build:sources', ['inject'], function() {
  var htmlFilter = $.filter('*.html', {restore: true});
  var jsFilter = $.filter('**/*.js', {restore: true});
  var cssFilter = $.filter('**/*.css', {restore: true});

  return gulp.src(path.join(conf.paths.tmp, 'index.html'))
    .pipe($.replace(/<html/g, '<html ng-strict-di'))
    .pipe($.useref())
    .pipe($.if('**/app*.js', $.intercept(setEnvironment)))
    .pipe(jsFilter)
    .pipe($.ngAnnotate())
    .pipe($.uglify({preserveComments: $.uglifySaveLicense})).on('error', conf.errorHandler('Uglify'))
    .pipe($.rev())
    .pipe(jsFilter.restore)
    .pipe(cssFilter)
    .pipe($.cleanCss({processImport: false}))
    .pipe($.rev())
    .pipe(cssFilter.restore)
    .pipe($.revReplace())
    .pipe(htmlFilter)
    .pipe($.htmlmin({
      removeComments: true,
      collapseWhitespace: true
    }))
    .pipe(htmlFilter.restore)
    .pipe(gulp.dest(path.join(conf.paths.dist, '/')))
    .pipe($.size({title: path.join(conf.paths.dist, '/'), showFiles: true}));
});

Once I upgrade gulp-filter to 4.x, files do not get filtered anymore... I've read through the docs and I could not find any breaking change, what could I be missing?

nfroidure · 2016-04-21T09:05:57Z

There are similar issues in the closed ones: https://github.com/sindresorhus/gulp-filter/issues?q=is%3Aissue+is%3Aclosed

sinedied · 2016-04-21T13:00:35Z

I am aware, but it does not solve my issue.

Is there a good reason to force using process.cwd(), as it is not desirable in my case.
The issue seems to lies in the restore part, as my build seems to be working and receiving files, but it seems they are somehow reverted back to before their processing somehow...

nfroidure · 2016-04-21T15:25:52Z

It is more probable that the filter was just applied for unexpected files ;). To be honest i have no idea of the implications of that change, i'm not an expert of the node-glob edge cases. Try to poke the commit author.

sinedied · 2016-04-22T06:52:17Z

@cb1kenobi could you please explain why using cwd was needed in the first place? (I did not saw an issue reference in your PR)

I saw many projects that are sticking to 3.x due to this new behavior, maybe it would be nice to be able to chose whether or not to use a path relative to cwd or not? If it seems ok for you, I can prepare a PR for this, to make it optional.

nfroidure · 2016-04-22T07:06:52Z

@sinedied more info for you here #61

cb1kenobi · 2016-04-22T07:49:02Z

@sinedied I wrote a decent description in the PR of the rationale behind the change: #59.

I believe my change is more correct than before my change.

Before my change, minimatch would be comparing your **/*.js filter to file.relative which contains just the filename and no path info. I believe this is a design flaw in gulp. This means gulp-filter was never performing the correct match, but you probably didn't notice or care as long as all of your *.js files passed through the filter.

However, I agree there is still an issue. If the relative path resolves outside the current directory tree, then minimatch will not properly match it. In other words, minimatch doesn't appear to like it if the path starts with ... I don't know if that's a bug in minimatch or what.

For example:

var minimatch = require('minimatch');
console.log(minimatch('../bar.js', '**/*.js')); // false
console.log(minimatch('/bar.js', '**/*.js')); // true

I did a quick test and said if the relative path starts with .. (it's outside the current directory), then resolve it to an absolute path before calling minimatch:

// change this:
var match = typeof pattern === 'function' ? pattern(file) :
    multimatch(path.relative(file.cwd, file.path), pattern, options).length > 0;

// to this:
var match;
if (typeof pattern === 'function') {
    match = pattern(file);
} else {
    var relPath = path.relative(file.cwd, file.path);
    if (relPath.indexOf('..') === 0) {
        relPath = path.resolve(relPath);
    }
    match = multimatch(relPath, pattern, options).length > 0;
}

This seems to work for me regardless if the input glob is absolute or relative. If you could try dropping that into your gulp-filter's index.js and see if that solves the problem. I'm not sure what sort of side effects this will cause when converting relative paths to absolute.

Update: removed relPath = file.relative; from code. It was accidentally left in there.

sinedied · 2016-04-22T14:38:24Z

I tried your fix, and it effectively solves the issue. 👍

Resolving to absolute path seems dodgy though, maybe the real issue here lies with multimatch not properly matching paths containing .. in the first place? (maybe because of missing an escape of the . character in the regex?)

I'm not sure what's the best fix here...

cb1kenobi · 2016-04-22T15:36:25Z

Well, well... isaacs/minimatch#30 (comment).

sinedied · 2016-04-25T13:07:28Z

Nice :)

Until then, is there any workaround I could use, except for modifying directly the gulp-filter source or sticking to @3.x?

…p-filter#67)

CSS concatenation doesn't work with gulp-filter version 4.0, due to some problems with path resolution, that comes from minimatch - sindresorhus/gulp-filter#67 - isaacs/minimatch#30

cb1kenobi mentioned this issue Apr 22, 2016

make relative path inference configurable #61

Closed

cb1kenobi mentioned this issue Apr 22, 2016

"**/" pattern doesn't match "./" path part isaacs/minimatch#30

Closed

danivek mentioned this issue May 4, 2016

gulp uglify task doesn't work anymore danivek/mean-starter-kit#3

Closed

floreks mentioned this issue May 5, 2016

Revert gulp-filter to version ~3.0.0 kubernetes/dashboard#711

Closed

ghoullier added a commit to cfpio/front-proposals that referenced this issue May 6, 2016

fix(build) Fix build issue due to gulp-filter@4.0.0 (sindresorhus/gul…

c3311b3

…p-filter#67)

wojciechpolak mentioned this issue May 15, 2016

Lock down dependency versions FountainJS/generator-fountain-webapp#25

Closed

peterjuras mentioned this issue Jul 2, 2016

Update all dependencies 🌴 peterjuras/slush-react-express#18

Closed

stefanKuijers mentioned this issue Nov 21, 2016

Matching parent paths #74

Closed

sindresorhus closed this as completed Jan 7, 2017

simonepri mentioned this issue Jan 19, 2017

Version 5.0.0 Changelog #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not working anymore since 4.0.0 #67

Not working anymore since 4.0.0 #67

sinedied commented Apr 21, 2016

nfroidure commented Apr 21, 2016

sinedied commented Apr 21, 2016

nfroidure commented Apr 21, 2016

sinedied commented Apr 22, 2016

nfroidure commented Apr 22, 2016

cb1kenobi commented Apr 22, 2016 •

edited

Loading

sinedied commented Apr 22, 2016

cb1kenobi commented Apr 22, 2016

sinedied commented Apr 25, 2016

Not working anymore since 4.0.0 #67

Not working anymore since 4.0.0 #67

Comments

sinedied commented Apr 21, 2016

nfroidure commented Apr 21, 2016

sinedied commented Apr 21, 2016

nfroidure commented Apr 21, 2016

sinedied commented Apr 22, 2016

nfroidure commented Apr 22, 2016

cb1kenobi commented Apr 22, 2016 • edited Loading

sinedied commented Apr 22, 2016

cb1kenobi commented Apr 22, 2016

sinedied commented Apr 25, 2016

cb1kenobi commented Apr 22, 2016 •

edited

Loading