Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(typescript-estree): only run projectService setHostConfiguration when needed#9336
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…tServiceCalling on every source causes a full graph update. Relocating to callonce 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJun 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJun 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
☁️ Nx Cloud ReportCI is running/has finished running commands for commite081759. 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 💌 fromNxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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. |
higherorderfunctor commentedJun 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 implFixes the test where it should not update to update twice with the samevalues to demonstrate the cache worked.Changes the cache to not update until successfully applied and cleans upthe implementation a bit.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…tensions testsEmpty or not provided extraFileExtensions are handled differentlydepending on the state of ProjectService. Clarified test names andadded additional tests to reflect this.
Uh oh!
There was an error while loading.Please reload this page.
JoshuaKGoldberg left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great to me, thanks! 🙌
Mostly touching up on the docs FAQs. I think they're pretty close, just need some reordering and trimming.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🔥 ! 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 intomain
, and then mergemain
intov8
- so all the FAQs including this one will be nicely organized.
d1ddbc2
intotypescript-eslint:v8Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
MovesSee comments regarding updated implementation.ProjectService.setHostConfiguration
fromuseProgramFromProjectService
where it is called on every source tocreateProjectService
where it is only called once perProjectService
.ProjectService.setHostConfiguration
reloads the whole project. The wayvue-eslint-parser
sends template fragments causes a reload to start and then getcanceled forn - 1 template fragments per file.This fix along with#9223 if using
eslint-plugin-import
/eslint-plugin-import-x
vastly improves performance.I am open to suggestions on improving the signature ofcreateProjectService
. I appended as a new 3rd argument calledparseSettings
which 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.