feat: speed up non-type-aware linting with project service #8322
Conversation
|
Thanks for the PR, @yepitschunked! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -127,6 +127,7 @@ export function createParseSettings( | |||
| DEFAULT_TSCONFIG_CACHE_DURATION_SECONDS, | |||
| )), | |||
| tsconfigRootDir, | |||
| typeAware: !!options.programs || !!options.project, | |||
There was a problem hiding this comment.
I noticed that hasFullTypeInformation was checking whether programs or projects were empty. However, this will always be the case when using the project service. The docs seem to point to using programs and project (singular) as the way to enable type-aware linting, so I decided to make this explicit here.
|
👋 thanks for sending this over @yepitschunked! I see from your GitHub history that you have a good history of sending nice paper cut / performance fixes to projects. Exciting that you're looking at typescript-eslint. 😄 But:
We ask that people file issues for non-trivial changes like this one for a few reasons:
Do you have the time to file an issue? For visibility, https://antfu.me/posts/why-reproductions-are-required is a good read on why I'm asking for a reproduction in particular. |
|
Note that I tried this out on #8424 and it seemed to nicely fix Sentry's
Very nicely done @yepitschunked! I need to do some more research to figure out whether there's a more root-level fix to be done, but my gut says the work here won't go to waste. |
For non-IDE use cases, we definitely would need a repro. I'm not familiar with Webpack plugins, etc. enough to evaluate this. Note that For IDE use cases: tracked for VS Code in microsoft/vscode-eslint#1774. (again just posting as reference) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v8 #8322 +/- ##
==========================================
+ Coverage 87.93% 87.94% +0.01%
==========================================
Files 401 401
Lines 13575 13585 +10
Branches 3941 3943 +2
==========================================
+ Hits 11937 11948 +11
Misses 1325 1325
+ Partials 313 312 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| : `\n${relativeProjects.map(project => `- ${project}`).join('\n')}`; | ||
| const errorLines = [ | ||
| `ESLint was configured to run on \`${describedFilePath}\` using \`parserOptions.project\`: ${describedPrograms}`, | ||
| `ESLint was configured to run on \`${describedFilePath}\` using \`parserOptions.project\`:${describedPrograms}`, |
There was a problem hiding this comment.
Not strictly needed for this PR, but it was so annoying having Prettier auto-save the snapshot to no longer have a trailing slash...
| parseSettings.programs != null || parseSettings.projects.size > 0; | ||
| parseSettings.programs != null || | ||
| parseSettings.projects.size > 0 || | ||
| !!parseSettings.projectService; |
There was a problem hiding this comment.
@yepitschunked following up on parserSettings.typeAware: whether the file is being parsed with type information is still inferable from existing parseSettings properties. So rather than refactor to add a new property, this just corrects the logic for const hasFullTypeInformation.
There was a problem hiding this comment.
@yepitschunked I ended up refactoring this file a good bit to move secondary logic into functions. It got rather difficult to read through once I added in handling for the new "default project".
packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts
Outdated
Show resolved
Hide resolved
packages/typescript-estree/tests/lib/useProgramFromProjectService.test.ts
Outdated
Show resolved
Hide resolved
| ${filesToPrint.map(file => `- ${file}`).join('\n')} | ||
| ${truncatedFileCount ? `...and ${truncatedFileCount} more files\n` : ''} | ||
| If you absolutely need more files included, set parserOptions.projectService.maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING to a larger value. | ||
| `, |
There was a problem hiding this comment.
Note that I've moved this "Too many files ..." notice to inside the if (opened.configFileName) in this PR. That should stop the notice from getting logged on all files!
|
cc @bradzacher - I think this is ready for re-review. |
PR Checklist
Overview
This PR fixes two issues with the experimental project service option.
Faster non-type-aware linting
The current implementation opens all files using the project service, even if the file is not opted into type-aware linting (with the
projectorprogramsoptions). This makes linting slower than necessary, since project for every file will be loaded. Of course, you could also just omit theEXPERIMENTAL_useProjectServiceoption, but I think that:This PR skips opening files with the project service if
hasFullTypeInformationis false, matching the existing behavior.Refresh non-type-aware files with project service
Due to the performance tradeoffs of type-aware linting, we currently only enable it on specific files/folders in our repo. However, this means that Typescript is never informed of changes to files outside of those locations (due to typescript-eslint hijacking the watch mechanism). We end up linting with stale type information if we depend on any types that are defined outside of these locations.
When opening non-type-aware files, this PR checks if the project service is already aware of this file
This approach ensures that if you only have non-type-aware files opened in the IDE, linting never creates a program and remains fast. If you open a mixture of files, then the project service is still informed of changes to non-type-aware files. There's of course some reliance on ordering here, but this is already a known/wontfix problem (e.g #4733), and shouldn't affect IDE use cases.