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

feat: speed up non-type-aware linting with project service#8322

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

@yepitschunked
Copy link
Contributor

@yepitschunkedyepitschunked commentedJan 30, 2024•
edited by JoshuaKGoldberg
Loading

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 theproject orprograms options). This makes linting slower than necessary, since project for every file will be loaded. Of course, you could also just omit theEXPERIMENTAL_useProjectService option, but I think that:

  • this option should eventually become the default, so it won't be configurable
  • enabling project service on non-type-aware linting also helps interop with type-aware linting (see next section)

This PR skips opening files with the project service ifhasFullTypeInformation is 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

  • If so, this means this file is part of a program used in type-aware linting, and we should open it with the project service to update it
  • If not, then we can fall back to the fast path (described in the previous section).

This approach ensures that if youonly 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.

StyleShit reacted with rocket emoji
@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint.

@netlify
Copy link

netlifybot commentedJan 30, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit80dbb38
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/669ae9406c7a410008976a78
😎 Deploy Previewhttps://deploy-preview-8322--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: 96 (🟢 up 2 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.

DEFAULT_TSCONFIG_CACHE_DURATION_SECONDS,
)),
tsconfigRootDir,
typeAware:!!options.programs||!!options.project,
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I noticed thathasFullTypeInformation was checking whetherprograms orprojects were empty. However, this will always be the case when using the project service. The docs seem to point to usingprograms andproject (singular) as the way to enable type-aware linting, so I decided to make this explicit here.

@yepitschunkedyepitschunked changed the titlespeed up non-type-aware linting with project servicefeat: speed up non-type-aware linting with project serviceJan 30, 2024
@JoshuaKGoldberg
Copy link
Member

👋 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:

  • The templates ask reporters to provide info we'd need to triage requests, including a full reproduction - which is also useful if we later have to spelunk through the repository's history
  • Sometimes the suggested changes are a duplicate of existing issues or PRs, and/or end up splitting into multiple issues
  • We want to raise visibility for the changes to let folks have a chance to discuss nuances before work is done
  • Even if you're at the 'expert' level where specifically you know to check for duplicatesand understand the codebase well enough to make these changes, most users aren't - so it sets a negative precedent

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.

@JoshuaKGoldbergJoshuaKGoldberg added awaiting responseIssues waiting for a reply from the OP or another party please fill out the templatewe have the processes for good reasons 😔 labelsFeb 3, 2024
@JoshuaKGoldberg
Copy link
Member

Note that I tried this out on#8424 and it seemed to nicely fix Sentry'stime yarn lint:js:

  • Not using project service:✨ Done in 35.34s. \n yarn lint:js 50.11s user 1.47s system 145% cpu 35.472 total
  • Without this change:✨ Done in 57.70s. \n yarn lint:js 78.63s user 5.09s system 144% cpu 57.814 total
  • With this change:✨ Done in 36.23s. \n yarn lint:js 50.69s user 1.53s system 143% cpu 36.354 total

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.

@JoshuaKGoldberg
Copy link
Member

Refresh non-type-aware files with project service
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.

For non-IDE use cases, we definitely would need a repro. I'm not familiar with Webpack plugins, etc. enough to evaluate this. Note thateslint --cache is not something we recommend:#4694. (it's not mentioned here, just posting as reference)

For IDE use cases: tracked for VS Code inmicrosoft/vscode-eslint#1774. (again just posting as reference)

@JoshuaKGoldbergJoshuaKGoldberg changed the base branch frommain tov8July 19, 2024 22:23
@codecov
Copy link

codecovbot commentedJul 19, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.94%. Comparing base(a3230a9) to head(80dbb38).

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
FlagCoverage Δ
unittest87.94% <100.00%> (+0.01%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

FilesCoverage Δ
...-estree/src/create-program/createProjectProgram.ts100.00% <ø> (ø)
...escript-estree/src/useProgramFromProjectService.ts100.00% <100.00%> (+1.96%)⬆️

@JoshuaKGoldbergJoshuaKGoldberg added team assignedA member of the typescript-eslint team should work on this. and removed awaiting responseIssues waiting for a reply from the OP or another party labelsJul 19, 2024
@JoshuaKGoldbergJoshuaKGoldberg added this to the8.0.0 milestoneJul 19, 2024
:`\n${relativeProjects.map(project=>`-${project}`).join('\n')}`;
consterrorLines=[
`ESLint was configured to run on \`${describedFilePath}\` using \`parserOptions.project\`:${describedPrograms}`,
`ESLint was configured to run on \`${describedFilePath}\` using \`parserOptions.project\`:${describedPrograms}`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not strictly needed for this PR, but it wasso 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@yepitschunked following up onparserSettings.typeAware: whether the file is being parsed with type information is still inferable from existingparseSettings properties. So rather than refactor to add anew property, this just corrects the logic forconst hasFullTypeInformation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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".

@nx-cloud
Copy link

nx-cloudbot commentedJul 19, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit80dbb38. 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 31 targets

Sent with 💌 fromNxCloud.

@JoshuaKGoldbergJoshuaKGoldberg removed the please fill out the templatewe have the processes for good reasons 😔 labelJul 20, 2024
${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.
`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Note that I've moved this"Too many files ..." notice toinside theif (opened.configFileName) in this PR. That should stop the notice from getting logged on all files!

@JoshuaKGoldberg
Copy link
Member

cc@bradzacher - I think this is ready for re-review.

@JoshuaKGoldbergJoshuaKGoldberg merged commitfd22484 intotypescript-eslint:v8Jul 21, 2024
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJul 29, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

@bradzacherbradzacherbradzacher approved these changes

Assignees

No one assigned

Labels

team assignedA member of the typescript-eslint team should work on this.

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

⚡️ Performance: Don't open client files unnecessarily for project service

3 participants

@yepitschunked@JoshuaKGoldberg@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp