New: Add option to allow use of file contents for cache

[c-home]

Summary

This RFC proposes improving cache for CI by providing an option to use a hash (instead of mtime and fsize) comparison to determine whether a file has changed.

Related Issues

• use checksum instead of mtime for caching to enable CI caching eslint#11319
• Provide users with a way to customize how changes to files are detected for caching purposes eslint#11490
• New: Add --cache-strategy CLI option (refs #11319) eslint#11487
• https://github.com/royriojas/file-entry-cache/pull/14

[jsf-clabot]

All committers have signed the CLA.

[anikethsaha]

It looks like the alternative approach (https://github.com/royriojas/file-entry-cache/pull/14) will be merged soon.

If that is the case, IMO this one will be a better choice?

[nzakas]

I like the idea, and I think it would be helpful to include more details in the detailed design section explaining how it would be implemented.

[nzakas]

Can you please describe the details of the implementation here? Which files would be changed?

[fa93hws]

It looks like the alternative approach (royriojas/file-entry-cache#14) will be merged soon.

If that is the case, IMO this one will be a better choice?

I don't feel setting environmental variable to control a library is a good idea.
It means the api is dependent on the implementation of a library, which may not be a good practice

i.e. Having an environmental variable to control eslint is fine, but having an environmental variable to control a library consumed by eslint maybe inappropriate from my opinion.

[anikethsaha]

I don't feel setting environmental variable to control a library is a good idea.
It means the api is dependent on the implementation of a library, which may not be a good practice

i.e. Having an environmental variable to control eslint is fine, but having an environmental variable to control a library consumed by eslint maybe inappropriate from my opinion.

Thanks for the input.

personally, I don't think there is any downside of controlling the library

Let me know if I am missing anything.

[nzakas]

I agree with @fa93hws - setting environment variables to control how a dependency works is a bit too magical. ESLint should control how it works on its own.

[nzakas]

Seems like a fairly simple implementation for a big win. I'm in favor.

[anikethsaha]

🎉

[mdjermanovic]

I also agree with @fa93hws.

royriojas/file-entry-cache#14 looks like a nice workaround in the absence of ESLint's option, but as a permanent solution - would we document FILE_ENTRY_CACHE_USE_CHECKSUM env variable?

If not, users will be unaware of it. If yes, things will be complicated if we'll need to switch to some other cache lib at some point (for whatever reason).

[nzakas]

would we document FILE_ENTRY_CACHE_USE_CHECKSUM env variable?

I don’t see that as a viable solution. End users shouldn’t know or care about which dependencies ESLint uses.

[kaicataldo]

Thanks for working on this!

[c-home]

Can this be moved to the final commenting phase?

[nzakas]

Per the last comment, moving into the Final Commenting phase.

[mdjermanovic]

It seems that the useCheckSum option in fileEntryCache#create was designed to be used consistently with the same cache file: either always false or always true.

In particular, useCheckSum = true doesn't remove previously stored size and mtime. In a similar way, useCheckSum = false doesn't remove previously stored hash.

I don't think this should be a blocker for this feature, but some unexpected results could happen if user switches between cache strategies, but doesn't delete the cache file, since a combination of the stored metadata and stored linting results might be invalid.

To reproduce:

1. Make a file without linting errors, and run eslint with useCheckSum = true.
2. Change the file to produce a linting error, and run with useCheckSum = false.
3. Undo the changes and run again with useCheckSum = true. It will mistakenly show the linting error from step 2.

[nzakas]

@mdjermanovic that's a good point, and I think we can probably address that either by detecting the type of cache file in use and invalidating it if the cache strategy has changed or we just add a field into the cache file that explicitly states which strategy was used.

[mdjermanovic]

I think we can probably address that either by detecting the type of cache file in use and invalidating it if the cache strategy has changed or we just add a field into the cache file that explicitly states which strategy was used.

Good idea! I'm not sure if file-entry-cache provides a documented API for this, but the solution with a custom field seems doable. We're already storing a hash of the applied eslint config, and other custom metadata. I guess we could figure out the details in the PR.

[mdjermanovic]

Just to note that in addition to any comparisons file-entry-cache is doing, we're also manually invalidating cache if any of the following has changed: ESLint configuration for the given file, ESLint version, Node version.

https://github.com/eslint/eslint/blob/66442a9faf9872db4a40f56dde28c48f4d02fc7b/lib/cli-engine/lint-result-cache.js#L32

If that's relevant for this RFC, it means that the same cache file still cannot be reused between different Node versions, even if --cache-strategy contents was used.