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
Merged

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

merged 15 commits into from Oct 11, 2022

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Sep 30, 2022

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 by fast-glob to walk the directory and look for matching filenames. Instead of forcing globby to reinterpret ignores, this solution uses FlatConfigArray#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.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Sep 30, 2022
@netlify
Copy link

netlify bot commented Sep 30, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 5e699df
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6344795953957200083449d0
😎 Deploy Preview https://deploy-preview-16369--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.


return (await doFsWalk(cwd, {

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.

return !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.

@@ -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(),
Copy link
Member Author

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.

@mdjermanovic
Copy link
Member

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 eslint-helpers.js.

@nzakas
Copy link
Member Author

nzakas commented Oct 4, 2022

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.

@mdjermanovic
Copy link
Member

For example, this test:

eslint/tests/lib/cli.js

Lines 494 to 507 in 720ff75

it(`should throw an error on unmatched glob pattern with configType:${configType}`, async () => {
let filePath = getFixturePath("unmatched-patterns");
const globPattern = "unmatched*.js";
if (useFlatConfig) {
filePath = filePath.replace(/\\/gu, "/");
}
await stdAssert.rejects(async () => {
await cli.execute(`"${filePath}/${globPattern}"`, null, useFlatConfig);
}, new Error(`No files matching '${filePath}/${globPattern}' were found.`));
});
});

In eslintrc mode, it traverses only fixtures/unmatched-patterns directory.

In flat config mode, it traverses the whole fixtures, and once again the whole fixtures.

Maybe we could keep the search logic from eslintrc?

eslintrc (ESLint class & FileEnumerator):

  • Runs multiple searches, one for each pattern. A downside is that it may traverse the same directories multiple times if the patterns have intersections, but that should be rare.
  • Starts search from glob parent directory.
  • Determines whether the search matches ignored files during the search.

flat config (FlatESLint class & eslint-helpers):

  • Runs one search with all patterns at once. A downside is that it doesn't know if each of the patterns has matched something (Bug: [new config system] unmatched individual patterns are not reported #16275).
  • Always starts search from cwd. It's less optimal than starting from the glob parent, and patterns like ../*.js don't work.
  • If search returned nothing, runs another search to see if it matches only ignored files.

@nzakas
Copy link
Member Author

nzakas commented Oct 4, 2022

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 entryFilter never returns true, we get this behavior. I can't figure out where it's hanging as it seems to be in between callbacks and if I try to use the debugger it steps through just fine.

At this point I'm out of ideas aside from manually creating my own filesystem walker. ☹️

@mdjermanovic
Copy link
Member

Maybe fswalk.walkStream hangs, would it work if we use doFsWalk instead? (we're forcing an early exit with found = true anyways).

@nzakas
Copy link
Member Author

nzakas commented Oct 4, 2022

After I napped I also realized that walkStream was the likely culprit. 👍

It looks like it's not an issue with streams per se, but rather an issue with using async interation via the for await-of loop. It seems like the stream isn't properly closed when we exit early from the loop. If I use a stream without async iteration then it works fine.

@nzakas
Copy link
Member Author

nzakas commented Oct 4, 2022

Okay, now other errors are happening, that's progress. :) That's all for today.

lib/eslint/eslint-helpers.js Outdated Show resolved Hide resolved
@nzakas nzakas marked this pull request as ready for review October 6, 2022 21:26
@nzakas
Copy link
Member Author

nzakas commented Oct 6, 2022

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 globby.

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

Copy link
Member

@mdjermanovic mdjermanovic left a 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

Copy link
Member

@mdjermanovic mdjermanovic left a 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

Copy link
Member

@mdjermanovic mdjermanovic left a 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

@nzakas
Copy link
Member Author

nzakas commented Oct 10, 2022

Thanks for the detailed review. It looks like each of the problems you've mentioned actually already exists with the globby implementation (on main), and because this PR was just intended to replace globby and not solve every bug in the current implementation, I'd like to suggest that we merge this as-is and ask you to open separate issues for each of those problems.

The three bugs this PR fixes are more happy side effects of swapping out globby rather than intentional. :)

What do you think?

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 11, 2022
@mdjermanovic
Copy link
Member

I was mostly concerned about #16369 (comment), but that's not a common use case and we'll have time to fix that now.

Copy link
Member

@mdjermanovic mdjermanovic left a 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.

@mdjermanovic mdjermanovic merged commit dd0c58f into main Oct 11, 2022
21 checks passed
@mdjermanovic mdjermanovic deleted the fswalk branch October 11, 2022 20:23
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 24, 2022
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 ([#&#8203;16420](eslint/eslint#16420)) (Yuki Hirasawa)
-   [`28d1902`](eslint/eslint@28d1902) feat: `no-implicit-globals` supports `exported` block comment ([#&#8203;16343](eslint/eslint#16343)) (Sosuke Suzuki)
-   [`e940be7`](eslint/eslint@e940be7) feat: Use ESLINT_USE_FLAT_CONFIG environment variable for flat config ([#&#8203;16356](eslint/eslint#16356)) (Tomer Aberbach)
-   [`dd0c58f`](eslint/eslint@dd0c58f) feat: Swap out Globby for custom globbing solution. ([#&#8203;16369](eslint/eslint#16369)) (Nicholas C. Zakas)

#### Bug Fixes

-   [`df77409`](eslint/eslint@df77409) fix: use `baseConfig` constructor option in FlatESLint ([#&#8203;16432](eslint/eslint#16432)) (Milos Djermanovic)
-   [`33668ee`](eslint/eslint@33668ee) fix: Ensure that glob patterns are matched correctly. ([#&#8203;16449](eslint/eslint#16449)) (Nicholas C. Zakas)
-   [`740b208`](eslint/eslint@740b208) fix: ignore messages without a `ruleId` in `getRulesMetaForResults` ([#&#8203;16409](eslint/eslint#16409)) (Francesco Trotta)
-   [`8f9759e`](eslint/eslint@8f9759e) fix: `--ignore-pattern` in flat config mode should be relative to `cwd` ([#&#8203;16425](eslint/eslint#16425)) (Milos Djermanovic)
-   [`325ad37`](eslint/eslint@325ad37) fix: make `getRulesMetaForResults` return a plain object in trivial case ([#&#8203;16438](eslint/eslint#16438)) (Francesco Trotta)
-   [`a2810bc`](eslint/eslint@a2810bc) fix: Ensure that directories can be unignored. ([#&#8203;16436](eslint/eslint#16436)) (Nicholas C. Zakas)
-   [`35916ad`](eslint/eslint@35916ad) fix: Ensure unignore and reignore work correctly in flat config. ([#&#8203;16422](eslint/eslint#16422)) (Nicholas C. Zakas)

#### Documentation

-   [`651649b`](eslint/eslint@651649b) docs: Core concepts page ([#&#8203;16399](eslint/eslint#16399)) (Ben Perlmutter)
-   [`631cf72`](eslint/eslint@631cf72) docs: note --ignore-path not supported with flat config ([#&#8203;16434](eslint/eslint#16434)) (Andy Edwards)
-   [`1692840`](eslint/eslint@1692840) docs: fix syntax in examples for new config files ([#&#8203;16427](eslint/eslint#16427)) (Milos Djermanovic)
-   [`d336cfc`](eslint/eslint@d336cfc) docs: Document extending plugin with new config ([#&#8203;16394](eslint/eslint#16394)) (Ben Perlmutter)

#### Chores

-   [`e917a9a`](eslint/eslint@e917a9a) ci: add node v19 ([#&#8203;16443](eslint/eslint#16443)) (Koichi ITO)
-   [`4b70b91`](eslint/eslint@4b70b91) chore: Add VS Code issues link ([#&#8203;16423](eslint/eslint#16423)) (Nicholas C. Zakas)
-   [`232d291`](eslint/eslint@232d291) chore: suppress a Node.js deprecation warning ([#&#8203;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 <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1599
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@silverwind
Copy link
Contributor

Found one regression: /bin/**.js no longer matches bin/foo.js, but did before this PR. I've updated my config to just /bin/ which works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
4 participants