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): pass jsDocParsingMode everywhere#7997

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

Merged
JoshuaKGoldberg merged 1 commit intomainfromts5dot3-set-jsdocparsingmode
Nov 27, 2023

Conversation

bradzacher
Copy link
Member

Overview

@fisker made me realise that we hadn't wired this new option up to every possible spot.
This PR ensures that all locations that we create a program or source file have the new option set to improve our perf.

fisker reacted with laugh emoji
@bradzacherbradzacher added the enhancementNew feature or request labelNov 27, 2023
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@bradzacher!

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 commentedNov 27, 2023
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitedb1b93
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/65649f710e4a280008401613
😎 Deploy Previewhttps://deploy-preview-7997--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 5 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.

@bradzacherbradzacher mentioned this pull requestNov 27, 2023
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.

@JoshuaKGoldbergJoshuaKGoldberg changed the titlefeat(typescript-estree): [ts5.3] pass jsDocParsingMode everywherefeat(typescript-estree): pass jsDocParsingMode everywhereNov 27, 2023
@JoshuaKGoldbergJoshuaKGoldberg merged commit3d2a344 intomainNov 27, 2023
@JoshuaKGoldbergJoshuaKGoldberg deleted the ts5dot3-set-jsdocparsingmode branchNovember 27, 2023 14:00
@jakebailey
Copy link
Collaborator

jakebailey commentedNov 27, 2023
edited
Loading

One day you may want to export this as some sort of setting; if lint rules want to do anything withProgram besides query types (or, they want to access the originalJSDoc nodes),ParseForTypeInfo /ParseNone won't provide the full picture anymore.

thorn0, JoshuaKGoldberg, and bradzacher reacted with thumbs up emoji

@bradzacher
Copy link
MemberAuthor

@jakebailey for sure - if we have some concrete usecases that would be an option!
The problem is that plugins run well after the parse+typecheck is done - which means that a plugin cannot control this setting. Instead a user would have to pass it via their eslint config as instructed by the plugin's docs.

they want to access the original JSDoc nodes

AFAIK the parsed JSDoc AST isn't consumed by anyone (at least nobody's ever mentioned a usecase for it -- people always avoid touching the TS AST if they can).

if lint rules want to do anything with Program besides query types ... ParseForTypeInfo / ParseNone won't provide the full picture anymore

What part of the picture is missing from the program? Isn't it just some scope analysis / usage information in terms of "this thing was used in JSDoc within a TS file"?

@jakebailey
Copy link
Collaborator

a user would have to pass it via their eslint config as instructed by the plugin's docs.

Yep, this is precisely what I meant.

AFAIK the parsed JSDoc AST isn't consumed by anyone (at least nobody's ever mentioned a usecase for it -- people always avoid touching the TS AST if they can).

I'm still holding out hope to reuse info from TS's parser to make eslint-plugin-jsdoc faster; TS parses the same info way faster and it would have already been doing it unconditionally until the new option.

What part of the picture is missing from the program? Isn't it just some scope analysis / usage information in terms of "this thing was used in JSDoc within a TS file"?

So concretely, you won't get any info about something being deprecated, because that is part of what's skipped (depending on the mode). That and a rule that could use any other kind of tag.

@bradzacher
Copy link
MemberAuthor

We actually currently rely on@deprecated viaeslint-plugin-deprecation
https://github.com/gund/eslint-plugin-deprecation/blob/ae8ca953b1b4cc2021525089f56ede97613e9311/src/rules/deprecation.ts#L237

However I just tested and re-tested this and it seems to be working fine even with this PR.

I.e. if the rule was unable to get the JSDoc tag from the TS ast this file would have lint errors due to unused disable comments:

// eslint-disable-next-line deprecation/deprecation -- this is safe as it's guarded
if(includeIllegalModifiers||ts.canHaveModifiers(node)){
// eslint-disable-next-line deprecation/deprecation -- this is safe as it's guarded
constmodifiers=ts.getModifiers(nodeasts.HasModifiers);

@jakebailey
Copy link
Collaborator

Right; Josh had told me that you used something that didn't use the TS info directly before I proposed/implemented the new option.

Mainly that was just an easy example.

The other case is see/link, which aren't parsed in the modes used after this PR, which would be needed to create an edge-case free version of no-unused-var, my main hope for the future.

@bradzacher
Copy link
MemberAuthor

Okay now I see it. I had to push through all of the nx caching to see it properly.
This change actually did breakeslint-plugin-deprecation.

@bradzacher
Copy link
MemberAuthor

I'll fix this and we'll do an out-of-band release to get this out soon.

@jakebailey
Copy link
Collaborator

jakebailey commentedNov 27, 2023
edited
Loading

Ugh, yeah, they are pulling it out of jsdoc nodes viagetJsDocTags:

https://github.com/gund/eslint-plugin-deprecation/blob/ae8ca953b1b4cc2021525089f56ede97613e9311/src/rules/deprecation.ts#L311-L318

I guess I really thought that this wasn't the case, but I never verified myself...

On the TS side, there's no jsdoc parsing mode besidesParseAll that would give this info; potentially this indicates that there should be more, or thatdeprecated should be included in others, but it sorta messes with the neat buckets I gave that relate to being able to produce accurate diagnostics or accurate type info (as deprecation is "its own thing" which realistically was about editor feedback).

@bradzacher
Copy link
MemberAuthor

bradzacher commentedNov 28, 2023
edited
Loading

Yeah there was a user in discord that also mentioned that they were using a custom JSDoc tag to power a custom lint rule and it OFC broke with this change.

So we can fit a lot of users in a neat box - but we also need to allow people to get out of the box.

Worth noting that a lot of those use cases could probably be sorted with a substring match egnode.getJsdocCommentText().includes('@deprecated') would work as an API and would likely cover most cases and avoid the need to parse JSDoc at all

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsDec 6, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

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.

3 participants
@bradzacher@jakebailey@JoshuaKGoldberg

[8]ページ先頭

©2009-2025 Movatter.jp