Do not remove unlinted files from cache if --cache is used

[IanVS]

First, here's a little background on the problem I'm trying to solve:

I run a linting step on my entire project prior to every commit. I use --cache to speed up linting and check only the files which have changed. So far so good.

However, between commits I also am saving code in Atom, using linter-eslint's auto-fix on save option. This executes ESLint without the --cache flag, so my cache is cleared and the next time I manually lint the cache has to be rebuilt, taking much longer than I'd like.

So, I thought I could just add an option in linter-eslint to add the --cache flag (AtomLinter/linter-eslint#635), but I realized this still wouldn't work, because the cache file will be completely overwritten with the cache results of just the one file that was saved/auto-fixed. Aside: to verify this is the case, lint two files, check that the .eslintcache file contains both filenames, then lint just one of those files (still using --cache), and you'll see that the other file has disappeared.

So my proposal is this:

Instead of throwing away the old cache file each time, simply update the results for files that are linted in that run. This way, files which were previously linted stay cached. I have submitted jaredwray/file-entry-cache#2 upstream to add that possibility in the package we use for our caching, and if that is accepted the only change to ESLint would likely be to update the dependency.

If the upstream change is not accepted, there are still ways we could accomplish the same result (albeit less cleanly) if the team thinks this would be a worthwhile change.

[royriojas]

Hi @IanVS, I remembered the reason I didn't keep the files, it is because otherwise files that were deleted will stay forever in the cache. I might have to find another solution to make sure files are not kept in the cache forever if they are not longer present. Or maybe that's not an issue and users can simply delete the files manually. It seems the benefits of keeping the files are greater than deleting them.

Will try to give it a try during the weekend.

Regards

[IanVS]

I hadn't thought of purging deleted files, that's a good point. I guess the only real downside of keeping them around forever is filesize and processing time of the cache file, yes? Those seem to me to be fairly small penalties, in the grand scheme of things. I suppose you could also check for files and purge missing ones each time the cache is used, although I'm not sure of the performance penalty for that.

Thanks for taking a crack at it, I'm excited to see what you come up with. :)

[pmcelhaney]

What happens when the config is updated? Does the cache file become invalidated at that point?

If I was going to use this feature (and I probably will if the mentioned problem with Atom is resolved), I would probably add an npm install and/or git checkout hook to delete the cache file on a regular basis. So the size of the cache file would never become a problem.

[IanVS]

Yes, the cache is invalidated if config is changed.

[royriojas]

@IanVS I added the option on the underlaying dependencies. I was thinking that maybe we need to make that optional (if we want to keep non visited keys). This can be done like:

eslint --cache --cache-prune 'src/**/*.js'

Or would you suggest to just make this the default behavior so no new option is added?

if that is the case the only thing left is to do this:

- cache.reconcile();
+ cache.reconcile(true /* noPrune */) 

Let me know I can just make the PR.

[ilyavolodin]

It sounds reasonable to me. I can't say that I love the idea of adding yet another CLI flag, but I'm not sure I see any other options. @eslint/eslint-tsc thoughts?

[IanVS]

Instead of an option (which I also would like to avoid), couldn't we (or file-entry-cache) just always iterate through the files in the cache and check that they exist? Or, would that be too much of a performance hit?

[ilyavolodin]

I would imagine on a large enough repository where you use cache to improve performance, it will take a while to go through every file...

[IanVS]

According to nodejs/node-v0.x-archive#6662 (comment), it seems like stating a million files takes roughly a second (or did 2 years ago). We could also be a little smart and only check for files which weren't linted, which might cut the number significantly. All this is speculation though, of course.

[royriojas]

Sure, I don't anticipate files to grow too much out of control, though. In this case is also pretty easy to just delete the cache file. Prune might also be able to check if files exists and then decide to remove them from the cache. That can probably be added as an option on the file-entry-cache module to avoid adding another flag on eslint. Something that can be done with:

cache.reconcile( { pruneRemoved: true } );

at saving time, entries that were not visited can be check for existence in the file system and be removed if they are not longer found. Will give that a try as well.

[nzakas]

If we can do it without a significant perf hit, I'm 👍 .

[IanVS]

Cool. Does anyone have opinions on whether this is a bug (and therefore able to be accepted now), or an enhancement requiring TSC approval? Looks like we have a tentative 👍 from @nzakas and @ilyavolodin. Perhaps you could try it out and do a little profiling @royriojas to help with a final decision?

[royriojas]

Sure. I honestly don't think this will have any sensible impact in
performance, but I can do some checks to view how much are we impacted

On Thu, Aug 4, 2016, 3:50 PM Ian VanSchooten notifications@github.com
wrote:

Cool. Does anyone have opinions on whether this is a bug (and therefore
able to be accepted now), or an enhancement requiring TSC approval? Looks
like we have a tentative 👍 from @nzakas https://github.com/nzakas and
@ilyavolodin https://github.com/ilyavolodin. Perhaps you could try it
out and do a little profiling @royriojas https://github.com/royriojas
to help with a final decision?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#6780 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIA2vIpLYWFvV4LaPRVM-FTdOpvlC1Gks5qclCBgaJpZM4JW3Dm
.

[IanVS]

Hi all, just checking in to see where we stand on this issue so it doesn't sit stale too long.

[royriojas]

@IanVS I had made a PR with the new version and tests, please let me know what you think.

[IanVS]

@ilyavolodin, @nzakas please see #6921 (comment) for the results of some timings that I did. TL;DR: checking for deleted files is pretty inconsequential (< 1% of total linting time, ~12 milliseconds for node.js repo).

[ilyavolodin]

That sounds good to me. 👍

[IanVS]

Great, I'm going to accept this as a bug fix, then. Of course push back if that's not how this should be treated.