typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

fix(eslint-plugin): [prefer-optional-chain] ignore`check` option for most RHS of a chain#11272

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

Jump to bottom

Merged

kirkwaiblinger merged 25 commits intotypescript-eslint:mainfromnayounsang:void-optional-chain

Aug 1, 2025

Merged

fix(eslint-plugin): [prefer-optional-chain] ignore`check` option for most RHS of a chain#11272

kirkwaiblinger merged 25 commits intotypescript-eslint:mainfromnayounsang:void-optional-chain

Aug 1, 2025

Conversation

Copy link

Contributor

nayounsang commentedJun 5, 2025•
edited
Loading

PR Checklist

Addresses an existing open issue:fixesBug: [prefer-optional-chain] should report case that can be converted to optional function call?.() #11270
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Apply check option to LHSs except most RHS to fix the bug where void functions were not reported

nayounsang added4 commits

June 5, 2025 16:11

feat: add check void option

aec9ef5

test: add test case for check void

e35a6f2

feat: check void type

eacc622

chore: add snapshot

88decc1

Copy link

Contributor

typescript-eslintbot commentedJun 5, 2025

Thanks for the PR,@nayounsang!

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

Copy link

netlifybot commentedJun 5, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`dee8b4d`
🔍 Latest deploy log	https://app.netlify.com/projects/typescript-eslint/deploys/688d42f4877c41000879fdf3
😎 Deploy Preview	https://deploy-preview-11272--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 87 (🔴 down 11 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (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 project configuration.

Copy link

nx-cloudbot commentedJun 5, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commitdee8b4d

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	5m 7s	View ↗
`nx run-many -t lint`	✅ Succeeded	3m 16s	View ↗
`nx run-many -t typecheck`	✅ Succeeded	2m 13s	View ↗
`nx test eslint-plugin-internal --coverage=false`	✅ Succeeded	3s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	1s	View ↗
`nx run integration-tests:test`	✅ Succeeded	3s	View ↗
`nx run types:build`	✅ Succeeded	5s	View ↗
`nx run generate-configs`	✅ Succeeded	6s	View ↗
`Additional runs (27)`	✅ Succeeded	...	View ↗

☁️Nx Cloud last updated this comment at2025-08-01 22:55:39 UTC

nayounsang changed the title~~fix(prefer-optional-chain): should report case that can be converted to optional function call ?.()~~fix(eslint-plugin): should report case that can be converted to optional function call ?.() when using perfer-optional-chain

Jun 5, 2025

nayounsang marked this pull request as draft

June 5, 2025 07:38

Copy link

codecovbot commentedJun 5, 2025•
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.86%. Comparing base (5b24864) to head (dee8b4d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@##             main   #11272   +/-   ##=======================================  Coverage   90.86%   90.86%           =======================================  Files         503      503             Lines       51046    51048    +2       Branches     8418     8416    -2     =======================================+ Hits        46384    46386    +2  Misses       4648     4648             Partials       14       14

Flag	Coverage Δ
unittest	`90.86% <100.00%> (+<0.01%)`	⬆️

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

Files with missing lines	Coverage Δ
...efer-optional-chain-utils/gatherLogicalOperands.ts	`100.00% <100.00%> (ø)`

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

docs: add checkvoid section

4171c5e

nayounsang changed the title~~fix(eslint-plugin): should report case that can be converted to optional function call ?.() when using perfer-optional-chain~~fix(eslint-plugin): should report case that can be converted to optional void function call ?.() when using perfer-optional-chain

Jun 5, 2025

chore: update snapshot

791461b

nayounsang marked this pull request as ready for review

June 5, 2025 08:11

kirkwaiblinger changed the title~~fix(eslint-plugin): should report case that can be converted to optional void function call ?.() when using perfer-optional-chain~~fix(eslint-plugin): [prefer-optional-chain] should report case that can be converted to optional void function call ?.()

Jun 6, 2025

kirkwaiblinger changed the title~~fix(eslint-plugin): [prefer-optional-chain] should report case that can be converted to optional void function call ?.()~~fix(eslint-plugin): [prefer-optional-chain] should report case that can be converted to optional void function call?.()

Jun 6, 2025

kirkwaiblinger reviewed

Jun 6, 2025

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

This generally looks great! Just one big question on the implementation approach we want to take. Looking forward to hearing your thoughts!

packages/eslint-plugin/src/rules/prefer-optional-chain-utils/gatherLogicalOperands.tsShow resolvedHide resolved

kirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party label

Jun 6, 2025

nayounsangand others added5 commits

June 7, 2025 17:22

refactor: inject option based on flagsToExcludeFromCheck

b3d2a4f

test: add test for checkVoid = false

656ec50

Merge branch 'main' into void-optional-chain

3ea00d0

Merge branch 'void-optional-chain' ofhttps://github.com/nayounsang/t…

5e7fec4

…ypescript-eslint into void-optional-chain

test: add tc for all checks are excluded but invaild

4b92ad4

Copy link

Member

kirkwaiblinger commentedJul 6, 2025

This PR has the effect that the following code is flagged (withcheckVoid enabled)

declareconstmaybeVoid:void|{x:()=>{some:'object'}};maybeVoid&&maybeVoid.x();

I'm not sure that that's what we want? I'd say instead the goal is that avoid-returning function call at the end of a chain whose return value isn't used should be reported regardless of thecheck* options.

In other words, as far as implementation - we're not looking to include possibly-void values in checked chainsexcept as the type of a function call at the end of the chain. It's not obvious to me that we need to create an option at all for this...

WDYT?

Copy link

Member

kirkwaiblinger commentedJul 7, 2025•
edited
Loading

I've realized — I think this is a much more general bug. Thecheck* options shouldnot apply to theright hand side member of a chain. Only theleft member. That is to say,

declareconstx:{a:string};// checkString: trueconsty=x&&x.a;// correctly reports// checkString: falseconsty=x&&x.a;// (BUG) doesn't report

So, if we generally fix the logic so that the type checking only applies to the LHS of a potential chain expressionx?.a, that should also automatically fix the reported bug regardingx?.a() as a special case.

Merge branch 'main' into void-optional-chain

01f6313

Copy link

ContributorAuthor

nayounsang commentedJul 12, 2025

Then, I guess we can check it except for right most. I will finish the work on void and reply.

nayounsang added3 commits

July 12, 2025 21:40

test: add test cases

b3bf6b7

fix: ignore isValidFalseBooleanCheckType if there is no more op

b0adaa8

feat: remove checkVoid option

2559e54

fix: reupdate snapshot

1ef4c51

kirkwaiblinger reviewed

Jul 14, 2025

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

I think this is moving in the right direction, but the current state of the PR seems to be mostly extraneous changes. Let's clean it up to be focused on the changes related to the bug. Thanks!

packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.tsShow resolvedHide resolved

packages/eslint-plugin/src/rules/prefer-optional-chain-utils/gatherLogicalOperands.tsShow resolvedHide resolved

kirkwaiblinger requested changes

Jul 14, 2025

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

(see review)

kirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party label

Jul 14, 2025

Copy link

ContributorAuthor

nayounsang commentedJul 16, 2025•
edited
Loading

Oh, sorry. I misunderstood your review. Now I know what to do
I'll revert all of changes because, I changed so much and produce unnecessary codes.
Then, applying the logic to check except most RHS, I will focus on whether the issue case passes.
And if there is any problem, I will ask you. Thank you.

nayounsangand others added5 commits

July 16, 2025 17:23

Merge branch 'main' into void-optional-chain

d0b2c6b

chore: revert gatherLogicalOperands.ts

87107cd

chore: revert perfer-optional-chain.test.ts

cba0f37

fix: ignore isValidFalseBooleanCheckType if there is no more op

433e6d4

test: add test case

baf82db

nayounsang changed the title~~fix(eslint-plugin): [prefer-optional-chain] should report case that can be converted to optional void function call?.()~~fix(eslint-plugin): [prefer-optional-chain] ignorecheck option for most RHS of a chain

Jul 16, 2025

Copy link

Member

kirkwaiblinger commentedJul 20, 2025

Hey@nayounsang — is this ready for another review pass? If so, weask that you request review in the GUI so we know to take another look. If not, take your time and don't mind me! Just looked like it might be ready for another pass. Thanks!

nayounsang requested a review fromkirkwaiblinger

July 20, 2025 10:40

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Jul 20, 2025

kirkwaiblinger reviewed

Jul 21, 2025

View reviewed changes

packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts OutdatedShow resolvedHide resolved

Update packages/eslint-plugin/tests/rules/prefer-optional-chain/prefe…

6095671

…r-optional-chain.test.ts

kirkwaiblinger previously approved these changes

Jul 21, 2025

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Thanks!

kirkwaiblinger added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Jul 21, 2025

Update prefer-optional-chain.test.ts

6fafc22

kirkwaiblinger dismissed theirstale review via6fafc22

July 21, 2025 07:51

kirkwaiblinger approved these changes

Jul 21, 2025

View reviewed changes

kirkwaiblingerenabled auto-merge (squash)

August 1, 2025 22:22

Merge branch 'main' into void-optional-chain

dee8b4d

kirkwaiblinger merged commit757f9ee intotypescript-eslint:main

Aug 1, 2025

61 checks passed

renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request

Aug 4, 2025