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

Improve performance of globbing with ignored paths #2399

Closed
ArmorDarks opened this issue Feb 26, 2017 · 8 comments
Closed

Improve performance of globbing with ignored paths #2399

ArmorDarks opened this issue Feb 26, 2017 · 8 comments
Labels
status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules
Milestone

Comments

@ArmorDarks
Copy link

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

Stylelint works extremely slow with large directories by taking a lot of time to discover files targeted by input glob, despite glob contain paths, ignored in Stylelint config with ignoreFiles option.

I've digged in code and discovered that it happens, because Stylelin initially passes to globby whole input glob and does not account for specified in ignoreFiles option path and files.

By stumbling into very large directory, globby tries to explore it completely and it takes a lot of time (with 4000 files it almost endless), while it had to ignore it in first place due to specified in ignoreFiles option path.

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

Partially related to ignoreFiles option.

What CSS is needed to reproduce this issue?

Fortunately, no CSS needed to reproduce this issue. However, you will need directory with Craft CMS and Nginx (with around 4000 files) near css files.

What stylelint configuration is needed to reproduce this issue?

You need to launch Stylelint with very wide glob as input:

stylelint **/*.css

And large directory near your css files.

Which version of stylelint are you using?

7.9.0

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

Project with following structure:

|- docker <- directory with 4000 files
|- source
    |-- *.css
...

CLI with

stylelint **/*.css

Config stored in .stylelintrc.yml:

---
ignoreFiles:
  - docker/{,**/}*
  - docker/***
  - '**/docker/{,**/}*'
  - '**/docker/**'

extends: stylelint-config-standard

Funky and in some sense same globs for ignoreFiles just to be sure that I passed in correct path.

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

No

What did you expect to happen?

Stylelint should add specified in ignoreFiles option paths to passed to globby files list as ignored (with !), so that globby won't try to scan directory which won't be ever used at first place.

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

Stylelint hangs for very long time.

@davidtheclark
Copy link
Contributor

Stylelint should add specified in ignoreFiles option paths to passed to globby files list as ignored (with !), so that globby won't try to scan directory which won't be ever used at first place.

@ArmorDarks I'm not sure such a simple solution will work. The problem is that we allow different files to load different configs: there's not always just one config per process. For example, you could have 4 parallel folders, a/b, a/c, a/d, and a/e, each with a different stylelint config, and all of them linted at once via stylelint "a/**/*.css". Each of those configs could have its own ignoreFiles property — so there is no universal ignoreFiles glob that we can apply negatively to the original input glob "a/**/*.css". Put another way: We don't know whether a file will be ignored or not until we have loaded the configuration that is relevant to it. Unfortunately, I can't right now think of a good way around this problem.

It seems to me that the simplest solution here is actually for you the user to enter a more complete input glob, instead of using ignoreFiles at all — like so: stylelint "**/*.css, !**/docker/**".

@jeddy3 jeddy3 added the status: needs discussion triage needs further discussion label Feb 27, 2017
@alexander-akait
Copy link
Member

alexander-akait commented Feb 27, 2017

@davidtheclark agree, best solution use stylelint "**/*.css" "!**/docker/**"

@ArmorDarks
Copy link
Author

ArmorDarks commented Feb 27, 2017

Thanks for reply!

Hm, as I initially though, case isn't that easy indeed.

Since we're using Stylelint through npm scripts, solution with stylelint "**/*.css, !**/docker/**" is perfectly fine for us. It just wasn't that clear from documentation that inputs could be specified as list (probably my miss).

So, particularly my issue can be considered as resolved.

Though, for those who using it via CLI typing all ignores in console can be very combustome.

ESLint standard seems to behave in this matter much easier. It's just plug in and go, even in very large repositories. I can't tell does ESLint has same cascading approach for configs, though. I think it just applies ignores at first place, so if you're giving in wide glob for input, and your config has ignore: ["docker/**"], it won't try to look for configs in ignored directories at all.

Well, it boils down to the point, that it just wasn't clear, that ignoreFiles option applies ignore to application of this ruleset, not to discovery mechanisms.

Maybe we need another option? Then Stylelint could locate first closest config by gradually checking from root to deeper directories, check does it have any global ignores, and continue based on them? But I can imagine that in some scenarios it can make things even slower...

@davidtheclark
Copy link
Contributor

Open to implementation ideas to improve this.

There are other corollary problems to consider. For example, verbose reporting tells you how many files were ignored, and which ones — would we be ok eliminating that feature?

Maybe we need another option?

Another configuration option would face the same problem as this one: which config is privileged? Does that mean we need to search for multiple configs for every file?

@ArmorDarks
Copy link
Author

There are other corollary problems to consider. For example, verbose reporting tells you how many files were ignored, and which ones — would we be ok eliminating that feature?

I think this one is easier to solve — such wide globbing can be done only when verbose reporting is asked for, in other scenarios it seems to be excessive (except that configs story...)

Another configuration option would face the same problem as this one: which config is privileged? Does that mean we need to search for multiple configs for every file?

ESLint and other tools expecting first found config to be privileged (be it rc config file, first encountered packages.json and so on), and it seems to be logically expected behavior to me.

@sergesemashko
Copy link
Contributor

sergesemashko commented Mar 30, 2017

@ArmorDarks, I recently worked on adding cache to lint only changed and invalid files. That should improve stylelint execution time in general, but not globby part.
So, I'm curious, what are your globby and entire stylelint execution times? that would give a better idea of where the narrow place is, and, maybe, globby doesn't take big portion of time from overall stylelint execution.

@davidtheclark
Copy link
Contributor

@ArmorDarks: I'm trying to followup on this issue this weekend. To followup on your comments about ESLint: does ESLint even have an equivalent of config.ignoreFiles? I can't find it in the docs. All I can find is the recommendation to use .eslintignore.

If my suspicion is right that they do not have such a config property, then all we'd need to do to match ESLint here is to ensure that we don't look up .stylelintignore for each file, but only look it up once at the beginning of the process, relative to process.cwd(), and use that everywhere.

@jeddy3 jeddy3 changed the title Very slow globbing, even with ignored paths Improve performance of globbing with ignored paths Apr 8, 2017
@jeddy3 jeddy3 added status: wip is being 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 Apr 8, 2017
@jeddy3 jeddy3 added this to the 8.0.0 milestone 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

5 participants