Fix: always resolve default ignore patterns from CWD (fixes #9227) #10638
Conversation
This fixes an issue where the default ignore patterns, such as `/node_modules`, would always be resolved from the location of an `.eslintignore` file. As a result of the bug, the `node_modules` folder in the project root directory would not be ignored if an `.eslintignore` file in a subdirectory was being used.
not-an-aardvark
added
bug
lib/ignored-paths.js
Outdated
|
|
||
| addPatternRelativeToIgnoreFile(ig, pattern) { | ||
| ig.addPattern( | ||
| this.getBaseDir() === this.options.cwd |
There was a problem hiding this comment.
Should this say something like this.getBaseDir() === this.ignoreFileDir and invert the consequent and alternate below, similar to the method just above this one? Or do we know for sure that this.getBaseDir() will always be the ignore directory or CWD, nothing else?
Edit: I see later down, that condition should indeed hold. But I still think this change would make sense in case we change something in the implementation and this.getBaseDir() could expand to a few different paths.
| ig.addPattern( | ||
| this.getBaseDir() === this.options.cwd | ||
| ? pattern | ||
| : relativize(pattern, path.relative(this.ignoreFileDir, this.options.cwd)) |
There was a problem hiding this comment.
I'm sorry for I didn't review on this PR.
After this PR merged, some tests are failing on my environment (#10687). I'm not sure what is there difference between Appvoyer and my environment.
I have not found the cause yet, but the path separator is \ on Windows, isn't it a problem?
There was a problem hiding this comment.
Yes, it seems likely that this PR introduced the bug. I had assumed that it was fine because it was working on Appveyor, but it does seem like differences in path separator might cause problems.
If we can figure out what the issue is, we can use something like path.posix.relative rather than path.relative as necessary for consistent behavior.
There was a problem hiding this comment.
I guess this.options.cwd can be Windows path, so we cannot use path.posix.relative. Anyway, I added .replace(/\\/g, "/") after the two path.relative(...), but it didn't solve the issue. I'm investigating...
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (#9227)
What changes did you make? (Give an overview)
This fixes an issue where the default ignore patterns, such as
/node_modules, would always be resolved from the location of an.eslintignorefile. As a result of the bug, thenode_modulesfolder in the project root directory would not be ignored if an.eslintignorefile in a subdirectory was being used.Is there anything you'd like reviewers to focus on?
Nothing in particular