Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

higherorderfunctor
Copy link
Contributor

@higherorderfunctorhigherorderfunctor commentedJun 11, 2024
edited
Loading

PR Checklist

Overview

MovesProjectService.setHostConfiguration fromuseProgramFromProjectService where it is called on every source tocreateProjectService where it is only called once perProjectService. See comments regarding updated implementation.

ProjectService.setHostConfigurationreloads 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 usingeslint-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.

…tServiceCalling on every source causes a full graph update.  Relocating to callonce per ProjectService.
@typescript-eslint
Copy link
Contributor

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.

@netlifyNetlify
Copy link

netlifybot commentedJun 11, 2024
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commite081759
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6677155e685e6a0009479df1
😎 Deploy Previewhttps://deploy-preview-9336--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🟢 up 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedJun 11, 2024
edited
Loading

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 fromNxCloud.

@higherorderfunctorhigherorderfunctor changed the titleperf(typescript-estree): moves setHostConfiguration into createProjectServicefix(typescript-estree): moves setHostConfiguration into createProjectServiceJun 11, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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 🙂

@higherorderfunctor
Copy link
ContributorAuthor

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

higherorderfunctor commentedJun 14, 2024
edited
Loading

Taking out of draft. Notable updates:

  • Discovered a bug when changingextraFileExtensions in the same linting session.
    • Added positive and negative test cases for this situation.
  • Moved the logic back topackages/typescript-estree/src/useProgramFromProjectService.ts.
  • The approach now caches the last setextraFileExtensions and only callssetHostConfiguration when there is a difference.
    • I'm caching directly on theProjectService with__extra_file_extensions so it gets garbage collected withclearTSServerProjectService. If you prefer a proper cleanup with it's own function, I can do so, but it may take a few days to find availability.
    • Using sets yielded the best performance when testing hence thesymmetricDifference logic. I did not exhaustively look at other options, just some tests iterating with for-loops. Let me know if you want a different approach.
  • I've added documentation to the site explaining how to minimize project reloads by settingextraFileExtensions only once.

This is part of a larger set of changes discussed on#9312 (comment) and#8835 (comment).

@higherorderfunctorhigherorderfunctor changed the titlefix(typescript-estree): moves setHostConfiguration into createProjectServicefix(typescript-estree): changes setHostConfiguration to only run when neededJun 14, 2024
@higherorderfunctorhigherorderfunctor marked this pull request as ready for reviewJune 14, 2024 03:01
…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.
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg 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 direction! Just requesting changes on docs & some of the implementation bits.

Cat head shaking on what looks like a hyperspeed background, implying high speed

…tensions testsEmpty or not provided extraFileExtensions are handled differentlydepending on the state of ProjectService.  Clarified test names andadded additional tests to reflect this.
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment
edited
Loading

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.

@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJun 20, 2024
@higherorderfunctor
Copy link
ContributorAuthor

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!

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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!

higherorderfunctorand others added5 commitsJune 22, 2024 18:01
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>
@higherorderfunctor
Copy link
ContributorAuthor

All review suggestions have been committed. Thanks again!

@higherorderfunctor
Copy link
ContributorAuthor

Inspecting pipeline errors and some of the links aren't rendering locally. Inspecting now.

@higherorderfunctor
Copy link
ContributorAuthor

Fixes made. So long as the pipeline succeeds, we should be good to merge!

@JoshuaKGoldbergJoshuaKGoldberg changed the titlefix(typescript-estree): changes setHostConfiguration to only run when neededfix(typescript-estree): only run projectService setHostConfiguration when neededJun 22, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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.

higherorderfunctor reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg merged commitd1ddbc2 intotypescript-eslint:v8Jun 22, 2024
64 of 65 checks passed
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJul 1, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jakebaileyjakebaileyjakebailey left review comments

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@higherorderfunctor@JoshuaKGoldberg@jakebailey

[8]ページ先頭

©2009-2025 Movatter.jp