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

Docs: explain whether type-aware linting is safe to use with eslint --cache #4694

Closed
six-transit opened this issue Mar 17, 2022 · 5 comments · Fixed by #8372
Closed

Docs: explain whether type-aware linting is safe to use with eslint --cache #4694

six-transit opened this issue Mar 17, 2022 · 5 comments · Fixed by #8372
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating

Comments

@six-transit
Copy link

six-transit commented Mar 17, 2022

Suggested Changes

I'd like to use ESLint's --cache functionality with typescript-eslint -- but according to the docs, the latter performs caching on a per-file basis. That suggests to me that it's incompatible with type-aware linting, since I might make a change in one file that would create a new lint in a different file -- one that might not have changed since the previous ESLint run.

typescript-eslint.io doesn't have any info about caching that I've been able to find. So I guess my suggestion for improving the docs would be:

  • if ESLint caching is incompatible with type-aware linting, that should be noted in the docs, since it's a fairly significant caveat.
  • if ESLint caching is compatible with type-aware linting, that seems like a pretty worthwhile thing to note in the docs as a way to speed up checks, maybe in the "Linting with Type Information" or FAQ page.

Affected URL(s)

https://typescript-eslint.io/docs/linting/type-linting/ and/or https://typescript-eslint.io/docs/linting/troubleshooting/

@six-transit six-transit added documentation Documentation ("docs") that needs adding/updating triage Waiting for maintainers to take a look labels Mar 17, 2022
@bradzacher
Copy link
Member

bradzacher commented Mar 17, 2022

From experience - in general ESLint caching can be a real problem across the entire ecosystem.

Unless every single lint rule you use only depends on single-file analysis, you can quickly end up in an inconsistent lint state.
A few quick examples off the top of my head:

  • there are a number of rules in eslint-plugin-import all do their own cross-file analysis (not using TS).
  • eslint-plugin-prettier depends on your .prettierrc file.
  • At Meta we also have a number of internal lint rules that depend on code loaded from source and also on file locations.

I also don't know if the cache respects plugin/config/parser versions - so if you checkout master and that has version changes, I don't know if ESLint would correctly blow away the cache automatically.

In general I would just warn against using the cache, regardless of if you use our tooling or not because you don't really know what external metadata your plugins may or may not depend on and the cache could cause undefined behaviour.

@six-transit
Copy link
Author

six-transit commented Mar 17, 2022

Thanks for the quick response! Yeah, I was wondering whether eslint-plugin-import could also be affected by similar issues.

Anyway, that's too bad -- maybe eventually we'll see an RFC in ESLint for more robust expression of dependencies that could be used for cache keying? (Poking around I see you've already commented in eslint/eslint#14139 and eslint/eslint#15583.)

@bradzacher
Copy link
Member

--cache is a pretty advanced usecase / optimisation anyway - most people wouldn't even know the flag existed!

TBH I don't think I've ever even seen the flag turned on in a codebase (eslint themselves don't even use it!).

I don't know if there is a very good spot to put this anywhere in our docs. The getting started guide is targeted at "noobs" so adding it there would be very superfluous and unuseful for those types of users. The FAQ is a potential spot but it also isn't actually a FAQ - this is the first question I can remember since we started the project.

So I think for now we will leave it undocumented. In future if we get more interest or issues we can consider explicitly mentioning not to use the feature.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Dec 18, 2023

@bradzacher I think we should reopen this and mark it as accepting PRs. https://sourcegraph.com/search?q=context%3Aglobal+eslint+--cache&patternType=standard&sm=1&groupBy=repo shows many projects using it, including at time of search: bootstrap, freecodecamp, langchain, mermaid-js, react-router, storybook, webpack, and vite. Plus anecdotally I've had a few chats with private companies that were using it in the hopes of improving performance.

Edit: oh, and #6373 was opened too.

Suggestion: let's add a brief entry under https://typescript-eslint.io/linting/troubleshooting/performance-troubleshooting with a title like "eslint --cache not improving performance times" explaining that type checked linting busts the caching mechanism?

@typescript-eslint typescript-eslint unlocked this conversation Dec 18, 2023
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Jan 6, 2024
@JoshuaKGoldberg
Copy link
Member

Coming back to this: I think this is more of a general issue, not just a performance one. I tried out --cache locally and it just wholeheartedly broke with type rules. I'll send an issue for the general troubleshooting docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants