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 allowDefaultProjectForFiles project service allowlist option#7752

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 commentedOct 15, 2023•
edited
Loading

PR Checklist

Overview

Removes the check thatprojectService.openClientFile should return an opened file. In my testing, it looks like we can still generate type info for the file just with the default program for it?

Standalone repro here:https://github.com/JoshuaKGoldberg/repros/tree/project-service-without-opened-filehttps://github.com/JoshuaKGoldberg/repros/tree/allow-default-project-for-files

Asked for confirmation about this approach on the TypeScript Discord:https://discord.com/channels/508357248330760243/640177429775777792/1163133940115197952. Edit: confirmed, and filed#7761 as a followup ✅

so1ve reacted with thumbs up 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 commentedOct 15, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit2ce23c0
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6587e15e2cd6e300080fb3e7
😎 Deploy Previewhttps://deploy-preview-7752--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: 98 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (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-cloud
Copy link

nx-cloudbot commentedOct 15, 2023•
edited
Loading

☁️ Nx Cloud Report

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

🟥 Failed Commands
nx run integration-tests:test
nx run-many --target=typecheck --parallel
✅ Successfully ran 17 targets

Sent with 💌 fromNxCloud.

@mrazauskas
Copy link
Contributor

Thanks for working on this.

I wonder, should it perhaps throw an error or at least log a warning is the default TSConfig is used? For instance, in my case there was no intention to use the default config. Seeing a warning or error would make me either to include the file into the program or to exclude it from lint.

Just thinking out loud. Can be I misunderstood the fix.

@JoshuaKGoldberg
Copy link
MemberAuthor

JoshuaKGoldberg commentedOct 15, 2023•
edited
Loading

Hmm yeah it's an interesting idea. I don't think we'd want to warn by default, because one of the primary motivations for the option is to allow unlisted files to be linted (e.g..eslintrc.cjs). But I can definitely see why you'd want that. I'll wait on getting explicit feedback around the approach from the TypeScript team (asked in the TS Discord & linked in the OP) before maybe filing an issue or discussion.

Edit:#7761 👍

Copy link
Collaborator

@jakebaileyjakebailey left a comment

Choose a reason for hiding this comment

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

Theoretically looks correct to me; the only actual change is not returning early.

JoshuaKGoldberg reacted with thumbs up emoji
@JoshuaKGoldberg
Copy link
MemberAuthor

JoshuaKGoldberg commentedOct 19, 2023•
edited
Loading

Excellent, thanks.I'll hold off on considering this mergeable until the conversation in#7761 is resolved. Just in case we end up radically overhauling everything. ✅ I think we're good to go for this specific issue.

@JoshuaKGoldbergJoshuaKGoldberg added the blocked by another issueIssues which are not ready because another issue needs to be resolved first labelOct 19, 2023
@JoshuaKGoldbergJoshuaKGoldberg changed the titlefix(typescript-estree): allow project service for unknown client filefeat(typescript-estree): allow project service for unknown client fileNov 2, 2023
@JoshuaKGoldberg
Copy link
MemberAuthor

Re-targeting this to specifically focus on#7871.

@JoshuaKGoldbergJoshuaKGoldberg changed the titlefeat(typescript-estree): allow project service for unknown client filefeat(typescript-estree): add allowDefaultProjectForFiles project service allowlist optionNov 7, 2023
@JoshuaKGoldbergJoshuaKGoldberg removed the blocked by another issueIssues which are not ready because another issue needs to be resolved first labelNov 7, 2023
@bradzacherbradzacher added the enhancementNew feature or request labelNov 10, 2023
@@ -9,7 +13,15 @@ const createStubFileWatcher = (): ts.FileWatcher => ({

export type TypeScriptProjectService = ts.server.ProjectService;

export function createProjectService(): TypeScriptProjectService {
export interface ProjectServiceSettings {
allowDefaultProjectForFiles: ReadonlySet<string>;
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This name is not particularly good but it's under an experimental option so I'd expect we'll rethink all names for v7.

bradzacher
bradzacher previously approved these changesDec 23, 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.

approving - pending that last convo being resolved

@JoshuaKGoldbergJoshuaKGoldberg merged commit7ddadda intotypescript-eslint:mainDec 24, 2023
@JoshuaKGoldbergJoshuaKGoldberg deleted the project-service-allow-no-client-file branchDecember 24, 2023 22:40
auvred pushed a commit to auvred/typescript-eslint that referenced this pull requestDec 29, 2023
…ice allowlist option (typescript-eslint#7752)* fix(typescript-estree): allow project service for unknown client file* Fixed up project throwing tests* Added allowDefaultProjectForFiles as nested option* Added a bit more testing* fix: only throw on missing file path if hasFullTypeInformation* Fix test snapshot* Remove unnecessary assertion* TypeScript_ESTree.mdx docs* Absolute and canonical file paths* Use minimatch instead of fs globbing* lint: import sorting, missing return type* Ignore some tests
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJan 1, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jakebaileyjakebaileyjakebailey approved these changes

@bradzacherbradzacherbradzacher left review comments

Assignees
No one assigned
Labels
enhancementNew feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Allow project service to provide type information for out-of-project files
4 participants
@JoshuaKGoldberg@mrazauskas@jakebailey@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp