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

Bug: [new config system] Unexpected glob behavior in FlatESLint #16340

Closed
1 task
Tracked by #13481
fisker opened this issue Sep 22, 2022 · 11 comments · Fixed by #16369
Closed
1 task
Tracked by #13481

Bug: [new config system] Unexpected glob behavior in FlatESLint #16340

fisker opened this issue Sep 22, 2022 · 11 comments · Fixed by #16369
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes
Projects

Comments

@fisker
Copy link
Contributor

fisker commented Sep 22, 2022

Environment

Node version: 18.8.0
npm version: N/A
Local ESLint version: 8.23.1
Global ESLint version: A/A
Operating System: Windows

What parser are you using?

Default (Espree)

What did you do?

Run FlatESLint with different cwd on same file.

What did you expect to happen?

Should result the same.

What actually happened?

They result differently.

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

Step to reproduce:

  1. Clone the repo https://github.com/fisker/flat-eslint-cmd-bug-repo (or run on gitpod.io)
  2. run yarn && node test.mjs to see the difference

It seems the globby option {absolute: true} cause globby can't match any file.

Remove that line cann't really fix my problem, but it should be another issue, possible related to #16264

Since the globby version we are using is really old, not sure what causes that. (I'm a maintainer of globby)

@fisker fisker added bug ESLint is working incorrectly repro:needed labels Sep 22, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 22, 2022
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Sep 22, 2022
@mdjermanovic
Copy link
Member

Only test/integration/fixtures/vscode cwd loads test/integration/fixtures/vscode/.eslintignore. That works as intended.

However, the patterns in that file should not ignore the src/bootstrap-amd.js file, but they do. In particular, it's ignored by **/fixtures/** pattern.

This is either a bug or a surprising behavior in globby/fast-glob. We expect that the patterns in ignore will be interpreted as relative to cwd, but { cwd: "/path/to/test/integration/fixtures/vscode", ignore: ["**/fixtures/**"], absolute: true } effectively ignores everything, probably because the ignore pattern matches absolute paths.

@mdjermanovic
Copy link
Member

Since the globby version we are using is really old, not sure what causes that. (I'm a maintainer of globby)

I'm getting the same behavior with the latest globby v13.1.2:

// globby-test-ignore/test.mjs

import { globby } from 'globby';

const paths = await globby(["**/*"], {
    ignore: ["**/globby-test-ignore/**"],
    absolute: true
});

console.log(paths); // []

Is this the intended behavior?

@fisker
Copy link
Contributor Author

fisker commented Sep 22, 2022

it's ignored by /fixtures/ pattern.

No wonder I can't reproduce when trying to make the reproduction smaller. (Took me a while to setup)

@fisker
Copy link
Contributor Author

fisker commented Sep 22, 2022

Is this the intended behavior?

Are you running the script inside globby-test-ignore or outside?

@mdjermanovic
Copy link
Member

Is this the intended behavior?

Are you running the script inside globby-test-ignore or outside?

Inside, cwd ends with /globby-test-ignore.

I would expect that **/globby-test-ignore/** ignores /my/absolute/path/to/globby-test-ignore/**/globby-test-ignore/**, but it effectively ignores /my/absolute/path/to/globby-test-ignore/**

@fisker
Copy link
Contributor Author

fisker commented Sep 22, 2022

Sounds like a bug to me. But seems absolute ignore is expected? mrmlnc/fast-glob#113

@fisker
Copy link
Contributor Author

fisker commented Sep 22, 2022

Maybe this Prettier issue is the same reason. "Problem 2"

@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage Sep 22, 2022
@mdjermanovic
Copy link
Member

I can reproduce this behavior with ignores in our eslint.config.js.

-    }
+    },
+    {
+        ignores: ["**/eslint/**"]
+    }

(my local clone is in a directory named eslint).

npx eslint *.js
Oops! Something went wrong! :(

ESLint: 8.23.1

You are linting "*.js", but all of the files matching the glob pattern "*.js" are ignored.

If you don't want to lint these files, remove the pattern "*.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.

@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 repro:yes and removed repro:needed labels Sep 22, 2022
@mdjermanovic mdjermanovic changed the title Bug: Unexpected glob behavior in FlatESLint Bug: [new config system] Unexpected glob behavior in FlatESLint Sep 22, 2022
nzakas added a commit that referenced this issue Sep 30, 2022
Removes globby due to numerous issues with ignoring files and returning
the correct files.

Fixes #16354
Fixes #16340
Fixes #16300
@nzakas nzakas self-assigned this Oct 3, 2022
@nzakas nzakas moved this from Feedback Needed to Ready to Implement in Triage Oct 3, 2022
@nzakas
Copy link
Member

nzakas commented Oct 3, 2022

Can someone summarize what the problem is here? I've re-read this a couple of times and can't quite tell how to create a simple repro case.

@mdjermanovic
Copy link
Member

Here's a simplified example:

foo/
└── eslint.config.js
// eslint.config.js
module.exports = [{
    ignores: ["**/foo/**"]
}];

Running eslint "*.js" in the directory foo. This is project's root dir, where the eslint.config.js file is. basePath = cwd = "/path/to/foo".

Path to the project dir and project dir's name shouldn't matter, so the expected result is to lint eslint.config.js file.

Actual result:

Oops! Something went wrong! :(

ESLint: 8.24.0

You are linting "*.js", but all of the files matching the glob pattern "*.js" are ignored.

If you don't want to lint these files, remove the pattern "*.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.

because ignore pattern **/foo/** matches absolute path /path/to/foo/eslint.config.js.

@nzakas
Copy link
Member

nzakas commented Oct 4, 2022

Got it, thank you! Working on this now

nzakas added a commit that referenced this issue Oct 4, 2022
Removes globby due to numerous issues with ignoring files and returning
the correct files.

Fixes #16354
Fixes #16340
Fixes #16300
nzakas added a commit that referenced this issue Oct 10, 2022
Removes globby due to numerous issues with ignoring files and returning
the correct files.

Fixes #16354
Fixes #16340
Fixes #16300
Triage automation moved this from Ready to Implement to Complete Oct 11, 2022
mdjermanovic pushed a commit that referenced this issue Oct 11, 2022
* 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
@nzakas nzakas mentioned this issue Oct 13, 2022
69 tasks
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 10, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants