fix(typescript-estree): only run projectService setHostConfiguration when needed #9336
Conversation
…tService Calling on every source causes a full graph update. Relocating to call once per ProjectService.
|
Thanks for the PR, @higherorderfunctor! 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. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e081759. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Tentatively looks lovely to me, with the refactor suggestion factored in. I'll take a deeper look once I have a chance to play with a repro.
cc @jakebailey as an FYI for this & the issue 🙂
packages/typescript-estree/src/create-program/createProjectService.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the initial review. I did discover one bug that is fixed in my branch with a collection of changes. If a user changes extensions from the initial ones, they do not get updated. I'll carry the fix in and add a test. |
|
Taking out of draft. Notable updates:
This is part of a larger set of changes discussed on #9312 (comment) and #8835 (comment).
|
…ed and clean up impl Fixes the test where it should not update to update twice with the same values to demonstrate the cache worked. Changes the cache to not update until successfully applied and cleans up the implementation a bit.
…tensions tests Empty or not provided extraFileExtensions are handled differently depending on the state of ProjectService. Clarified test names and added additional tests to reflect this.
|
Made the reordering and trimming changes requested. I also updated the links with the title change in the performance section. Let me know if any further edits are needed! |
There was a problem hiding this comment.
Lovely, thanks for this! 🙌
I just have docs nitpicks left over. We'll make sure this is merged before our Monday release in two days. 🚀 thanks a bunch @higherorderfunctor!
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
|
All review suggestions have been committed. Thanks again! |
|
Inspecting pipeline errors and some of the links aren't rendering locally. Inspecting now. |
|
Fixes made. So long as the pipeline succeeds, we should be good to merge! |
There was a problem hiding this comment.
🔥 ! Very excited to get this released, thank you! A bit of a relief to fix such a disruptive performance issue for Vue users.
I'll merge this in now, merge #9024 into main, and then merge main into v8 - so all the FAQs including this one will be nicely organized.
PR Checklist
Overview
MovesSee comments regarding updated implementation.ProjectService.setHostConfigurationfromuseProgramFromProjectServicewhere it is called on every source tocreateProjectServicewhere it is only called once perProjectService.ProjectService.setHostConfigurationreloads the whole project. The wayvue-eslint-parsersends template fragments causes a reload to start and then get canceled for n - 1 template fragments per file.This fix along with #9223 if using
eslint-plugin-import/eslint-plugin-import-xvastly improves performance.I am open to suggestions on improving the signature ofcreateProjectService. I appended as a new 3rd argument calledparseSettingswhich is an object to allow for additional settings to be added in the future if needed. I could see this being merged with the second argument, but I was not sure of the implications in making a breaking change to the existing signature.