-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
||
return (await doFsWalk(cwd, { | ||
|
||
deepFilter(entry) { |
There was a problem hiding this comment.
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.
return !configs.isDirectoryIgnored(entry.path); | ||
}, | ||
|
||
entryFilter(entry) { |
There was a problem hiding this comment.
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.
tests/lib/eslint/flat-eslint.js
Outdated
@@ -985,6 985,7 @@ describe("FlatESLint", () => { | |||
|
|||
it("should throw an error when given a directory with all eslint excluded files in the directory", async () => { | |||
eslint = new FlatESLint({ | |||
cwd: getFixturePath(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are removing ignorePath
, not a big deal to fix this test up to make sure it works correctly.
I'm not sure why timeouts happen only on Node 18, but what's common for all failing tests is that only flat config tests are failing and they're all testing behavior with unmatched patterns, so perhaps this indicates that handling unmatched patterns should be optimized in |
Ah good call! I've optimized the search so that it's excluding more directories by default and that's making some of the tests run noticeably faster locally. I'll look more deeply into the unmatched branch. |
For example, this test: Lines 494 to 507 in 720ff75
In eslintrc mode, it traverses only In flat config mode, it traverses the whole Maybe we could keep the search logic from eslintrc? eslintrc (ESLint class & FileEnumerator):
flat config (FlatESLint class & eslint-helpers):
|
So interestingly when I log through all off the filtering, it appears that the traversal completes and then just hangs. Whatever cleanup is being done, either from the package or from Node.js, is clearly getting stuck. Basically, if At this point I'm out of ideas aside from manually creating my own filesystem walker. |
Maybe |
After I napped I also realized that It looks like it's not an issue with streams per se, but rather an issue with using async interation via the |
Okay, now other errors are happening, that's progress. :) That's all for today. |
Okay, I believe this should work now. Undoubtedly there are perf improvements that can be made, but I think those should be handled in another PR. From my local testing, this code looks pretty close to what we had with |
return new minimatch.Minimatch(patternToUse); | ||
}); | ||
|
||
return (await doFsWalk(cwd, { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #16413
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File should be ignored if any of the ancestor directories are ignored.
For example:
.
├── foo/
│ └── bar/
│ └── a.js
└── eslint.config.js
// eslint.config.js
module.exports = [{
ignores: ["foo"]
}];
eslint foo/bar/a.js -f tap
Expected result:
TAP version 13
1..1
ok 1 - D:\tmp\eslint-ignore-patch\foo\bar\a.js
---
message: >-
File ignored because of a matching ignore pattern. Use "--no-ignore" to
override.
severity: warning
data:
line: 0
column: 0
ruleId: ''
...
Actual result:
TAP version 13
1..1
ok 1 - D:\tmp\eslint-ignore-patch\foo\bar\a.js
(with globs, it works as expected because traversal doesn't descend into ignored foo
)
Note: eslint "foo/**/*.js"
works as expected because it doesn't enter ignored foo
. But cd foo && eslint "bar/*.js"
doesn't work as expected as it enters foo/bar
although that directory should be ignored because it's in an ignored directory foo
.
Edit: #16414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to unignore a directory.
For example:
.
├── foo/
│ └── bar/
│ └── a.js
└── eslint.config.js
// eslint.config.js
module.exports = [{
ignores: ["foo/*", "!foo/bar"]
}];
eslint "foo/**/*.js" -f tap
Expected result:
TAP version 13
1..1
ok 1 - D:\tmp\eslint-ignore-patch\foo\bar\a.js
Actual result:
Oops! Something went wrong! :(
ESLint: 8.24.0
You are linting "foo/**/*.js", but all of the files matching the glob pattern "foo/**/*.js" are ignored.
If you don't want to lint these files, remove the pattern "foo/**/*.js" from the list of arguments passed to ESLint.
If you do want to lint these files, try the following solutions:
* Check your .eslintignore file, or the eslintIgnore property in package.json, to ensure that the files are not configured to be ignored.
* Explicitly list the files from this glob that you'd like to lint on the command-line, rather than providing a glob as an argument.
Edit: #16415
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want .gitignore-like behavior, then the last pattern that matches the file should determine whether the file is ignored (unless the file is under an ignored directory; in that case, the file is ignored).
For example:
.
├── a.js
└── eslint.config.js
// eslint.config.js
module.exports = [{
ignores: [
"*.js",
"!a*.js",
"a.js"
]
}];
eslint a.js -f tap
Expected result:
TAP version 13
1..1
ok 1 - D:\tmp\eslint-ignore-patch\a.js
---
message: >-
File ignored because of a matching ignore pattern. Use "--no-ignore" to
override.
severity: warning
data:
line: 0
column: 0
ruleId: ''
...
Actual result:
TAP version 13
1..1
ok 1 - D:\tmp\eslint-ignore-patch\a.js
Edit: #16416
Thanks for the detailed review. It looks like each of the problems you've mentioned actually already exists with the globby implementation (on The three bugs this PR fixes are more happy side effects of swapping out globby rather than intentional. :) What do you think? |
I was mostly concerned about #16369 (comment), but that's not a common use case and we'll have time to fix that now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I'll open issues about the problems mentioned here.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.25.0` -> `8.26.0`](https://renovatebot.com/diffs/npm/eslint/8.25.0/8.26.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.26.0`](https://github.com/eslint/eslint/releases/tag/v8.26.0) [Compare Source](eslint/eslint@v8.25.0...v8.26.0) #### Features - [`4715787`](eslint/eslint@4715787) feat: check `Object.create()` in getter-return ([#​16420](eslint/eslint#16420)) (Yuki Hirasawa) - [`28d1902`](eslint/eslint@28d1902) feat: `no-implicit-globals` supports `exported` block comment ([#​16343](eslint/eslint#16343)) (Sosuke Suzuki) - [`e940be7`](eslint/eslint@e940be7) feat: Use ESLINT_USE_FLAT_CONFIG environment variable for flat config ([#​16356](eslint/eslint#16356)) (Tomer Aberbach) - [`dd0c58f`](eslint/eslint@dd0c58f) feat: Swap out Globby for custom globbing solution. ([#​16369](eslint/eslint#16369)) (Nicholas C. Zakas) #### Bug Fixes - [`df77409`](eslint/eslint@df77409) fix: use `baseConfig` constructor option in FlatESLint ([#​16432](eslint/eslint#16432)) (Milos Djermanovic) - [`33668ee`](eslint/eslint@33668ee) fix: Ensure that glob patterns are matched correctly. ([#​16449](eslint/eslint#16449)) (Nicholas C. Zakas) - [`740b208`](eslint/eslint@740b208) fix: ignore messages without a `ruleId` in `getRulesMetaForResults` ([#​16409](eslint/eslint#16409)) (Francesco Trotta) - [`8f9759e`](eslint/eslint@8f9759e) fix: `--ignore-pattern` in flat config mode should be relative to `cwd` ([#​16425](eslint/eslint#16425)) (Milos Djermanovic) - [`325ad37`](eslint/eslint@325ad37) fix: make `getRulesMetaForResults` return a plain object in trivial case ([#​16438](eslint/eslint#16438)) (Francesco Trotta) - [`a2810bc`](eslint/eslint@a2810bc) fix: Ensure that directories can be unignored. ([#​16436](eslint/eslint#16436)) (Nicholas C. Zakas) - [`35916ad`](eslint/eslint@35916ad) fix: Ensure unignore and reignore work correctly in flat config. ([#​16422](eslint/eslint#16422)) (Nicholas C. Zakas) #### Documentation - [`651649b`](eslint/eslint@651649b) docs: Core concepts page ([#​16399](eslint/eslint#16399)) (Ben Perlmutter) - [`631cf72`](eslint/eslint@631cf72) docs: note --ignore-path not supported with flat config ([#​16434](eslint/eslint#16434)) (Andy Edwards) - [`1692840`](eslint/eslint@1692840) docs: fix syntax in examples for new config files ([#​16427](eslint/eslint#16427)) (Milos Djermanovic) - [`d336cfc`](eslint/eslint@d336cfc) docs: Document extending plugin with new config ([#​16394](eslint/eslint#16394)) (Ben Perlmutter) #### Chores - [`e917a9a`](eslint/eslint@e917a9a) ci: add node v19 ([#​16443](eslint/eslint#16443)) (Koichi ITO) - [`4b70b91`](eslint/eslint@4b70b91) chore: Add VS Code issues link ([#​16423](eslint/eslint#16423)) (Nicholas C. Zakas) - [`232d291`](eslint/eslint@232d291) chore: suppress a Node.js deprecation warning ([#​16398](eslint/eslint#16398)) (Koichi ITO) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuNSIsInVwZGF0ZWRJblZlciI6IjMyLjI0MS4xMCJ9--> Co-authored-by: cabr2-bot <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1599 Reviewed-by: Epsilon_02 <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>
Found one regression: |
Removes globby due to numerous issues with ignoring files and returning the correct files.
Fixes #16354
Fixes #16340
Fixes #16300
Prerequisites checklist
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)
I removed
globby
and replaced it with a custom solution built upon@nodelib/fs.walk
, which was used byfast-glob
to walk the directory and look for matching filenames. Instead of forcingglobby
to reinterpret ignores, this solution usesFlatConfigArray#isFileIgnored()
as part of the traversal so it exactly mirrors the ignoring behavior inside of a config array. This should fix a lot of the outstanding issues related to ignoring certain files.Is there anything you'd like reviewers to focus on?
If you've seen weird behavior, please check out this branch and try it out to see if it fixes the issue. I'm still adding tests to this PR to make sure everything is fixed.
Also, I marked as "feat" because it seemed worthy of a minor release, but totally fine to mark as "fix" if others feel differently.