feat: Implement alternate config lookup #18742

nzakas · 2024-08-01T16:06:50Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Implemented the design from eslint/rfcs#120:

New unstable_config_lookup_from_file flag
Refactored config lookup into ConfigLoader and LegacyConfigLoader
Refactored findFiles() to use new config scheme.
Updated documentation
Updated tests

Fixes #18385

Is there anything you'd like reviewers to focus on?

Marking as a draft because I'd like us to merge #18134 first.

Note that I did not remove the config loading logic from the ESLint class, I just duplicated it in the config-loader.js file in order to have an easier merge once #18134 is merged. (This accounts for the linting error in eslint.js)

netlify · 2024-08-01T16:07:10Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`e16493b`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/66e1dc315768ad000857ec8b

fasttime · 2024-08-20T20:06:37Z

There are merge conflicts now.

nzakas · 2024-08-21T14:11:03Z

I'll look at those, but if you can please review ASAP I'd appreciate it. This is a complicated change and there will continue to be more conflicts until we get this merged. It's already been three weeks since I posted it.

docs/src/use/configure/configuration-files.md

lib/cli.js

lib/config/config-loader.js

tests/lib/eslint/eslint.js

lib/eslint/eslint.js

mdjermanovic · 2024-08-26T14:29:27Z

lib/eslint/eslint-helpers.js

+ const configs = rawPatterns[matchIndex] !== "." && !entry.path.includes("/")
+ ? void 0
+ : await configLoader.loadConfigArrayForDirectory(absolutePath);


Special-casing only immediate children would cause inconsistent behavior, in my opinion. For example, if the root config file ignores foo and bar/baz directories, and both of them have their own config files, eslint foo would work, while eslint bar/baz wouldn't.

Sorry, I'm having trouble following what it is you're describing. Can you put together a test case that will fail?

And do you have other suggestions for how you might approach this? It turns out getting the exact behavior in the RFC is pretty difficult and this was the only approach I found that passed all of the tests we had described there.

Here's a test case: bb439b1

One approach could be to search per each pattern and start loading configs from globParent() of the pattern. That's how eslintrc's FileEnumerator works. A downside is performance as the same directories might be traversed multiple times.

Another approach could be to keep searching with multiple patterns at once but check if the directory path is a part of the globParent() of at least one of the patterns, and in that case enter the directory even if it's ignored by a config in a parent directory. Though, this seems complicated as only those patterns should match in the directory.

Here's another test that I think should pass but is failing with the current iteration of this PR: 7e21937

Another possible approach: perhaps we could always group patterns by their globParent() and start loading configs from that path.

fasttime · 2024-08-27T13:36:21Z

There is a new merge conflict now and the tests were not run.

nzakas · 2024-08-28T15:20:50Z

Added in @mdjermanovic's test and removed the check for immediate children. Somehow, this caused all tests to pass. 😄 Maybe I'm missing something, though.

fasttime · 2024-09-02T08:04:52Z

LGTM, thanks! Leaving open for @mdjermanovic to complete his review.

mdjermanovic · 2024-09-03T07:57:07Z

lib/eslint/eslint-helpers.js

+ const configs = rawPatterns[matchIndex] !== "."
+ ? void 0
+ : await configLoader.loadConfigArrayForDirectory(absolutePath);
+
+ return !configs?.isDirectoryIgnored(absolutePath);


Now, on this branch, a pattern like **/*.js would traverse all directories, which is not what we want, I believe.

To reproduce, add console.log(entry.path); at the beginning of async directoryFilter(entry) and run npx eslint "**/*.js".

.cache .git .git/hooks .git/info .git/logs .git/logs/refs .git/logs/refs/heads .git/logs/refs/remotes .git/logs/refs/remotes/origin .git/logs/refs/remotes/origin/chore .git/logs/refs/remotes/origin/docs .git/logs/refs/remotes/origin/fix .git/logs/refs/remotes/origin/renovate .git/objects .git/objects/00 .git/objects/01 .git/objects/02 .git/objects/03 .git/objects/04 .git/objects/05 .git/objects/06 .git/objects/07 .git/objects/08 .git/objects/09 .git/objects/0a .git/objects/0b .git/objects/0c .git/objects/0d .git/objects/0e ... (~10,000 directories)

Hmmm yeah, that's not right. I'll take a look.

I'm not able to reproduce this (Windows 10). Are you sure you're using the right command (npx eslint would download ESLint if you don't have it locally?). Here's what I'm getting:

$ node ./bin/eslint.js "**/*.js" (node:5064) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files (Use `node --trace-warnings ...` to show where the warning was created) .cache .git .github .nyc_output .trunk .vscode bin build conf coverage docs lib messages node_modules packages templates tests tmp tools Oops! Something went wrong! :( ESLint: 9.9.1 No files matching the pattern "**C:/Program Files/Git/*.js" were found. Please check for typing mistakes in the pattern.

Oops! Something went wrong! :(

ESLint: 9.9.1

No files matching the pattern "**C:/Program Files/Git/*.js" were found.
Please check for typing mistakes in the pattern.

That "**C:/Program Files/Git/*.js" looks weird, I'm not sure where it did come from when running $ node ./bin/eslint.js "**/*.js" (maybe your shell is doing some expansion despite the quotes?).

I tried again, on this branch, with a clean npm install, and in cmd.exe which doesn't expand globs. I also added console.log({ patterns }); at the beginning of async function globSearch to make sure that the pattern is passed through.

C:\projects\eslint>node ./bin/eslint.js "**/*.js" (node:7136) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files (Use `node --trace-warnings ...` to show where the warning was created) { patterns: [ 'C:\\projects\\eslint\\**\\*.js' ] } .cache .git .git/hooks .git/info .git/logs .git/logs/refs .git/logs/refs/heads .git/logs/refs/remotes .git/logs/refs/remotes/origin .git/logs/refs/remotes/origin/chore .git/logs/refs/remotes/origin/docs .git/logs/refs/remotes/origin/fix .git/logs/refs/remotes/origin/renovate .git/objects .git/objects/00 .git/objects/01 .git/objects/02 .git/objects/03 .git/objects/04 .git/objects/05 .git/objects/06 .git/objects/07 .git/objects/08 .git/objects/09 .git/objects/0a .git/objects/0b .git/objects/0c ...

Okay, I can see this in cmd but not in Git Bash. It looks like Git Bash is representing the path in Windows differently:

patterns: [ 'C:\\Users\\nzaka\\projects\\eslint\\eslint\\**C:\\Program Files\\Git\\*.js' ]

I'll take a look at that, too.

So it looks like Git for Windows is expanding the pattern even if it's quoted.
git-for-windows/git#5131

mdjermanovic · 2024-09-03T09:58:42Z

tests/lib/eslint/eslint.js

+ }
+ }
+ });
+ const failFilePath = fs.realpathSync(ge


What is the expected behavior in this case when cwd has some files in it? When I add a file (any file, e.g.. file with no extension) directly into subdir-only-config, the test fails with "Error: Could not find config file.".

I don't think we explicitly defined this behavior. Technically, throwing an error is correct because our logic says "for a given file, if you can't find a config file that tells you what to do with it, throw an error."

I'm not sure how common it would be for someone to run eslint . in a directory without a config file. This might be a case where we leave this behavior as-is and see what feedback we get. Otherwise, I'd be back to checking "if pattern is . and file is an immediate child of ., then ignore it".

I agree that throwing an error when a pattern matches a file for which a config file cannot be found is the correct behavior.

nzakas · 2024-09-10T20:29:23Z

@mdjermanovic I've been fighting with this for the last three days and have made zero progress. Do you want to take a stab at it?

nzakas · 2024-09-11T15:24:01Z

Hold that thought! I might have figured it out.

fixes #18385

Co-authored-by: 唯然 <[email protected]>

Co-authored-by: Francesco Trotta <[email protected]>

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Aug 1, 2024

github-actions bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels Aug 1, 2024

nzakas force-pushed the issue18385c branch from 5ff8b44 to 90ba595 Compare August 9, 2024 19:02

nzakas marked this pull request as ready for review August 9, 2024 19:08

nzakas requested a review from a team as a code owner August 9, 2024 19:08

nzakas mentioned this pull request Aug 12, 2024

Bug: Config array "./src" and "src" aren't treated as the same #18757

Open

1 task

nzakas force-pushed the issue18385c branch from 4b5b937 to b187a17 Compare August 12, 2024 19:29

fasttime added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 20, 2024

nzakas force-pushed the issue18385c branch from ab566d9 to 3a5b6db Compare August 21, 2024 14:34

aladdin-add self-requested a review August 22, 2024 05:39

fasttime reviewed Aug 23, 2024

View reviewed changes

docs/src/use/configure/configuration-files.md Outdated Show resolved Hide resolved

lib/cli.js Show resolved Hide resolved

aladdin-add reviewed Aug 26, 2024

View reviewed changes

lib/config/config-loader.js Outdated Show resolved Hide resolved

lib/config/config-loader.js Outdated Show resolved Hide resolved

fasttime reviewed Aug 26, 2024

View reviewed changes

lib/config/config-loader.js Show resolved Hide resolved

tests/lib/eslint/eslint.js Show resolved Hide resolved

mdjermanovic reviewed Aug 26, 2024

View reviewed changes

lib/eslint/eslint.js Show resolved Hide resolved

mdjermanovic reviewed Aug 26, 2024

View reviewed changes

nzakas force-pushed the issue18385c branch from b767e37 to 1a2ec20 Compare August 27, 2024 14:01

nzakas force-pushed the issue18385c branch from 6c079cc to d58d20b Compare August 29, 2024 15:03

fasttime previously approved these changes Sep 2, 2024

View reviewed changes

mdjermanovic reviewed Sep 3, 2024

View reviewed changes

mdjermanovic mentioned this pull request Sep 4, 2024

refactor: Extract processor logic into ProcessorService #18818

Open

1 task

Narretz mentioned this pull request Sep 8, 2024

Change Request: Make it easier to inherit flat configs from the repo root #18385

Open

1 task

nzakas dismissed fasttime’s stale review via 678ff02 September 11, 2024 16:08

nzakas and others added 25 commits September 11, 2024 12:14

feat: Look up config file from file being linted

a70221b

fixes #18385

Fix ESLint class flag lookup

75c636f

Fix up tests for config lookup

4796cc7

Add documentation for new config lookup flag

cd144dc

Clean up directory searching behavior

9967b90

Read config files from starting from parent

f8a73ef

Cover edge cases

49eb886

Fix failing test

201d915

Fix test and linting errors

262ff52

Remove .only from tests

6899f31

Fix failing test

d9d312e

Incorporate TypeScript config files

73675e4

Fix lint errors

f5e9b74

Fix docs link

eca81ea

Fix broken link

2a4bcde

Fix conflicts

e1f47c6

Update lib/config/config-loader.js

a979cb3

Co-authored-by: 唯然 <[email protected]>

Update lib/config/config-loader.js

788ee01

Co-authored-by: 唯然 <[email protected]>

Update docs/src/use/configure/configuration-files.md

d4f2f27

Co-authored-by: Francesco Trotta <[email protected]>

Add comment about CLI overrideConfig

fff442b

Remove duplicate code

1e0076e

Fix error

38a9b2d

add test with a deep subdir

9d9166e

Remove immediate child check

92254ba

Fix edge cases

2163eeb

nzakas force-pushed the issue18385c branch from 678ff02 to 2163eeb Compare September 11, 2024 16:15

Clean up logic

e16493b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement alternate config lookup #18742

feat: Implement alternate config lookup #18742

nzakas commented Aug 1, 2024 •

edited

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

fasttime commented Aug 20, 2024

nzakas commented Aug 21, 2024

mdjermanovic Aug 26, 2024

nzakas Aug 26, 2024

mdjermanovic Aug 27, 2024

mdjermanovic Aug 27, 2024 •

edited

Loading

mdjermanovic Sep 3, 2024

mdjermanovic Sep 3, 2024

fasttime commented Aug 27, 2024

nzakas commented Aug 28, 2024

fasttime commented Sep 2, 2024

mdjermanovic Sep 3, 2024 •

edited

Loading

nzakas Sep 3, 2024

nzakas Sep 3, 2024

mdjermanovic Sep 4, 2024

nzakas Sep 4, 2024

nzakas Sep 9, 2024

mdjermanovic Sep 3, 2024

nzakas Sep 3, 2024

mdjermanovic Sep 4, 2024

nzakas commented Sep 10, 2024

nzakas commented Sep 11, 2024

feat: Implement alternate config lookup #18742

Are you sure you want to change the base?

feat: Implement alternate config lookup #18742

Conversation

nzakas commented Aug 1, 2024 • edited Loading

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Aug 1, 2024 • edited Loading

✅ Deploy Preview for docs-eslint canceled.

fasttime commented Aug 20, 2024

nzakas commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented Aug 27, 2024

nzakas commented Aug 28, 2024

fasttime commented Sep 2, 2024

mdjermanovic Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Sep 10, 2024

nzakas commented Sep 11, 2024

nzakas commented Aug 1, 2024 •

edited

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

mdjermanovic Aug 27, 2024 •

edited

Loading

mdjermanovic Sep 3, 2024 •

edited

Loading