Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect deep filtering #78

Closed
mrmlnc opened this issue Mar 1, 2018 · 3 comments
Closed

Incorrect deep filtering #78

mrmlnc opened this issue Mar 1, 2018 · 3 comments
Assignees

Comments

@mrmlnc
Copy link
Owner

mrmlnc commented Mar 1, 2018

Environment

  • OS Version: 10.13.3 (macOS)
  • Node.js Version: 9.5.0

Actual behaviour

Where we set the cwd option, the packages path is passed to the depth filter and this is matched to the * negative pattern. After that, we do not read this directory.

If you pass patterns without cwd option (like app/**/node_modules/**), then we work with app/packages and it's works fine.

[]

Expected behaviour

[
  'packages/bar/node_modules',
  'packages/bar/node_modules/baz',
  'packages/bar/node_modules/baz/c.json',
  'packages/bar/node_modules/baz/node_modules',
  'packages/bar/node_modules/baz/node_modules/d.json',
  'packages/bar/node_modules/baz/node_modules/something'
]

Steps to reproduce

app
└── packages
    ├── bar
    │   ├── b.json
    │   └── node_modules
    │       └── baz
    │           ├── c.json
    │           └── node_modules
    │               ├── d.json
    │               └── something
    └── foo
        └── a.json

Code sample

const globby = require('globby');
const fg = require('fast-glob');
const fgNext = require('../../OpenSource/fast-glob');

const entries1 = globby.sync(['**/node_modules/**', '!*'], { cwd: 'app', nodir: false });
const entries2 = fg.sync(['**/node_modules/**', '!*'], { cwd: 'app', onlyFiles: false });
const entries3 = fgNext.sync(['**/node_modules/**', '!*'], { cwd: 'app', onlyFiles: false });

console.log('GLOBBY@7', entries1); // Expected behaviour 
console.log('FAST-GLOB@2', entries2); // []
console.log('FAST-GLOB@NEXT', entries3); // []
@mrmlnc
Copy link
Owner Author

mrmlnc commented Mar 1, 2018

I'm not sure that is a bug...

@jonschlinkert
Copy link

I'm not sure if this is related, but I just found an inconsistency in how node-glob matches paths versus how minimatch matches paths. In node-glob, when a pattern ends with /**, it treats the pattern like {,/**}. But minimatch (and micromatch, and I believe bash) do not follow that behavior.

However, it does seem to make more sense in the context of globbing, since the glob is typically matching against the relative part of a (longer) file path.

Maybe try doing something lik this on the pattern before passing it to micromatch.

if (pattern.slice(-3) === '/**') {
  pattern =  pattern.slice(0, -3) + '{,/**}';
}

It might also be worthwhile comparing some tests to output from node-glob. Not that node-glob is always correct, but it might be informative.

@mrmlnc
Copy link
Owner Author

mrmlnc commented Mar 9, 2018

Hello, @jonschlinkert,

First, thanks to a great tip. Yeap, it's a good idea, but this does not apply to the current problem.

Unfortunately, this problem also related to issue #80. We exclude directories from reading even if we don't want this. This is related to deep filter:

if (patternUtils.matchAny(entry.path, negativeRe)) {

Here we try to exclude the directory from reading using negative patterns. For example, when we provide the * pattern as negative pattern – he will be match to packages directory. Then we never read packages directory.

I also added more than 300 smoke tests. For more details see #81.

I want to close this issue in favour of #80.

@mrmlnc mrmlnc closed this as completed Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants