fix: Ensure flat config unignores work consistently like eslintrc by nzakas · Pull Request #16579 · eslint/eslint

nzakas · 2022-11-23T21:24:36Z

We have several tests that were ignored back when we were waiting for fast-glob fixes. Because we no longer use fast-glob, the tests now work.

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
[ ] Add something to the core
[x] Other, please explain:

Test updates

What changes did you make? (Give an overview)

There were several tests that we were ignoring for FlatESLint due to bugs in fast-glob. Now that we no longer use fast-glob, we can make these tests run.

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

I changed the last test a bit because ignores works a bit differently than ignorePatterns did. Please double-check that change.

netlify · 2022-11-23T21:24:42Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`e16e16a`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/639b8b3938dd940009ec3c8e

nzakas · 2022-11-23T21:25:06Z


            const { prepare, cleanup, getPath } = createCustomTeardown({
-                cwd: root,
+                cwd: `${root}-unignores`,


Seems like there was some timing issues where just root was colliding due to Mocha running some tests in parallel, so I made them unique.

nzakas · 2022-11-23T21:25:25Z

-                    path.join(root, "node_modules/foo/index.js")
+                    path.join(getPath(), "eslint.config.js"),
+                    path.join(getPath(), "foo.js"),
+                    path.join(getPath(), "node_modules/foo/.dot.js"),


We now include dot files by default, so had to update tests accordingly.

nzakas · 2022-11-23T21:27:04Z

-                    "node_modules/myconf/eslint.config.js": `module.exports = {
-                        ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
+                    "node_modules/myconf/eslint.config.js": `module.exports = [{
+                        ignores: ["!node_modules/myconf", "foo/*.js"],


Remove "**/eslint.config.js" from this array. With ignorePatterns, that file would be ignored; with ignores, the file isn't ignored due to "!node_module/myconf unignoring the whole directory, which overrides "**/eslint.config.js".

nzakas · 2022-11-23T21:27:30Z

-                        ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
+                    "node_modules/myconf/eslint.config.js": `module.exports = [{
+                        ignores: ["!node_modules/myconf", "foo/*.js"],
+                    }, {


Had to separate into a different config object so ignores is treated as global ignores.

mdjermanovic · 2022-11-24T13:51:52Z

                    "eslint.config.js": `module.exports = {
                        ignores: ["!**/node_modules/foo/**"]


I wouldn't expect this pattern to have any effects after **/node_modules/**.

**/node_modules/** !**/node_modules/foo/**

The first test below asserts that node_modules/foo/index.js is not ignored. I think that this file should be ignored because it's in ignored directory node_modules/foo.

**/node_modules/** ignores node_modules/foo, but !**/node_modules/foo/** does not unignore it.

Hmmm, are you sure about that? it seems like this test was indicating our behavior worked in a different way.

Generally we are treating patterns ending with star-str as file matchers and not directory matchers.

Here is the current test, which verifies this behavior:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L5413-L5460

Here is the current test, which verifies this behavior:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L5413-L5460

In the original test, the list of patterns is:

/**/node_modules/* !/node_modules/foo

node_modules/ doesn't match any patterns; node_modules/foo/ matches /**/node_modules/* but also matches a subsequent negative pattern !/node_modules/foo so it's not ignored; node_modules/foo/index.js doesn't match any patterns.

In the flat eslint test, the list of patterns is:

**/node_modules/** !**/node_modules/foo/**

node_modules/ doesn't match any patterns; node_modules/foo/ matches **/node_modules/** and doesn't match any negative patterns so node_modules/foo/ is ignored and thus node_modules/foo/index.js should be ignored because it's in an ignored directory.

Generally we are treating patterns ending with star-str as file matchers and not directory matchers.

I think it would be confusing if trailing /** doesn't match subdirectories, as it is expected to match everything.

In my opinion, foo/** pattern should not match foo/ directory, but it should match foo/bar/ directory. This is how gitignore works. In minimatch, however, foo/** does match foo/ directory, but that may be a bug (isaacs/minimatch#179).

Looks like that minimatch behavior is intentional.

I'll take some time to look at this again...it's such an edge case that I think there's room for disagreement on how to handle this. Because we are not explicitly guaranteeing to work the same way gitignore works, I think we can kind of chart our own course here.

So interestingly, it appears that ConfigArray actually works the way you describe. I added some tests to verify:
humanwhocodes/config-array#73

It looks like there must be a bug on the ESLint side of things, so I'm investigating that next.

I'll take a look at humanwhocodes/config-array#73.

So it actually looks like it's a bug in isDirectoryIgnored() as opposed to isFileIgnored().

Fix is up, for when you have time:
humanwhocodes/config-array#74

mdjermanovic · 2022-11-24T14:38:13Z

-                    "node_modules/myconf/eslint.config.js": `module.exports = {
-                        ignores: ["**/eslint.config.js", "!node_modules/myconf", "foo/*.js"],
+                    "node_modules/myconf/eslint.config.js": `module.exports = [{
+                        ignores: ["!node_modules/myconf", "foo/*.js"],


!node_modules/myconf also shouldn't have any effects after **/node_modules/**. because it unignores the directory but everything under it is still ignored by **/node_modules/**.

This also seems counter to what existing tests are indicating. 🤔

Here is the original test:
https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L6426-L6459

It seems to behave in the same way as I have currently.

Here is the original test: https://github.com/eslint/eslint/blob/main/tests/lib/eslint/eslint.js#L6426-L6459

This is a bit different because in eslintrc the default pattern is /**/node_modules/* (* instead of ** at the end is the difference; leading slash doesn't matter), so in the original test the list of patterns is:

/**/node_modules/* !/node_modules/myconf

node_modules/ doesn't match any patterns; node_modules/myconf/ matches /**/node_modules/* but also matches a subsequent negative pattern !/node_modules/myconf so it's not ignored; node_modules/myconf/foo/ doesn't match any patterns; node_modules/myconf/foo/test.js doesn't match any patterns.

In the flat eslint test, the list of patterns is:

**/node_modules/** !node_modules/myconf

node_modules/ doesn't match any patterns; node_modules/myconf/ matches **/node_modules/* but also matches a subsequent negative pattern !node_modules/myconf so it's not ignored; node_modules/myconf/foo/ matches **/node_modules/** and doesn't match any negative patterns so that should be the end - directory node_modules/myconf/foo/ is ignored and thus everything under it is ignored as well, including node_modules/myconf/foo/test.js.

nzakas · 2022-12-14T21:01:41Z

After digging deeply into this, it appears that this wasn't just a chore, but actually we needed a bug fix, so I've switched this PR to "fix".

I've updated the default ignore pattern for flat config to be **/node_modules/* to match what we have for eslintrc. This, plus an update of config-array now makes flat config unignores work similarly to eslintrc.

nzakas · 2022-12-14T21:21:51Z

Hmmmm, it looks like tests are failing on Node.js 19 only...no idea why that would be. I even downloaded Node.js 19 locally and can't reproduce. 🤔 No more time to look into this today, so if anyone has any ideas, I'd appreciate the help.

mdjermanovic · 2022-12-14T21:34:35Z

Seems that the problem is file: with recently released npm v9.2.0 which is included in recently released Node v19.3.0.

mdjermanovic · 2022-12-14T21:44:42Z

This is actually npm major upgrade v8 -> v9, so there must be a breaking change in v9 that's causing this. On the latest working commit it was { node: 'v19.2.0', npm: '8.19.3' }, now it is { node: 'v19.3.0', npm: '9.2.0' }.

mdjermanovic · 2022-12-14T23:29:27Z

it looks like tests are failing on Node.js 19 only

Fixed on main now.

mdjermanovic · 2022-12-15T20:24:04Z

There's a merge conflict now.

We have several tests that were ignored back when we were waiting for fast-glob fixes. Because we no longer use fast-glob, the tests now work.

nzakas · 2022-12-15T21:01:57Z

Thanks for fixing that! I've rebased and resolved the merge conflicts.

mdjermanovic · 2022-12-15T21:05:04Z

docs/** pattern now ignores docs/ directory? This is observable from running npm run lint:docsjs ( runs eslint docs/.eleventy.js):

C:\milos\eslint\docs\.eleventy.js
  0:0  warning  File ignored by default.  Use a negated ignore pattern (like "--ignore-pattern '!<relative/path/to/filename>'") to override

✖ 1 problem (0 errors, 1 warning)

I think this is good, because this is how minimatch works, and whatever we try to adjust in minimatch's behavior probably won't work 100% correctly, just wanted to check with you if this is intentional because config-array docs still state that it shouldn't match, here.

nzakas · 2022-12-15T21:19:48Z

Oops yes, that is intentional. I just forgot to update the config-array docs:
humanwhocodes/config-array#76

mdjermanovic · 2022-12-16T11:35:10Z

+            beforeEach(prepare);
+            afterEach(cleanup);
+
+            it("'isPathIgnored()' should return 'true' for 'node_modules/foo/index.js'.", async () => {


Suggested change

it("'isPathIgnored()' should return 'true' for 'node_modules/foo/index.js'.", async () => {

it("'isPathIgnored()' should return 'false' for 'node_modules/foo/index.js'.", async () => {

Test title doesn't match the assertion in the test.

The assertion is correct. node_modules/foo/ is ignored by default, but since **/node_modules/foo/** now matches node_modules/foo/, !**/node_modules/foo/** will unignore it, so the files inside are not ignored.

mdjermanovic · 2022-12-16T11:35:34Z

+                assert.strictEqual(await engine.isPathIgnored("node_modules/foo/index.js"), false);
+            });
+
+            it("'isPathIgnored()' should return 'true' for 'node_modules/foo/.dot.js'.", async () => {


Suggested change

it("'isPathIgnored()' should return 'true' for 'node_modules/foo/.dot.js'.", async () => {

it("'isPathIgnored()' should return 'false' for 'node_modules/foo/.dot.js'.", async () => {

Same as the previous, the test is correct just the title should be updated.

mdjermanovic

LGTM, thanks! I'll prepare a follow-up to fix #16579 (comment) and to update ignore patterns in our config file. Not a blocker for the release.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.29.0` -> `8.30.0`](https://renovatebot.com/diffs/npm/eslint/8.29.0/8.30.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.30.0`](https://github.com/eslint/eslint/releases/tag/v8.30.0) [Compare Source](eslint/eslint@v8.29.0...v8.30.0) #### Features - [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#16637](eslint/eslint#16637)) (Daniel Bartholomae) - [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#16654](eslint/eslint#16654)) (Sébastien Règne) #### Bug Fixes - [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#16579](eslint/eslint#16579)) (Nicholas C. Zakas) - [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#16611](eslint/eslint#16611)) (Milos Djermanovic) #### Documentation - [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#16663](eslint/eslint#16663)) (Nicholas C. Zakas) - [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#16563](eslint/eslint#16563)) (Ben Perlmutter) - [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#16606](eslint/eslint#16606)) (Sam Chen) - [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#16631](eslint/eslint#16631)) (Percy Ma) - [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#16625](eslint/eslint#16625)) (Milos Djermanovic) - [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#16628](eslint/eslint#16628)) (Karl Horky) - [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#16591](eslint/eslint#16591)) (Tanuj Kanti) - [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#16566](eslint/eslint#16566)) (Ben Perlmutter) - [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#16607](eslint/eslint#16607)) (Pavel) - [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#16603](eslint/eslint#16603)) (Sam Chen) #### Chores - [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@eslint/eslintrc](https://github.com/eslint/eslintrc)[@1](https://github.com/1).4.0 ([#16675](eslint/eslint#16675)) (Milos Djermanovic) - [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#14827](eslint/eslint#14827) ([#16315](eslint/eslint#16315)) (Patrick McElhaney) - [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#16664](eslint/eslint#16664)) (Milos Djermanovic) - [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#16618](eslint/eslint#16618)) (Bryan Mishkin) </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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689 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>

eslint-github-bot Bot added chore This change is not user-facing triage An ESLint team member will look at this issue soon labels Nov 23, 2022

nzakas commented Nov 23, 2022

View reviewed changes

mdjermanovic added 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 Nov 24, 2022

mdjermanovic reviewed Nov 24, 2022

View reviewed changes

This comment was marked as spam.

Sign in to view

eslint deleted a comment from Gstar123456 Nov 25, 2022

nzakas changed the title ~~chore: Finish FlatESLint tests~~ fix: Ensure flat config unignores work consistently like eslintrc Dec 14, 2022

nzakas added bug ESLint is working incorrectly and removed chore This change is not user-facing labels Dec 14, 2022

nzakas requested a review from a team as a code owner December 14, 2022 21:00

mdjermanovic mentioned this pull request Dec 14, 2022

ci: fix npm v9 problem with file: #16664

Merged

1 task

nzakas added 2 commits December 15, 2022 13:01

chore: Finish FlatESLint tests

ef6bcc8

We have several tests that were ignored back when we were waiting for fast-glob fixes. Because we no longer use fast-glob, the tests now work.

Update default config and tests

e16e16a

nzakas force-pushed the flat-eslint-tests branch from 5e77440 to e16e16a Compare December 15, 2022 21:01

nzakas mentioned this pull request Dec 15, 2022

Update docs about ignored patterns humanwhocodes/config-array#76

Open

mdjermanovic reviewed Dec 16, 2022

View reviewed changes

mdjermanovic approved these changes Dec 16, 2022

View reviewed changes

mdjermanovic merged commit 1a327aa into main Dec 16, 2022

mdjermanovic deleted the flat-eslint-tests branch December 16, 2022 22:32

mdjermanovic mentioned this pull request Dec 19, 2022

chore: update ignore patterns in eslint.config.js #16678

Merged

1 task

github-actions Bot mentioned this pull request May 2, 2023

Upgraded some dependencies - Attempt 2 AurorNZ/report-updated-dependencies#638

Merged

eslint-github-bot Bot locked and limited conversation to collaborators Jun 15, 2023

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 Jun 15, 2023

		"eslint.config.js": `module.exports = {
		ignores: ["!/node_modules/foo/"]

	it("'isPathIgnored()' should return 'true' for 'node_modules/foo/index.js'.", async () => {
	it("'isPathIgnored()' should return 'false' for 'node_modules/foo/index.js'.", async () => {

Uh oh!

Conversation

nzakas commented Nov 23, 2022

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?

Uh oh!

netlify Bot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint canceled.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

nzakas commented Dec 14, 2022

Uh oh!

nzakas commented Dec 14, 2022

Uh oh!

mdjermanovic commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdjermanovic commented Dec 14, 2022

Uh oh!

mdjermanovic commented Dec 14, 2022

Uh oh!

mdjermanovic commented Dec 15, 2022

Uh oh!

nzakas commented Dec 15, 2022

Uh oh!

mdjermanovic commented Dec 15, 2022

Uh oh!

nzakas commented Dec 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

netlify Bot commented Nov 23, 2022 •

edited

Loading

mdjermanovic commented Dec 14, 2022 •

edited

Loading