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

Don't iterate over files and folders that should be ignored #1023

Closed
watson opened this issue Dec 6, 2017 · 19 comments · Fixed by standard/standard-engine#228
Closed

Don't iterate over files and folders that should be ignored #1023

watson opened this issue Dec 6, 2017 · 19 comments · Fixed by standard/standard-engine#228

Comments

@watson
Copy link
Member

watson commented Dec 6, 2017

In a project of mine, I have a build directory that quickly gets really large. It's ignored via .gitignore, so standard doesn't fail on it, but it still runs through all files in the directory. It results in a running time of 13 seconds as opposed to 1 second without the build directory.

I know this have been discussed before (#680) and it seems to be a "feature" inherited from eslint. But I'm opening this issue to hopefully start a discussion on how to work around this or how to potentially fix it (either in standard or in eslint).

@Flet
Copy link
Member

Flet commented Dec 6, 2017

Maybe this fixes it in eslint 4?
eslint/eslint#8706

@watson
Copy link
Member Author

watson commented Dec 6, 2017

Hmm not sure. I'm not sure why caching should fix it, but I know almost nothing about eslint internals, so I could very well be wrong.

@ematipico
Copy link
Member

AFAIK standard doesn't support eslint 4. I am right?

@watson
Copy link
Member Author

watson commented Dec 7, 2017

@ematipico Correct. I think what @Flet was hinting at was if the solution to this might come automatically when at some point standard is upgraded to 4.x

@Flet
Copy link
Member

Flet commented Dec 15, 2017

@watson could you try with standard@beta? It uses the latest version of eslint. At least we could determine if its fixed? :)

@watson
Copy link
Member Author

watson commented Dec 15, 2017

@Flet doesn't seem to fix it unfortunately:

image

@watson
Copy link
Member Author

watson commented Dec 15, 2017

hmmm hang on... might have spoke too soon. Let me just check something

@watson
Copy link
Member Author

watson commented Dec 15, 2017

Nah, I wanted to try to also ignore the folder inside package.json, but no dice. Here you can also see the time it takes if I delete the folder:

image

@Flet
Copy link
Member

Flet commented Dec 15, 2017

Aww, was worth a shot :(

@GantMan
Copy link

GantMan commented Dec 20, 2017

I'm having this same issue. Fix is traversing ignored paths and complaining with a exit code 1

@searls
Copy link

searls commented Dec 21, 2017

Can confirm, I have the same issue with a very-very-very large (almost cyclic) examples/node_modules ignored directory in testdouble.js (still reproducible at this commit). All major versions of Node.js literally run out of heapspace attempting to scan it.

@feross
Copy link
Member

feross commented Feb 27, 2018

The issue seems related to deglob. Posting more info from @searls:

screen shot 2018-02-27 at 3 41 09 pm

screen shot 2018-02-27 at 3 41 14 pm

@searls
Copy link

searls commented Feb 27, 2018

LOL amazing issue report

@Flet
Copy link
Member

Flet commented Feb 27, 2018

Nice thank you for digging @serls :)

@watson
Copy link
Member Author

watson commented Mar 12, 2018

I took at stab at this and found that this is a combination of a few issues.

As @searls pointed out, the issue is inside deglob.

  1. Diving further, the time is spend inside the glob module: https://github.com/standard/deglob/blob/bce559a275faf286395c52912a82c601a6740f2d/index.js#L23-L27
  2. The ignore option passed to the glob module does not contain patterns found in the .gitignore file, so files only ignored here will still be processed by glob
  3. Even if the lines from the .gitignore file were passed to the ignore option, these would have to end with /** to ignore whole directories. This is inconsistent with how .gitignore works and with what's documented in the README

Suggested solution:

  1. Document that you need to append /** to directories you wish to ignore in the package.json standard.ignore array
  2. Convert the patterns from .gitignore to patterns compatible with the glob module and add them to the list of ignored glob patterns

@feross what do you think of this?

@searls
Copy link

searls commented Mar 20, 2018

My preferred solution would be to update deglob to behave like the glob module or .gitignore when it comes to foo/**/* style patterns.

I realize it could be seen a breaking change, but one person's breaking change is another's patch fix! Based on the popularity of using **/* to mean "all files, recursively" I can't imagine anyone is intentionally passing deglob ignore rules of that type with an expectation they will not be ignored recursively.

Is that on the table?

@feross
Copy link
Member

feross commented Oct 28, 2020

I'm removing deglob for standard 16. So this issue will be fixed for free!

@searls
Copy link

searls commented Oct 29, 2020

Woohoo! Thanks @feross!

@feross
Copy link
Member

feross commented Oct 29, 2020

This will be fixed in standard 16

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants