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): adds support for project services using extended config files#9306

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 8, 2024•
edited
Loading

PR Checklist

Overview

  • Changespackages/typescript-estree/src/create-program/createProjectService.ts to usetsserver.getParsedCommandLineOfConfigFile instead oftsserver.readConfigFile to process extended tsconfigs.
  • Moves thetsserver.getParsedCommandLineOfConfigFile boilerplate frompackages/typescript-estree/src/create-program/useProvidedPrograms.ts into a new filepackages/typescript-estree/src/create-program/getParsedConfigFile.ts to keep the code DRY.
  • Adds tests for single and multiple extends topackages/typescript-estree/tests/lib/createProjectService.test.ts. Also adds a couple tests to round out coverage for uncommon code paths to bringgetParsedConfigFile.ts to 100% (tsserver.sys === undefined and triggeringgetCanonicalFileName withtsserver.formatDiagnostics).
  • Adds a new fixture atpackages/typescript-estree/tests/fixtures/projectServicesComplex/.

A few implementation notes for the reviewer:

  • There is a hardany cast inpackages/typescript-estree/src/create-program/createProjectService.ts. I left a code comment with reasoning for doing so. The originaltsserver.readConfigFile was alsoany with a hard cast.
  • formatDiagnostics in the newpackages/typescript-estree/src/create-program/getParsedConfigFile.ts file was placed insidegetParsedConfigFile. This was only to share the lazy import of tsserver. The existing Jest mocks seem to rely on tsserver being a lazy import. I'm not very familiar with eslint's module import cache-bypassing behavior, so I don't know if there is a performance implication here. The original lazy import is also still present inpackages/typescript-estree/tests/lib/createProjectService.test.ts. We could try adding tsserver as a parameter togetParsedConfigFile to remove the extra lazy import if we think it will be a problem.
  • Switched from a globalmock to adoMock inside abeforeAll for testingtsserver.env === undefined case. I've primarily used mocha/chai, vitest, and bun test; so if it there is a cleaner way to do this in jest I can make that update.
  • All previous unit tests inpackages/typescript-estree/tests/lib/createProjectService.test.ts were truly unit tests mocking all calls to tsserver. Wanting to test the extends behavior, some of the tests now call through to tsserver. Since these are really testing integration functionality, please let me know if adjustments are required. These tests also have slightly looser assertions since tsserver adds some metadata. A code comment has been added to mark each.
  • I did not end up needing to use all of theprojectServicesComplex fixture, specifically the project references. It may have future uses, but I can trim the currently unused project references if preferred.

Inline comments work for me on the PR.

JoshuaKGoldberg reacted with eyes emoji
@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.

@netlify
Copy link

netlifybot commentedJun 8, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit4478aea
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/66a62e98249f920008d167c1
😎 Deploy Previewhttps://deploy-preview-9306--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 (no change 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.

@higherorderfunctorhigherorderfunctor changed the titlefix(typescript-estree): Adds support for project services and extended config filesfix(typescript-estree): adds support for project services and extended config filesJun 8, 2024
@nx-cloud
Copy link

nx-cloudbot commentedJun 8, 2024•
edited
Loading

☁️ Nx Cloud Report

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

@codecov
Copy link

codecovbot commentedJun 8, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is95.83333% with1 line in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base(f3dfc0a) to head(ac8c9bc).
Report is 4 commits behind head on v8.

❗Current headac8c9bc differs from pull request most recent head4478aea

Pleaseupload reports for the commit4478aea to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@##               v8    #9306      +/-   ##==========================================+ Coverage   88.01%   88.04%   +0.03%==========================================  Files         401      402       +1       Lines       13637    13639       +2       Branches     3960     3959       -1     ==========================================+ Hits        12003    12009       +6+ Misses       1325     1322       -3+ Partials      309      308       -1
FlagCoverage Δ
unittest88.04% <95.83%> (+0.03%)⬆️

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

FilesCoverage Δ
...-estree/src/create-program/createProjectService.ts95.34% <100.00%> (+1.87%)⬆️
...t-estree/src/create-program/useProvidedPrograms.ts100.00% <100.00%> (+12.12%)⬆️
...t-estree/src/create-program/getParsedConfigFile.ts93.75% <93.75%> (ø)

... and3 files with indirect coverage changes

@higherorderfunctorhigherorderfunctor marked this pull request as ready for reviewJune 8, 2024 23:25
@higherorderfunctorhigherorderfunctor changed the titlefix(typescript-estree): adds support for project services and extended config filesfix(typescript-estree): adds support for project services using extended config filesJun 10, 2024
@JoshuaKGoldbergJoshuaKGoldberg added this to the8.0.0 milestoneJun 20, 2024
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.

I like the general direction! +1 on extracting out agetParsedConfigFile, that's really useful. 🚀

Requesting changes mostly on reorganization, and therequire()ing too. Cheers!

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelJun 25, 2024
…ts with review feedbackCo-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@JoshuaKGoldberg
Copy link
Member

👋 are you waiting on us for anything@higherorderfunctor?

@higherorderfunctor
Copy link
ContributorAuthor

👋 are you waiting on us for anything@higherorderfunctor?

Nope! I'll be picking this back up this week / weekend. Had a vacation and some deadlines the last two weeks.

JoshuaKGoldberg reacted with rocket emoji

@JoshuaKGoldberg
Copy link
Member

Great! Let's time box this to a week from now, then - we really want to release v8 as soon as possible.

@higherorderfunctor
Copy link
ContributorAuthor

CreatedgetParsedConfigFile.test.ts.

Will revert most of the changes tocreateProjectService.test.ts except for the use of the new function (mocked).
Decide if dropping or creating the complex tests as an integration (probably drop).
Merge in v8 again and resolve conflicts.

Plan to be done by tonight.

JoshuaKGoldberg reacted with rocket emoji

@higherorderfunctor
Copy link
ContributorAuthor

@JoshuaKGoldberg I will have some time tomorrow to check on the CI for any errors or for any final edits you would like to make. Thanks!

JoshuaKGoldberg reacted with thumbs up emojiJoshuaKGoldberg reacted with heart emoji

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.

🔥 this is great, thanks!

I only have one teeny nit. It's absolutely not blocking at all. Will leave this open for a bit in case anybody else from the team has time to review.

@JoshuaKGoldbergJoshuaKGoldberg added 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting responseIssues waiting for a reply from the OP or another party labelsJul 24, 2024
higherorderfunctorand others added2 commitsJuly 24, 2024 19:09
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@higherorderfunctor
Copy link
ContributorAuthor

All changes made. Let me know if anything else!

I think the running CI job is stuck,netlify shows complete.

Opened#9641 for the test clean up.

JoshuaKGoldberg reacted with thumbs up emoji

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.

🔥 thanks!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@bradzacherbradzacherbradzacher 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

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

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