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

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

Merged
merged 3 commits into from Sep 9, 2020

Conversation

c-home
Copy link
Contributor

@c-home c-home commented Jul 20, 2020

@jsf-clabot
Copy link

jsf-clabot commented Jul 20, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Detailed Design

This RFC adds a `--cache-strategy` CLI option. Users can specify the option to be:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nzakas nzakas added enhancement New feature or request Initial Commenting This RFC is in the initial feedback stage and removed triage labels Jul 23, 2020
@fa93hws
Copy link
Contributor

fa93hws commented Jul 27, 2020

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
Copy link
Member

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.

@c-home c-home requested a review from nzakas Jul 27, 2020
@nzakas
Copy link
Member

nzakas commented Jul 28, 2020

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
nzakas approved these changes Jul 28, 2020
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@c-home c-home requested a review from anikethsaha Jul 29, 2020
Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@mdjermanovic
Copy link
Member

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
Copy link
Member

nzakas commented Jul 30, 2020

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.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@c-home
Copy link
Contributor Author

c-home commented Aug 13, 2020

Can this be moved to the final commenting phase?

@nzakas
Copy link
Member

nzakas commented Aug 15, 2020

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

@nzakas nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Aug 15, 2020
@mdjermanovic
Copy link
Member

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
Copy link
Member

nzakas commented Aug 20, 2020

@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
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Final Commenting This RFC is in the final week of commenting
Projects
None yet
8 participants