feat: Swap out Globby for custom globbing solution. (#16369)

nzakas · web-flow · commit dd0c58f0f34d · 2022-10-11T22:23:01.000+02:00
* feat: Swap out Globby for custom globbing solution. Removes globby due to numerous issues with ignoring files and returning the correct files. Fixes #16354 Fixes #16340 Fixes #16300 * Fix failing test * Add test for 16300 * Make more tests pass * Fix another test * Add another test * Remove old test * Fix cli tests * Bump up Mocha timeout for Node 18 * Revert timeout bump * Manually use stream * clean up comment * Set concurrency back to default * Fix remaining tests * Cleanup requires
diff --git a/lib/config/default-config.js b/lib/config/default-config.js
@@ -52,7 +52,7 @@ exports.defaultConfig = [
     {
         ignores: [
             "**/node_modules/**",
-            ".git/**"
+            ".git/"
         ]
     },
 
diff --git a/lib/eslint/eslint-helpers.js b/lib/eslint/eslint-helpers.js
@@ -13,9 +13,17 @@ const path = require("path");
 const fs = require("fs");
 const fsp = fs.promises;
 const isGlob = require("is-glob");
-const globby = require("globby");
 const hash = require("../cli-engine/hash");
 const minimatch = require("minimatch");
+const util = require("util");
+const fswalk = require("@nodelib/fs.walk");
+
+//-----------------------------------------------------------------------------
+// Fixup references
+//-----------------------------------------------------------------------------
+
+const doFsWalk = util.promisify(fswalk.walk);
+const Minimatch = minimatch.Minimatch;
 
 //-----------------------------------------------------------------------------
 // Errors
@@ -97,6 +105,120 @@ function isGlobPattern(pattern) {
     return isGlob(path.sep === "\\" ? normalizeToPosix(pattern) : pattern);
 }
 
+/**
+ * Searches a directory looking for matching glob patterns. This uses
+ * the config array's logic to determine if a directory or file should
+ * be ignored, so it is consistent with how ignoring works throughout
+ * ESLint.
+ * @param {Object} options The options for this function.
+ * @param {string} options.cwd The directory to search.
+ * @param {Array<string>} options.patterns An array of glob patterns
+ *      to match.
+ * @param {FlatConfigArray} options.configs The config array to use for
+ *      determining what to ignore.
+ * @returns {Promise<Array<string>>} An array of matching file paths
+ *      or an empty array if there are no matches.
+ */
+async function globSearch({ cwd, patterns, configs }) {
+
+    if (patterns.length === 0) {
+        return [];
+    }
+
+    const matchers = patterns.map(pattern => {
+        const patternToUse = path.isAbsolute(pattern)
+            ? normalizeToPosix(path.relative(cwd, pattern))
+            : pattern;
+
+        return new minimatch.Minimatch(patternToUse);
+    });
+
+    return (await doFsWalk(cwd, {
+
+        deepFilter(entry) {
+            const relativePath = normalizeToPosix(path.relative(cwd, entry.path));
+            const matchesPattern = matchers.some(matcher => matcher.match(relativePath, true));
+
+            return matchesPattern && !configs.isDirectoryIgnored(entry.path);
+        },
+        entryFilter(entry) {
+            const relativePath = normalizeToPosix(path.relative(cwd, entry.path));
+
+            // entries may be directories or files so filter out directories
+            if (entry.dirent.isDirectory()) {
+                return false;
+            }
+
+            const matchesPattern = matchers.some(matcher => matcher.match(relativePath));
+
+            return matchesPattern && !configs.isFileIgnored(entry.path);
+        }
+    })).map(entry => entry.path);
+
+}
+
+/**
+ * Determines if a given glob pattern will return any results.
+ * Used primarily to help with useful error messages.
+ * @param {Object} options The options for the function.
+ * @param {string} options.cwd The directory to search.
+ * @param {string} options.pattern A glob pattern to match.
+ * @returns {Promise<boolean>} True if there is a glob match, false if not.
+ */
+function globMatch({ cwd, pattern }) {
+
+    let found = false;
+    const patternToUse = path.isAbsolute(pattern)
+        ? normalizeToPosix(path.relative(cwd, pattern))
+        : pattern;
+
+    const matcher = new Minimatch(patternToUse);
+
+    const fsWalkSettings = {
+
+        deepFilter(entry) {
+            const relativePath = normalizeToPosix(path.relative(cwd, entry.path));
+
+            return !found && matcher.match(relativePath, true);
+        },
+
+        entryFilter(entry) {
+            if (found || entry.dirent.isDirectory()) {
+                return false;
+            }
+
+            const relativePath = normalizeToPosix(path.relative(cwd, entry.path));
+
+            if (matcher.match(relativePath)) {
+                found = true;
+                return true;
+            }
+
+            return false;
+        }
+    };
+
+    return new Promise(resolve => {
+
+        // using a stream so we can exit early because we just need one match
+        const globStream = fswalk.walkStream(cwd, fsWalkSettings);
+
+        globStream.on("data", () => {
+            globStream.destroy();
+            resolve(true);
+        });
+
+        // swallow errors as they're not important here
+        globStream.on("error", () => {});
+
+        globStream.on("end", () => {
+            resolve(false);
+        });
+        globStream.read();
+    });
+
+}
+
 /**
  * Finds all files matching the options specified.
  * @param {Object} args The arguments objects.
@@ -142,7 +264,7 @@ async function findFiles({
             if (stat.isFile()) {
                 results.push({
                     filePath,
-                    ignored: configs.isIgnored(filePath)
+                    ignored: configs.isFileIgnored(filePath)
                 });
             }
 
@@ -226,32 +348,34 @@ async function findFiles({
     });
 
     // note: globbyPatterns can be an empty array
-    const globbyResults = (await globby(globbyPatterns, {
+    const globbyResults = await globSearch({
         cwd,
-        absolute: true,
-        ignore: configs.ignores.filter(matcher => typeof matcher === "string")
-    }));
+        patterns: globbyPatterns,
+        configs,
+        shouldIgnore: true
+    });
 
     // if there are no results, tell the user why
     if (!results.length && !globbyResults.length) {
 
         // try globby without ignoring anything
-        /* eslint-disable no-unreachable-loop -- We want to exit early. */
         for (const globbyPattern of globbyPatterns) {
 
-            /* eslint-disable-next-line no-unused-vars -- Want to exit early. */
-            for await (const filePath of globby.stream(globbyPattern, { cwd, absolute: true })) {
+            // check if there are any matches at all
+            const patternHasMatch = await globMatch({
+                cwd,
+                pattern: globbyPattern
+            });
 
-                // files were found but ignored
+            if (patternHasMatch) {
                 throw new AllFilesIgnoredError(globbyPattern);
             }
 
-            // no files were found
+            // otherwise no files were found
             if (errorOnUnmatchedPattern) {
                 throw new NoFilesFoundError(globbyPattern, globInputPaths);
             }
         }
-        /* eslint-enable no-unreachable-loop -- Go back to normal. */
 
     }
 
diff --git a/package.json b/package.json
@@ -56,8 +56,9 @@
   "bugs": "https://github.com/eslint/eslint/issues/",
   "dependencies": {
     "@eslint/eslintrc": "^1.3.3",
-    "@humanwhocodes/config-array": "^0.10.5",
+    "@humanwhocodes/config-array": "^0.11.2",
     "@humanwhocodes/module-importer": "^1.0.1",
+    "@nodelib/fs.walk": "^1.2.8",
     "ajv": "^6.10.0",
     "chalk": "^4.0.0",
     "cross-spawn": "^7.0.2",
@@ -75,7 +76,6 @@
     "find-up": "^5.0.0",
     "glob-parent": "^6.0.1",
     "globals": "^13.15.0",
-    "globby": "^11.1.0",
     "grapheme-splitter": "^1.0.4",
     "ignore": "^5.2.0",
     "import-fresh": "^3.0.0",
diff --git a/tests/fixtures/ignores-directory/eslint.config.js b/tests/fixtures/ignores-directory/eslint.config.js
@@ -0,0 +1,3 @@
+module.exports = {
+    ignores: ["subdir/subsubdir"]
+};
diff --git a/tests/fixtures/ignores-directory/subdir/subsubdir/a.js b/tests/fixtures/ignores-directory/subdir/subsubdir/a.js
diff --git a/tests/fixtures/ignores-relative/a.js b/tests/fixtures/ignores-relative/a.js
diff --git a/tests/fixtures/ignores-relative/eslint.config.js b/tests/fixtures/ignores-relative/eslint.config.js
@@ -0,0 +1,5 @@
+module.exports = [
+    {
+        ignores: ["a.js"]
+    }
+];
diff --git a/tests/fixtures/ignores-relative/subdir/a.js b/tests/fixtures/ignores-relative/subdir/a.js
diff --git a/tests/fixtures/ignores-self/eslint.config.js b/tests/fixtures/ignores-self/eslint.config.js
@@ -0,0 +1,3 @@
+module.exports = {
+    ignores: ["**/ignores-self/**"]
+};
diff --git a/tests/lib/cli.js b/tests/lib/cli.js
@@ -160,17 +160,6 @@ describe("cli", () => {
             });
         });
 
-        describe("when given a config file and a directory of files", () => {
-            it(`should load and execute without error with configType:${configType}`, async () => {
-                const configPath = getFixturePath("configurations", "semi-error.js");
-                const filePath = getFixturePath("formatters");
-                const code = `--config ${configPath} ${filePath}`;
-                const exitStatus = await cli.execute(code, null, useFlatConfig);
-
-                assert.strictEqual(exitStatus, 0);
-            });
-        });
-
         describe("when there is a local config file", () => {
 
             it(`should load the local config file with configType:${configType}`, async () => {
@@ -460,6 +449,17 @@ describe("cli", () => {
                 process.cwd = originalCwd;
             });
 
+            describe("when given a config file and a directory of files", () => {
+                it(`should load and execute without error with configType:${configType}`, async () => {
+                    const configPath = getFixturePath("configurations", "semi-error.js");
+                    const filePath = getFixturePath("formatters");
+                    const code = `--no-ignore --config ${configPath} ${filePath}`;
+                    const exitStatus = await cli.execute(code, null, useFlatConfig);
+
+                    assert.strictEqual(exitStatus, 0);
+                });
+            });
+
             describe("when executing with global flag", () => {
 
                 it(`should default defined variables to read-only with configType:${configType}`, async () => {
@@ -755,22 +755,22 @@ describe("cli", () => {
                     });
 
                     if (useFlatConfig) {
-                        it("should not ignore files if the pattern is a path to a directory (with trailing slash)", async () => {
+                        it("should ignore files if the pattern is a path to a directory (with trailing slash)", async () => {
                             const filePath = getFixturePath("cli/syntax-error.js");
                             const exit = await cli.execute(`--ignore-pattern cli/ ${filePath}`, null, true);
 
                             // parsing error causes exit code 1
                             assert.isTrue(log.info.called);
-                            assert.strictEqual(exit, 1);
+                            assert.strictEqual(exit, 0);
                         });
 
-                        it("should not ignore files if the pattern is a path to a directory (without trailing slash)", async () => {
+                        it("should ignore files if the pattern is a path to a directory (without trailing slash)", async () => {
                             const filePath = getFixturePath("cli/syntax-error.js");
                             const exit = await cli.execute(`--ignore-pattern cli ${filePath}`, null, true);
 
                             // parsing error causes exit code 1
                             assert.isTrue(log.info.called);
-                            assert.strictEqual(exit, 1);
+                            assert.strictEqual(exit, 0);
                         });
                     }
                 });
diff --git a/tests/lib/eslint/flat-eslint.js b/tests/lib/eslint/flat-eslint.js
@@ -978,16 +978,6 @@ describe("FlatESLint", () => {
                 }, /All files matched by 'node_modules\/\*\*\/\*\.js' are ignored\./u);
             });
 
-            it("should throw an error when given a directory with all eslint excluded files in the directory", async () => {
-                eslint = new FlatESLint({
-                    overrideConfigFile: getFixturePath("eslint.config_with_ignores.js")
-                });
-
-                await assert.rejects(async () => {
-                    await eslint.lintFiles([getFixturePath("./cli-engine/")]);
-                }, /All files matched by '.*?cli-engine[\\/]\*\*[\\/]\*\.js' are ignored/u);
-            });
-
             it("should throw an error when all given files are ignored", async () => {
                 eslint = new FlatESLint({
                     overrideConfigFile: getFixturePath("eslint.config_with_ignores.js")
@@ -1099,6 +1089,58 @@ describe("FlatESLint", () => {
                 assert.strictEqual(results[0].messages[1].severity, 2);
                 assert.strictEqual(results[0].suppressedMessages.length, 0);
             });
+
+            // https://github.com/eslint/eslint/issues/16300
+            it("should process ignore patterns relative to basePath not cwd", async () => {
+                eslint = new FlatESLint({
+                    cwd: getFixturePath("ignores-relative/subdir")
+                });
+                const results = await eslint.lintFiles(["**/*.js"]);
+
+                assert.strictEqual(results.length, 1);
+                assert.strictEqual(results[0].filePath, getFixturePath("ignores-relative/subdir/a.js"));
+            });
+
+
+            // https://github.com/eslint/eslint/issues/16354
+            it("should skip subdirectory files when ignore pattern matches subdirectory", async () => {
+                eslint = new FlatESLint({
+                    cwd: getFixturePath("ignores-directory")
+                });
+
+                await assert.rejects(async () => {
+                    await eslint.lintFiles(["subdir/**"]);
+                }, /All files matched by 'subdir\/\*\*' are ignored\./u);
+
+                await assert.rejects(async () => {
+                    await eslint.lintFiles(["subdir/subsubdir/**"]);
+                }, /All files matched by 'subdir\/subsubdir\/\*\*' are ignored\./u);
+
+                const results = await eslint.lintFiles(["subdir/subsubdir/a.js"]);
+
+                assert.strictEqual(results.length, 1);
+                assert.strictEqual(results[0].filePath, getFixturePath("ignores-directory/subdir/subsubdir/a.js"));
+                assert.strictEqual(results[0].warningCount, 1);
+                assert(results[0].messages[0].message.startsWith("File ignored"), "Should contain file ignored warning");
+
+            });
+
+            // https://github.com/eslint/eslint/issues/16340
+            it("should lint files even when cwd directory name matches ignores pattern", async () => {
+                eslint = new FlatESLint({
+                    cwd: getFixturePath("ignores-self")
+                });
+
+                const results = await eslint.lintFiles(["*.js"]);
+
+                assert.strictEqual(results.length, 1);
+                assert.strictEqual(results[0].filePath, getFixturePath("ignores-self/eslint.config.js"));
+                assert.strictEqual(results[0].errorCount, 0);
+                assert.strictEqual(results[0].warningCount, 0);
+
+            });
+
+
         });
 
 

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ exports.defaultConfig = [`
`52`	`52`	`{`
`53`	`53`	`ignores: [`
`54`	`54`	`"/node_modules/",`
`55`		`- ".git/**"`
	`55`	`+ ".git/"`
`56`	`56`	`]`
`57`	`57`	`},`
`58`	`58`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ ignores: ["subdir/subsubdir"]`
	`3`	`+};`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +module.exports = [
 +    {
 +        ignores: ["a.js"]
 +    }
 +];
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ ignores: ["/ignores-self/"]`
	`3`	`+};`