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(typescript-estree): add EXPERIMENTAL_useProjectService option to use TypeScript project service#6754

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

@JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg commentedMar 24, 2023•
edited
Loading

PR Checklist

Overview

Creates an experimental ready-to-try-only-if-you-dare prototype implementation of#6575: using TypeScript's "project service" / server to create programs instead of our existing manual project creation. Doing so brings the"create program(s) for source files" part of typed linting towards how editors behave:

// .eslintrc.jsmodule.exports={extends:['eslint:recommended','plugin:@typescript-eslint/recommended-type-checked',],plugins:['@typescript-eslint'],parser:'@typescript-eslint/parser',parserOptions:{EXPERIMENTAL_useProjectService:true,},root:true,};

Technical Details

In essence, this PR:

  • Adds anEXPERIMENTAL_useProjectService option to typescript-estree
    • Settingprocess.env.TYPESCRIPT_ESLINT_EXPERIMENTAL_TSSERVER can also enable the option
  • AdjustscreateParserSettings to call to a newcreateProjectService function lazily when the experimental option is enabled (thereby settingparseSettings.EXPERIMENTAL_projectService)
  • AdjustsgetProgramAndAST to use that project service if it exists, with a newuseProgramFromProjectService
  • Massages internal tests to always use absolute (or failing that, relative to project root) paths for files if possible, as the project service expects that(is this phrasing accurate?)
  • Skips tests that aren't applicable anymore when the project service is in use

Seehttps://github.com/JoshuaKGoldberg/repros/tree/cbe9d9c9b19c09b085a0be2024221b2d05753cc8 for a reproduction case showing this working.

Performance Comparison

Complete Project Linting

forbranchNamein v6 create-project-service;do    git checkout$branchName    yarn    hyperfine"yarn lint --skip-nx-cache">$branchName.txtdone hyperfine"TYPESCRIPT_ESLINT_EXPERIMENTAL_TSSERVER=true yarn lint --skip-nx-cache"> enabled.txt
v6#6754 baseline#6754 enabled#6754 Δ
62.976 s ± 0.479 s65.383 s ± 0.226 s31.169 s ± 0.401 s-52% 🟢

Incremental Project Linting

forglobin"eslint-plugin/src/rules/*.ts""typescript-estree/src/create-program/*.ts""website/**/*.tsx";doforsettinginfalsetrue;do        hyperfine"TYPESCRIPT_ESLINT_EXPERIMENTAL_TSSERVER=$setting npx eslint --config .eslintrc.js packages/$glob"donedone
GlobBaselineEnabledΔ
eslint-plugin/src/rules/*.ts8.552 s ± 0.100 s9.525 s ± 0.084 s+11% ❌
packages/typescript-estree/src/create-program/*.ts7.162 s ± 0.079 s2.153 s ± 0.007 s-70% 🟢
packages/website/**/*.tsx9.955 s ± 0.080 s3.714 s ± 0.029 s-63% 🟢

swandir, haltcase, jakebailey, paulmelero, tonivj5, felixfbecker, and rdsedmundo reacted with thumbs up emojithetric reacted with heart emojipaulmelero, tonivj5, and huuduy005 reacted with rocket emojihaltcase, tonivj5, and jaydenseric reacted with eyes emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@JoshuaKGoldberg!

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 commentedMar 24, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit7e74202
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/64b3315b3783730008dbc7af
😎 Deploy Previewhttps://deploy-preview-6754--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@nx-cloud
Copy link

nx-cloudbot commentedMar 24, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit7e74202. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 35 targets

Sent with 💌 fromNxCloud.

@bradzacherbradzacher added the DO NOT MERGEPRs which should not be merged yet labelApr 10, 2023
@bradzacherbradzacher added the enhancementNew feature or request labelApr 17, 2023
@JoshuaKGoldbergJoshuaKGoldberg changed the titleWIP: createProjectServicefeat(typescript-estree): add EXPERIMENTAL_useProjectService option to use TypeScript project serviceApr 18, 2023
@codecov
Copy link

codecovbot commentedApr 18, 2023•
edited
Loading

Codecov Report

Merging#6754 (7e74202) intomain (606a52c) willdecrease coverage by0.11%.
The diff coverage is62.50%.

Additional details and impacted files
@@            Coverage Diff             @@##             main    #6754      +/-   ##==========================================- Coverage   87.57%   87.47%   -0.11%==========================================  Files         377      379       +2       Lines       13201    13230      +29       Branches     3901     3906       +5     ==========================================+ Hits        11561    11573      +12- Misses       1261     1279      +18+ Partials      379      378       -1
FlagCoverage Δ
unittest87.47% <62.50%> (-0.11%)⬇️

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

Impacted FilesCoverage Δ
...escript-estree/src/useProgramFromProjectService.ts16.66% <16.66%> (ø)
...t-estree/src/create-program/useProvidedPrograms.ts84.84% <66.66%> (ø)
...pt-estree/src/parseSettings/createParseSettings.ts81.81% <71.42%> (+0.18%)⬆️
...-estree/src/create-program/createProjectService.ts80.00% <80.00%> (ø)
packages/eslint-plugin-tslint/src/rules/config.ts97.77% <100.00%> (+0.05%)⬆️
packages/typescript-estree/src/clear-caches.ts100.00% <100.00%> (ø)
...-estree/src/create-program/createProjectProgram.ts97.56% <100.00%> (-0.12%)⬇️
.../src/create-program/getWatchProgramsForProjects.ts82.12% <100.00%> (-2.71%)⬇️
...ges/typescript-estree/src/create-program/shared.ts77.50% <100.00%> (ø)

... and1 file with indirect coverage changes

@JoshuaKGoldberg
Copy link
MemberAuthor

Re-requesting review from@bradzacher because I applied a couple of small touchups & answered a question. But otherwise I feel comfortable merging this after we get v6 out the door (just to be safe & not conflict with the v6 release). Woop woop! 🚀

bradzacher
bradzacher previously approved these changesJun 15, 2023
Copy link
Member

@bradzacherbradzacher left a comment

Choose a reason for hiding this comment

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

acutmore and exodus4d reacted with rocket emoji
@JoshuaKGoldbergJoshuaKGoldberg removed this from the6.0.0 milestoneJun 15, 2023
@bradzacherbradzacher added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJun 24, 2023
@JoshuaKGoldbergJoshuaKGoldberg deleted the branchtypescript-eslint:mainJuly 10, 2023 17:52
@ritschwumm
Copy link

may i ask what happened with this PR - is it merged now, or had it to be abandoned?

@JoshuaKGoldberg
Copy link
MemberAuthor

JoshuaKGoldberg commentedJul 10, 2023•
edited
Loading

This was unintentionally auto-closed when we merged thev6 branch 🙃 it'll berecreated reopened.

ritschwumm reacted with heart emoji

@JoshuaKGoldbergJoshuaKGoldberg changed the base branch fromv6 tomainJuly 10, 2023 21:11
@JoshuaKGoldbergJoshuaKGoldberg dismissedbradzacher’sstale reviewJuly 10, 2023 21:11

The base branch was changed.

@JoshuaKGoldberg
Copy link
MemberAuthor

JoshuaKGoldberg commentedJul 10, 2023•
edited
Loading

Now thatv6 is merged intomain, I'm planning on merging this as well some time in the next 1-2 weeks. Depending on whether there are any major issues reported with6.0.0:

  • If there are, we'll fix them in next week's release (July 17th) and I'll plan to merge this this in time for the following week (July 24th)
  • If there aren't, I'll plan to merge this in time for next week's release (July 17th)

Note that we're keeping theEXPERIMENTAL prefix on the option name for now. It's very new and scary. We're also going to want to think a bit more deeply on how theparserOptions type shape should look.

FYI@jakebailey - we're moving quickly this month! ⚡

isaacparker-at reacted with heart emoji

@JoshuaKGoldberg
Copy link
MemberAuthor

https://github.com/typescript-eslint/typescript-eslint/milestone/10 has a small enough number of bugs -and no major architectural issues- reported that I feel comfortable merging this in. Hooray! 🚀

(knocking on wood, fingers crossed 🤞)

swandir reacted with hooray emoji

@JoshuaKGoldbergJoshuaKGoldberg merged commit6d3d162 intotypescript-eslint:mainJul 16, 2023
@JoshuaKGoldberg
Copy link
MemberAuthor

This change terrifies me.

Tom Hanks in a captain's outfit saying 'May God have mercy on my soul'

jakebailey reacted with laugh emoji

Comment on lines +196 to +226
unit_tests_tsserver:
name:Run Unit Tests with Experimental TSServer
needs:[build]
runs-on:ubuntu-latest
strategy:
matrix:
package:
[
'eslint-plugin',
'eslint-plugin-internal',
'eslint-plugin-tslint',
'typescript-estree',
]
env:
COLLECT_COVERAGE:false
steps:
-name:Checkout
uses:actions/checkout@v3
with:
fetch-depth:2
-name:Install
uses:./.github/actions/prepare-install
with:
node-version:18
-name:Build
uses:./.github/actions/prepare-build
-name:Run unit tests for ${{ matrix.package }}
run:npx nx test ${{ matrix.package }} --coverage=false
env:
CI:true
TYPESCRIPT_ESLINT_EXPERIMENTAL_TSSERVER:true
Copy link
Member

Choose a reason for hiding this comment

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

This was a great thing to add!

JoshuaKGoldberg added a commit that referenced this pull requestJul 17, 2023
…ption to use TypeScript project service (#6754)"This reverts commit6d3d162.
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJul 25, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@bradzacherbradzacherbradzacher left review comments

+3 more reviewers

@armano2armano2armano2 left review comments

@jakebaileyjakebaileyjakebailey left review comments

@sheetalkamatsheetalkamatsheetalkamat left review comments

Reviewers whose approvals may not affect merge requirements

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 mergeenhancementNew feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhancement: Try using tsserverlibrary's ProjectService to handle type-aware mode

6 participants

@JoshuaKGoldberg@bradzacher@jakebailey@ritschwumm@armano2@sheetalkamat
[8]ページ先頭
©2009-2025 Movatter.jp