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

Adding ignore-path flag referencing a file in node_modules not respecting ignore config #9227

Closed
kentcdodds opened this Issue Sep 4, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@kentcdodds
Copy link
Contributor

kentcdodds commented Sep 4, 2017

Tell us about your environment

  • ESLint Version: 4.6.1 (latest)
  • Node Version: 8.4.0 (latest)
  • npm Version: 5.4.0 (latest)

What parser (default, Babel-ESLint, etc.) are you using? N/A

Please show your full configuration: N/A

Configuration
{}

What did you do? Please include the actual source code causing the issue.

eslint --ignore-path node_modules/kcd-scripts/dist/config/eslintignore .

What did you expect to happen?

I expected the eslintignore to be applied

What actually happened? Please include the actual, raw output from ESLint.

It is not. I get an error indicating that eslint is processing files in node_modules.

Here's the contents of node_modules/kcd-scripts/dist/config/eslintignore:

node_modules
coverage
dist
build
out
.next

I think that the docs indicate that the patterns here will be relative to the .eslintignore file, so I changed the eslintignore file to various patterns (including ../../../node_modules/, **/node_modules/**). None of those combinations seemed to work. I think there may be a bug here, but I'm uncertain... Happy to dig in more if someone can tell me I'm not missing something obvious.

To reproduce: https://github.com/kentcdodds/eslint-config-issue

Install deps and run npm run broken and npm run working.

Thanks!

@eslintbot eslintbot added the triage label Sep 4, 2017

@not-an-aardvark not-an-aardvark added accepted bug core and removed triage labels Sep 5, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Sep 5, 2017

Thanks for reporting, I can reproduce this.

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

kentcdodds commented Sep 5, 2017

How can I help fix this?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Sep 5, 2017

From investigating just now, I think the issue was introduced in e395919, where we switched to resolving ignore patterns from the location of the .eslintignore file rather than the CWD.

I think the problem is that in addition to resolving ignore patterns that appear in the .eslintignore file from the ignore file's location, we're also resolving the default ignore patterns (such as /node_modules/*) from the location of the ignore file, rather than the CWD. So when you use eslint --ignore-path node_modules/kcd-scripts/dist/config/eslintignore, it's effectively ignoring files in node_modules/kcd-scripts/dist/config/node_modules rather than ./node_modules, which causes the rest of the files in ./node_modules to get linted.

The cause of the issue seems to be here. this.baseDir is currently always set to the location of the ignore file (or the CWD if no ignore file is being used), so we're using the same relative path for both the default ignore patterns and the custom ignore patterns. I think the issue will be fixed if we use different relative paths for each check there (a path relative to the CWD for default patterns, and a path relative to the ignore file location for custom patterns).

edit: I think this would also need to be changed in a similar way, too.

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

kentcdodds commented Sep 5, 2017

Thanks for that insight! So, if I understand you correctly, the fix would be changing this:

eslint/lib/ignored-paths.js

Lines 229 to 251 in c8bf687

/**
* Determine whether a file path is included in the default or custom ignore patterns
* @param {string} filepath Path to check
* @param {string} [category=null] check 'default', 'custom' or both (null)
* @returns {boolean} true if the file path matches one or more patterns, false otherwise
*/
contains(filepath, category) {
let result = false;
const absolutePath = path.resolve(this.options.cwd, filepath);
const relativePath = pathUtil.getRelativePath(absolutePath, this.baseDir);
if ((typeof category === "undefined") || (category === "default")) {
result = result || (this.ig.default.filter([relativePath]).length === 0);
}
if ((typeof category === "undefined") || (category === "custom")) {
result = result || (this.ig.custom.filter([relativePath]).length === 0);
}
return result;
}

to:

     contains(filepath, category) {
 
         let result = false;
         const absolutePath = path.resolve(this.options.cwd, filepath);
-        const relativePath = pathUtil.getRelativePath(absolutePath, this.baseDir);
+        const baseDir = category === "default" ? this.options.cwd : this.baseDir;
+        const relativePath = pathUtil.getRelativePath(absolutePath, baseDir);

         if ((typeof category === "undefined") || (category === "default")) {
             result = result || (this.ig.default.filter([relativePath]).length === 0);
         }

         if ((typeof category === "undefined") || (category === "custom")) {
             result = result || (this.ig.custom.filter([relativePath]).length === 0);
         }

         return result;

     }

Also, what's a good place to write tests for this changed behavior? in tests/lib/ignored-paths.js?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Sep 5, 2017

I think that's almost right, although I think that would have the issue where category can also be null, in which case both the default and custom filters should be applied, so the if statements in that function aren't mutually exclusive.

Something like this should work:

     contains(filepath, category) {
 
         let result = false;
         const absolutePath = path.resolve(this.options.cwd, filepath);
-        const relativePath = pathUtil.getRelativePath(absolutePath, this.baseDir);

         if ((typeof category === "undefined") || (category === "default")) {
+            const relativePath = pathUtil.getRelativePath(absolutePath, this.options.cwd);
             result = result || (this.ig.default.filter([relativePath]).length === 0);
         }

         if ((typeof category === "undefined") || (category === "custom")) {
+            const relativePath = pathUtil.getRelativePath(absolutePath, this.baseDir);
             result = result || (this.ig.custom.filter([relativePath]).length === 0);
         }

         return result;

     }

Also, after I commented before I noticed another place that might also need to be changed in a similar way (to resolve default patterns from the cwd rather than this.baseDir).

Also, what's a good place to write tests for this changed behavior? in tests/lib/ignored-paths.js?

Yes, I think that's the right place to put them.

@pensierinmusica

This comment has been minimized.

Copy link

pensierinmusica commented May 13, 2018

Hi,

We have the same problem.

@not-an-aardvark @kentcdodds any chance of seeing this fixed?

@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

kentcdodds commented May 13, 2018

I'd love for this to be fixed, but my pull request hit a wall and I don't have any more time for it.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Jul 21, 2018

Working on this.

@not-an-aardvark not-an-aardvark self-assigned this Jul 21, 2018

not-an-aardvark added a commit that referenced this issue Jul 21, 2018

Fix: always resolve default ignore patterns from CWD (fixes #9227)
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.
@kentcdodds

This comment has been minimized.

Copy link
Contributor Author

kentcdodds commented Jul 26, 2018

Thank you! 👏

@cemremengu

This comment has been minimized.

Copy link

cemremengu commented Oct 9, 2018

Still getting this error with latest eslint 5.6.1. I am on windows. Any ideas?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Oct 9, 2018

@cemremengu May you please create a new issue? It's possible that a different set of circumstances can lead to the same behavior, but it's hard to tell without more information.

@eslint eslint bot locked and limited conversation to collaborators Jan 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.