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

Disregard default ignores when ignoreFiles is used #2236

Closed
bzang opened this issue Jan 4, 2017 · 20 comments
Closed

Disregard default ignores when ignoreFiles is used #2236

bzang opened this issue Jan 4, 2017 · 20 comments
Assignees
Labels
status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules
Milestone

Comments

@bzang
Copy link

bzang commented Jan 4, 2017

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

Target files located in any node_modules folder are ignore even when I specifically want to lint them.

Which rule, if any, is this issue related to?

N/A

What CSS is needed to reproduce this issue?

N/A

What stylelint configuration is needed to reproduce this issue?

N/A

Which version of stylelint are you using?

7.7.1

How are you running stylelint: CLI, PostCSS plugin, Node API?

CLI with stylelint "packages/node_modules/**/*.css" --config .stylelintrc.yml -i node_modules

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

No

What did you expect to happen?

All files with extension css in the packages/node_modules folder to be linted

What actually happened (e.g. what warnings or errors you are getting)?

Nothing happens

Possible Solutions

@alexander-akait
Copy link
Member

@bzang why you need linting in node_modules? it is quite specific and would like to hear real use case

@alexander-akait alexander-akait added the status: needs discussion triage needs further discussion label Jan 6, 2017
@bzang
Copy link
Author

bzang commented Jan 6, 2017

@evilebottnawi. Our project uses the alle pattern https://github.com/boennemann/alle, which is a monorepo pattern that places all packages in a packages/node_modules folder. This allows node to traverse up the file tree to find modules that are in the repo and make them usable as imports. As it stands, even if we point stylelint to a specific file, the file does not get linted due to the hard coded alwaysIgnore values.

@jeddy3
Copy link
Member

jeddy3 commented Jan 8, 2017

@bzang Interesting use case. Do you encounter the same issue when using eslint as they also ignore the node_modules directory by default?

@jeddy3 jeddy3 changed the title node_modules always ignored Always ignoring node_modules is incompatible with alle monorepo pattern Jan 8, 2017
@bzang
Copy link
Author

bzang commented Jan 8, 2017

@jeddy3, eslint actually only ignores the base node_modules directory. That's why they specify /node_modules/*. Any other node_modules directory isn't automatically ignored.
I think this is a reasonable setup since the root node_modules directory is generally where npm installs everything.

@jeddy3
Copy link
Member

jeddy3 commented Jan 9, 2017

@bzang Thanks for looking into it.

only ignores the base node_modules directory.

Funnily enough, we're following the lerna monorepo approach on my current project.

packages/
  package_1/
    node_modules/
  package_2/
    node_modules/

So, having stylelint ignore any file with node_modules somewhere in the path is beneficial to us.

Having said that, as stylelint doesn't have a option to disable the default ignores, perhaps they should be more permissive? @stylelint/core Should we change this default behaviour for 8.0.0?

@bzang
Copy link
Author

bzang commented Jan 9, 2017

@jeddy3, yea we were using Lerna before switching over to alle, and stylelint worked great!

I believe that the ignoreFiles config can already deal with additional ignore paths. So passing something like packages/**/node_modules/* should handle the Lerna use case.

It makes sense to have node_modules as a default ignore because that is a very common use case, but incorporating an override would add a lot more flexibility.

@alexander-akait
Copy link
Member

@jeddy3 maybe if not passed ignoreFiles patterns use default, if passed use only their?

@BradleyStaples
Copy link

BradleyStaples commented Jan 11, 2017

I was just looking into this as well, as my company uses both /node_modules and /src/node_modules in our apps because of our internal framework. Any CSS files in /src/node_modules/... are showing in the list of sources in the console output, but have (ignored) after them.

I tried setting ignoreFiles to [], '', null, '!/src/node_modules/**/*.scss', and ['!/src/node_modules/**/*.scss'] with no changes in behavior.

Green file was successfully linted, gray file was ignored.
ignored_file_stylelint

👍 for doing like ESLint does (which we also use), where the base /node_modules/ gets ignored but not any nested node_modules directories.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 12, 2017

@jeddy3 i think we should change logic in the 8.0.0 release for nested node_modules directories
Our code:

const alwaysIgnoredGlobs = [ "**/node_modules/**", "**/bower_components/**" ]

Eslint code: https://github.com/eslint/eslint/blob/616611afe19da5380344042fc73c7b91e59375cc/lib/ignored-paths.js#L32

@jeddy3
Copy link
Member

jeddy3 commented Jan 12, 2017

i think we should change logic in the 8.0.0 release for nested node_modules directories

I think so too.

Releasing 8.0.0 is a bit away yet, and so what are our options for getting @bzang & @BradleyStaples up and running in the interim? Would a no-default-ignore option suffice here?

@alexander-akait
Copy link
Member

@jeddy3 no-default-ignore sgtm

@jeddy3
Copy link
Member

jeddy3 commented Jan 12, 2017

@davidtheclark Always keen for your input and nod of approval when adding options to the CLI. Any objection to this option, or shall I open it out to contribution?

@davidtheclark
Copy link
Contributor

Ugh. Sticky situation. no-default-ignore really bugs me, and I feel weird about including weird logic for such a weird use-case with nested node_modules. Next we'll get an issue with people complaining because their nested node_modules are being linted — which is totally counterintuitive because other node_modules (non-nested) are being automatically ignored.

Here's what I'd favor: Make ignoreFiles have a regular old default of **/node_modules/**/* — all node_modules. A regular old default is just overridden when you provide a value — so that's what would happen. If you want to ignore other files, you include your own ignore pattern, including whatever node_modules ignores you want. @evilebottnawi suggested the same above, I believe.

@alexander-akait
Copy link
Member

@davidtheclark right, perhaps this logic is best suited to solve the issue

@jeddy3
Copy link
Member

jeddy3 commented Jan 13, 2017

A regular old default is just overridden when you provide a value — so that's what would happen. If you want to ignore other files, you include your own ignore pattern, including whatever node_modules ignores you want.

SGTM, thanks for chiming in. This change of behaviour will be for 8.0.0, right? As some users might be relying on the fact that node_modules is always ignored regardless of whether they have specified their own pattern or not.

@jeddy3 jeddy3 changed the title Always ignoring node_modules is incompatible with alle monorepo pattern Disregard default ignores when ignoreFiles is used Jan 13, 2017
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Jan 13, 2017
@davidtheclark
Copy link
Contributor

This change of behaviour will be for 8.0.0, right

Yeah, absolutely.

@jeddy3 jeddy3 added this to the 8.0.0 milestone Jan 13, 2017
@BradleyStaples
Copy link

Is there a rough guess for when 8.0.0 will be out? I'm going to have to find some solution to lint our files at work and I'm likely unable to wait for a long time, so I'll need to evaluate my options.

@jeddy3
Copy link
Member

jeddy3 commented Jan 14, 2017

Is there a rough guess for when 8.0.0 will be out?

Probably a few weeks as there's quite a bit left to do. You can help us speed up the process by tackling some of the 8.0.0 milestone issues.

@bzang
Copy link
Author

bzang commented Jan 18, 2017

Thanks for hopping on this everyone. I definitely agree with @davidtheclark's solution. That seems to be the most sustainable and also similar to how other tooling environments like webpack deal with overrides and ignores. Will lend a hand at the 8.0.0 milestone issues to speed this along.

@davidtheclark davidtheclark added the good first issue is good for newcomers label Jan 23, 2017
@hudochenkov hudochenkov added the status: wip is being worked on by someone label Feb 23, 2017
@hudochenkov hudochenkov self-assigned this Feb 23, 2017
@jeddy3 jeddy3 removed good first issue is good for newcomers status: ready to implement is ready to be worked on by someone labels Apr 8, 2017
@davidtheclark
Copy link
Contributor

Closed by #2464.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules
Development

No branches or pull requests

6 participants