Chore: Cache fs reads in ignored-paths (fixes #8363)

[VictorHom]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:
Per #8363 , this pr introduces caching fs reads to the ignored-paths.js file. This is to reduce the synchronous reads which would affect performance.

What changes did you make? (Give an overview)
Per #8363 , this pr introduces caching fs reads to the ignored-paths.js file. This is to reduce the synchronous reads which would affect performance.

Is there anything you'd like reviewers to focus on?
config-file.js - I am unclear on how to verify the additional reads since #8363 also mentions similar repeats in this file.

Also should there be any tests related to caching?

Any suggestions on on refactoring or any other approaches is appreciated.

Let me know what you think and how I can make better.

[VictorHom]

@not-an-aardvark when you get a chance, can you review this pr?

• We should be careful with pr New: Add eslintIgnore support to package.json (fixes #8458) #8690 since they touch the same file.

[eslintbot]

LGTM

[not-an-aardvark]

Thanks! This generally looks good aside from a recommendation about where to use the cache.

Also, can you add a test for this change? One way to do it would be to stub out fs.readFileSync to make sure it's only called once. Another option would be to use mock-fs to simulate a fake .eslintignore file, then modify the file between the function calls and make sure the old version of the file is still used.

[not-an-aardvark]

Can you make the cache a part of the implementation of readIgnoreFile? That way, other functions like addIgnoreFile can just use readIgnoreFile as an abstraction without needing to worry about whether the files are already in the cache.

So readIgnoreFile could look like this:

class IgnoredPaths {
    constructor() {
        this.cache = {};
        // ...
    }
    readIgnoreFile(filePath) {
        if (!this.cache[filePath]) {
            this.cache[filePath] = fs.readFileSync(filePath, "utf8");
        }
        return this.cache[filePath];
    }
}

Then no other methods would have to use this.cache.

[VictorHom]

@not-an-aardvark I am still looking into this. I might have to do something like so. Does that make sense?

                addIgnoreFile(this.ig.custom, ignorePath, this.cache);
                addIgnoreFile(this.ig.default, ignorePath, this.cache);

or the main question I have would be: if I pull readIgnoreFile out of the constructor, how can I access it in the constructor?

[not-an-aardvark]

I think it would be better to avoid doing that. Conceptually, it seems like it would be simpler for the cache to be an implementation detail of readIgnoreFile. That way, everything else can just treat readIgnoreFile as an abstraction for "get the contents of this file somehow", without worrying about the details of how/whether files are cached.

This might be easier if you make addIgnoreFile and readIgnoreFile class methods in IgnoredPaths (and change calls to use this.addIgnoreFile() rather than addIgnoreFile()), because then readIgnoreFile will be able to deal with this.cache on its own rather than accepting it as an argument.

[VictorHom]

@not-an-aardvark
you're right. For some reason, the method approach was showing errors before, which is why I had the previous approach with passing the cache as a parameter. I am not sure why I had errors at that point.

I moved addIgnoreFile and readIgnoreFile to class methods now and it is much simpler.

Added a test as well, counting that the calls to readFileSync only happens once in the test instead of the two-time reads that happens right now when addIgnoreFile gets called twice in succession.

[eslintbot]

LGTM

[not-an-aardvark]

I might be missing something, but it seems like this is only called once, so it's not testing that the cache works. Shouldn't it be called twice instead?

[not-an-aardvark]

Can you put the spy logic in a beforeEach/afterEach block? As currently written, the spy won't be restored if there's an assertion failure, which could affect other tests and cause confusion.

[eslintbot]

LGTM

[eslintbot]

LGTM

[VictorHom]

@not-an-aardvark I adjusted the cache test.

[not-an-aardvark]

Looking good, but I found a very minor bug (in the code that I recommended you use before -- sorry about that).

[not-an-aardvark]

If an .eslintignore file exists and happens to be empty, the cache won't get used because this.cache[filePath] will be the empty string.

One way to resolve this would be to use if (typeof this.cache[filePath] === "undefined") instead.

[eslintbot]

LGTM

[eslintbot]

LGTM

[VictorHom]

addressed your comments and made updates

[not-an-aardvark]

Looks good to me. Thanks for contributing!

I'll just leave this open for another day or so in case anyone else wants to review it.

[platinumazure]

Just one minor comment, otherwise LGTM. Thanks!

[platinumazure]

Has a sinon sandbox been defined elsewhere in scope? If so, I would prefer we use that- otherwise this LGTM.

[VictorHom]

there isn't a sinon sandbox used elsewhere in the file, and just to double check, are you referring to sandbox as in http://sinonjs.org/releases/v1.17.7/sandbox/ ? @platinumazure

[platinumazure]

Yes, that's the one. If one isn't being used elsewhere, I won't insist that you use one in this PR- we can always incorporate sandboxes later in a chore PR. Thanks for confirming!

[platinumazure]

LGTM, comments have been addressed.

[not-an-aardvark]

Thanks for contributing!