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

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

Merged
merged 15 commits into from Oct 11, 2022
2 changes: 1 addition & 1 deletion lib/config/default-config.js
Expand Up @@ -52,7 +52,7 @@ exports.defaultConfig = [
{
ignores: [
"**/node_modules/**",
".git/**"
".git/"
]
},

Expand Down
148 changes: 136 additions & 12 deletions lib/eslint/eslint-helpers.js
Expand Up @@ -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
Expand Down Expand Up @@ -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, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting from cwd makes it impossible to lint files outside cwd.

For example, do cd lib and then npx eslint "../*.js".

Expected result is to lint Makefile.js, karma.conf.js, and other files in the root of the eslint/eslint project. It used to work before this change, and it works with eslintrc.

Actual result:

Oops! Something went wrong! :(

ESLint: 8.24.0

No files matching the pattern "../*.js" were found.
Please check for typing mistakes in the pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #16413


deepFilter(entry) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deepFilter determines whether or not we descend into a directory. So, we want to cut off things like node_modules here to avoid the traversal.

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entryFilter determines which entries will show up in the results, so I filter out directories and check patterns here.

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.
Expand Down Expand Up @@ -142,7 +264,7 @@ async function findFiles({
if (stat.isFile()) {
results.push({
filePath,
ignored: configs.isIgnored(filePath)
ignored: configs.isFileIgnored(filePath)
});
}

Expand Down Expand Up @@ -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. */

}

Expand Down
4 changes: 2 additions & 2 deletions package.json
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions tests/fixtures/ignores-directory/eslint.config.js
@@ -0,0 +1,3 @@
module.exports = {
ignores: ["subdir/subsubdir"]
};
Empty file.
Empty file.
5 changes: 5 additions & 0 deletions tests/fixtures/ignores-relative/eslint.config.js
@@ -0,0 +1,5 @@
module.exports = [
{
ignores: ["a.js"]
}
];
Empty file.
3 changes: 3 additions & 0 deletions tests/fixtures/ignores-self/eslint.config.js
@@ -0,0 +1,3 @@
module.exports = {
ignores: ["**/ignores-self/**"]
};
30 changes: 15 additions & 15 deletions tests/lib/cli.js
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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);
});
}
});
Expand Down
62 changes: 52 additions & 10 deletions tests/lib/eslint/flat-eslint.js
Expand Up @@ -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")
Expand Down Expand Up @@ -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);

});


});


Expand Down