Bug: [new config system] .eslintignore doesn't work like .gitignore

[mdjermanovic]

Environment

Node version: v16.14.0
npm version: v8.3.1
Local ESLint version: v8.23.0 (Currently used)
Global ESLint version: Not found
Operating System: win32 10.0.19044

What parser are you using?

Default (Espree)

What did you do?

# .eslintignore

foo/
!bar.js

// eslint.config.js

module.exports = [{
    rules: {
        semi: "error"
    }
}];

// foo/bar.js

a

npx eslint foo/bar.js

What did you expect to happen?

0:0  warning  File ignored because of a matching ignore pattern. Use "--no-ignore" to override

Per the .gitignore specification, foo/bar.js should be ignored because it is not possible to re-include a file if a parent directory of that file is excluded. With an equivalent .eslintrc.js config file in place of the eslint.config.js config file, foo/bar.js would be ignored.

What actually happened?

  1:2  error  Missing semicolon  semi

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Participation

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

Additional comments

There may be other differences between the .gitignore spec and our custom interpretation of the patterns, so it might be better to keep using the ignore library (assuming that .eslintignore should still behave like .gitignore).

[nzakas]

There may be other differences between the .gitignore spec and our custom interpretation of the patterns, so it might be better to keep using the ignore library (assuming that .eslintignore should still behave like .gitignore).

Unfortunately this is not so easy. Because ignores in eslint.config.js work differently than in .eslintignore and yet are expected to work together (being able to unignore a file from eslint.config.js that was ignored in .eslintignore), I had to move to one common way of interpreting ignores. I can take another look, but I'm fairly certain this won't be a viable option.

Note: Originally the flat config RFC specified that .eslintignore would not be supported due to the complexities with how ignoring files would work between .eslintignore and eslint.config.js.

So I think we have these possible paths forward:

1. Figure out a way to mimic .gitignore without using the ignore package
2. Try to reincorporate ignore without breaking existing tests
3. Live with this difference in functionality
4. Stop supporting .eslintignore

I'm pretty burned out from all this config work, so if anyone else wants to take a look, I'd appreciate it.

[mdjermanovic]

Because ignores in eslint.config.js work differently than in .eslintignore and yet are expected to work together

I don't think it's viable for the two different styles to work together, because each style has its own rules about how a list of patterns as a whole should work. I think we should decide between the .gitignore style (with the .gitignore interpretation of individual patterns and the .gitignore rules about how lists of patterns work) and the other style (with globs and ESLint-specific rules about how lists of patterns work).

1. Figure out a way to mimic .gitignore without using the ignore package
2. Try to reincorporate ignore without breaking existing tests

If we decide to support .gitignore-style, then it seems much better to use an existing library like the ignore package.

3. Live with this difference in functionality
4. Stop supporting .eslintignore

If it comes down to these two choices, I'm in favor of dropping .eslintignore. It would be very confusing if we keep it but it doesn't work the exact same way as it used to. Also, use cases like eslint --ignore-path .gitignore won't work as expected.

I think the first question should be - do we want to support .gitignore-style?

Pros:

• This is how ignoring files in ESLint already works and has been working since v2.0.0, so there will be no surprises.
• It's a well-known specification.
• It seems common (.gitignore, .npmignore, .prettierignore, .stylelintignore are .gitignore-style files) and the files can be reused, e.g., eslint --ignore-path .gitignore.

Cons:

• Glob libraries like glob or globby/fast-glob don't support passing .gitignore-style patterns, so we would have to add our own handling on top, or maybe even keep maintaining our FileEnumerator utility. Though, the mentioned libraries do not support the currently implemented style either (Can't unignore patterns inside of ignore option mrmlnc/fast-glob#356).
• This would be a change in the flat config design - ignores: [] would become a list of .gitignore patterns.

[nzakas]

I remembered last night why I didn’t use the ignore package. There were two issues:

1. in order to use ignore, we would need to write a new version of FileEnumerator just for that purpose instead of using something like globby, which already exists. I don’t think we should be maintaining a globbing utility when there are plenty of better-tested solutions out there. Globby only accepts strings as ignore patterns and doesn’t implement the gitignore spec.
2. If we keep two different ways of handling ignores, that means they can’t interact. With eslintrc, you can use ignorePatterns to negate a pattern in eslintignore. Flat config allows this too, because it translates the eslintignore patterns into minimatch patterns, making the config array the single source of truth for ignores.

As a note: the other examples you mentioned don’t all implement the gitignore spec. npmignore definitely doesn’t as it uses minimatch.

I think there is a strong argument to eliminate eslintignore for flat config. Early on, users asked us to put ignores into the config file (eslintrc) so they could have all of their configuration in one spot. Once we did that, we probably could have eliminated eslintignore. Also, with flat config, people can create an instance of ignore themselves and drop that into an ignores array. (This is actually how FlatCompat works.)

[nzakas]

@btmills could use your input here.

[btmills]

If there's no good way to have .gitignore-style .eslintignore alongside flat config, I'd agree we're better off taking this opportunity to drop it than trying to shoehorn it in and being stuck with it. If we get a ton of pushback, deciding not to support it now doesn't mean we can't add support in the future if someone cares enough to figure out how to make it work.

And as @nzakas pointed out above, if someone still wants to be able to reuse their .gitignore file, they can. I wouldn't be surprised to see a couple different third-party packages designed to drop ignores into flat config with slightly different semantics. At least those tradeoffs are available to the user.

[nzakas]

Okay, it sounds like we are in agreement to remove eslintignore support and see how things go.

[mdjermanovic]

The same applies to --ignore-pattern CLI option. Assuming that we want to keep this option, I think the patterns should have the same syntax and semantics as patterns in config ignores: []. In other words, I think we shouldn't do .map(gitignoreToMinimatch) on these patterns.

Also the same for ignorePatterns FlatESLint constructor option, I don't think we should use gitignoreToMinimatch on these patterns either. Another question is - why we have this option in FlatESLint? It didn't exist in the ESLint class, and --ignore-pattern does not translate to it.

[nzakas]

I think the patterns should have the same syntax and semantics as patterns in config ignores: [].

I agree. If we remove eslintignore then it makes sense for —ignore-pattern to use the same syntax as the config files.

Another question is - why we have this option in FlatESLint? It didn't exist in the ESLint class, and --ignore-pattern does not translate to it.

That’s interesting. I think this comes from CLIEngine. The ESLint class is just a wrapper around CLIEngine, and it was difficult trying to reconcile the two files. I probably just copied it at some point and didn’t stop to verify.

eslint/lib/cli-engine/cli-engine.js

Line 79 in 8cc0bbe

* @property {string|string[]} [ignorePattern] One or more glob patterns to ignore.

It seems safe to remove that option.

[mdjermanovic]

@nzakas I could take this if you haven't already started working on it.

[nzakas]

@mdjermanovic go for it!

[mdjermanovic]

One question before removing ignorePatterns FlatESLint constructor option: was it maybe added to handle --ignore-pattern CLI option, as this comment implies?

I believe that patterns coming from --ignore-pattern should be relative to cwd. Currently, they're copied into overrideConfig, and they are relative to cwd because config ignores are relative to cwd. But, when we fix #16300, they will become relative to config.basePath, unless we adjust patterns in overrideConfig. I think it would actually make sense to treat all patterns (files and ignores) in baseConfig and overrideConfig as relative to cwd, because these configs are passed through API, and at that point the location of eslint.config.js is unknown to API consumers. In eslintrc, baseConfig and overrideConfig are relative to cwd.

[nzakas]

One question before removing ignorePatterns FlatESLint constructor option: was it maybe added to handle --ignore-pattern CLI option, as this comment implies?

Yes, that was the intent. But it looks like in eslintrc this option was removed rather than handled separately, which is why I think things got weird when I was trying to get flat config in. We should be mirroring what eslintrc did.

I believe that patterns coming from --ignore-pattern should be relative to cwd.

Agreed.

I think it would actually make sense to treat all patterns (files and ignores) in baseConfig and overrideConfig as relative to cwd

There is a distinction here: baseConfig (the default ESLint config) contains only patterns that begin with **, so it doesn't matter if they are resolved relative to cwd or the basePath, the result is always the same.

For overrideConfig, I agree it makes sense to normalize that to cwd, although I'm not quite sure how we'd do that. (cwd is always either equal to basePath or is contained within basePath) Are you suggesting we'd need to edit those patterns before passing into FlatConfigArray?

[mdjermanovic]

There is a distinction here: baseConfig (the default ESLint config) contains only patterns that begin with **, so it doesn't matter if they are resolved relative to cwd or the basePath, the result is always the same.

By baseConfig I mean the FlatESLint constructor option baseConfig (#16341)

For overrideConfig, I agree it makes sense to normalize that to cwd, although I'm not quite sure how we'd do that. (cwd is always either equal to basePath or is contained within basePath) Are you suggesting we'd need to edit those patterns before passing into FlatConfigArray?

I think the expected behavior would be that patterns in overrideConfig (and baseConfig FlatESLint constructor option if we decide to keep that option) are relative to cwd, as that's a known location for the consumer at the time constructor is called, unlike basePath (the location of eslint.config.js) which can be any of the parent directories. Technically, yes, if they should be relative to cwd, I think it would be best to edit patterns before passing into FlatConfigArray.

[nzakas]

After thinking about this a bit, assuming we keep baseConfig, I think it might actually make sense to keep using basePath. I believe the intent was to define a config that would be inserted before any config found in a file, so the result would be [ default config, base config, config file config, override config ]. Because the base config can only be defined via API, we can assume a certain level of sophistication necessary for its use.

overrideConfig could fall into the same category except that we are jamming CLI ignore patterns into it.

[nzakas]

I've been looking more deeply into this, and here's what I think is the best path forward:

1. I think that baseConfig and overrideConfig should both be relative to the basePath in the config array. I think it would be too confusing to try to make both of those relative to cwd. Because these are both API options (baseConfig isn't used through the CLI at all), I think it's fair to just explain in the docs that baseConfig and overrideConfig are essentially inserted into the array returned from eslint.config.js.
2. Given that, we would need to keep ignorePatterns as an option to FlatESLint so that --ignore-pattern can be added outside of overrideConfig. I do agree that --ignore-pattern would be expected to be relative to cwd.

What do you think?

[mdjermanovic]

1. I think that baseConfig and overrideConfig should both be relative to the basePath in the config array. I think it would be too confusing to try to make both of those relative to cwd. Because these are both API options (baseConfig isn't used through the CLI at all), I think it's fair to just explain in the docs that baseConfig and overrideConfig are essentially inserted into the array returned from eslint.config.js.

👍

I'd guess that specifying files/ignores isn't a common use case for these configs, so it seems better to simplify this greatly by not adjusting paths.

2. Given that, we would need to keep ignorePatterns as an option to FlatESLint so that --ignore-pattern can be added outside of overrideConfig. I do agree that --ignore-pattern would be expected to be relative to cwd.

👍

I assumed we might want to keep ignorePatterns for --ignore-pattern, so its removal was left out from #16355.

[mdjermanovic]

2. Given that, we would need to keep ignorePatterns as an option to FlatESLint so that --ignore-pattern can be added outside of overrideConfig. I do agree that --ignore-pattern would be expected to be relative to cwd

I'm working on this.

[jedwards1211]

@nzakas FWIW I made a complete implementation of the gitignore spec: https://github.com/jedwards1211/gitignore-fs.
Would make it easy to implement an option to just ignore everything that git does, at least. I could make a PR if you're interested

[nzakas]

@jedwards1211 Thanks for the option but we are removing gitignore-style ignores.

[TomerAberbach]

I know this is already closed out, but just wanted to add my thoughts.

Almost every project that uses ESLint also uses git (ESLint sort of acknowledges this by ignoring .git/** by default) and almost every git project has a .gitignore file.

There's virtually no scenario where someone running ESLint on a project with a .gitignore wants ESLint to run over the ignored files. The developer will always want ESLint to ignore those files. So it seems that by removing this functionality, almost every project using ESLint will have to duplicate its ignore rules between .gitignore and the ESLint config. This is unfortunate because the two configs could easily become out of sync. People won't like that so inevitably they will try to come up with ways to generate the ESLint ignore patterns from their .gitignore files.

If that's where we'd likely end up, then I wonder why not bake this functionality into ESLint to avoid N different userland solutions for the problem? I get that it's a complex issue, but it seems like scoping it out of ESLint would just push the problem into userland, which doesn't seem desirable.

[airtonix]

stupid question:

why are we even bothering to mimic gitignore... isn't things like micromatch etc better syntax? (spoiler: i think so)

If that's where we'd likely end up, then I wonder why not bake this functionality into ESLint to avoid N different userland solutions for the problem? I get that it's a complex issue, but it seems like scoping it out of ESLint would just push the problem into userland, which doesn't seem desirable.

can just use some logic like "convert found ignore file to micromatch globs, then merge as winning globs"