feat(eslint-plugin): [no-unnecessary-condition] make `allowConstantLoopConditions` more granular #10639

ronami · 2025-01-11T20:18:45Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [no-unnecessary-condition] Make allowConstantLoopConditions more granular #7047
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR attempts to tackle #7047 and changes allowConstantLoopConditions from a boolean to the following enum:

'never': same as the current false
'neverExceptWhileTrue': disallows constant conditions in all loops except while loops with the literal true.
'always': same as the current true

This is a non-breaking change as the existing options map to the new option (minus neverExceptWhileTrue).

Some thoughts:

I've used a similar option name for 'neverExceptWhileTrue' as no-constant-condition's checkLoops. The option name is somewhat limiting because it's so specific, it may be better to have an option name that's a bit less specific to allow future changes(?).

The original issue suggests that the default may be changed on a future major. However, since 'neverExceptWhileTrue' is similar to the current default ('never') but allows for an additional use-case, I think it can be considered to become the new default, especially since the pattern it allows is relatively common (or so I think).

typescript-eslint · 2025-01-11T20:18:49Z

Thanks for the PR, @ronami!

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.

netlify · 2025-01-11T20:19:03Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`10c8e97`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67a28d0934c3a200083621b7
😎 Deploy Preview	https://deploy-preview-10639--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🔴 down 1 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 your Netlify site configuration.

nx-cloud · 2025-01-11T20:19:37Z

View your CI Pipeline Execution ↗ for commit 10c8e97.

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 24s	View ↗
`nx test eslint-plugin`	✅ Succeeded	5m 58s	View ↗
`nx test utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test typescript-eslint --coverage=false`	✅ Succeeded	8s	View ↗
`nx test visitor-keys --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test type-utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx run types:build`	✅ Succeeded	<1s	View ↗
`nx test visitor-keys`	✅ Succeeded	<1s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-02-04 22:11:19 UTC

codecov · 2025-01-11T20:58:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.28%. Comparing base (81dc579) to head (10c8e97).
Report is 78 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10639      +/-   ##
==========================================
+ Coverage   86.94%   87.28%   +0.34%     
==========================================
  Files         446      450       +4     
  Lines       15508    15750     +242     
  Branches     4518     4613      +95     
==========================================
+ Hits        13484    13748     +264     
+ Misses       1669     1645      -24     
- Partials      355      357       +2

Flag	Coverage Δ
unittest	`87.28% <100.00%> (+0.34%)`	⬆️

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

Files with missing lines	Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts	`99.20% <100.00%> (-0.04%)`	⬇️

... and 115 files with indirect coverage changes

JoshuaKGoldberg

🚀 Whoohoo! Thanks for taking on this old issue, it's been bugging me for ages!

I agree with the implementation and direction. Keeping the legacy options around to make this non-breaking seems reasonable + necessary to me.

Just requesting changes on the name, since onlyTrue is a lot cleaner IMO.

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx

kirkwaiblinger · 2025-02-02T19:29:38Z

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx

@@ -84,12 +84,70 @@ function bar(arg?: string | null) {

 {/* insert option description */}

-Example of correct code for `{ allowConstantLoopConditions: true }`:
+#### `'never'` or `false`


I'm wondering if we can de-emphasize the true/false a bit to nudge users towards the string options now... As a random example from the top of my head, the vscode docs about code actions on save just give the string name in the header and mention the legacy boolean alias as an afterthought. Probably there are many more examples to draw from in the ecosystem.

Interesting! I like this, especially since this will probably (I assume) be removed or deprecated in the next major.

The closet I could find one typescript-eslint's repo is possibly TypeOrValueSpecifier's universal string specifiers (which isn't too similar). If these options would be removed or deprecated, should this have a note on the docs to warn about this potentially?

I guess that another option is to stick with true, false, and only-allowed-literals (I've seen this done here and also here for example).

I updated the PR to remove this from the header and down to the description (similar to the linked vscode example).

Another option I'm thinking of is the order of these three options: currently 'never' shows up first, followed by 'always', with 'only-allowed-literals' being last (even though it's probably [I'm assuming] the recommended way to go).

Should it potentially be the first option to promote users using it, at least until it's changed to the default?

Interesting! I like this, especially since this will probably (I assume) be removed or deprecated in the next major.

The closet I could find one typescript-eslint's repo is possibly TypeOrValueSpecifier's universal string specifiers (which isn't too similar). If these options would be removed or deprecated, should this have a note on the docs to warn about this potentially?

Eh, I don't think there's any value in adding a pre-deprecated state 🙃 I personally would be happy for the true/false option types to be deprecated in this PR but maybe others will have opinions.

I guess that another option is to stick with true, false, and only-allowed-literals (I've seen this done here and also here for example).

My reaction to this is 🤮, haha

I updated the PR to remove this from the header and down to the description (similar to the linked vscode example).

Another option I'm thinking of is the order of these three options: currently 'never' shows up first, followed by 'always', with 'only-allowed-literals' being last (even though it's probably [I'm assuming] the recommended way to go).

Should it potentially be the first option to promote users using it, at least until it's changed to the default?

Hmm. I don't have a strong opinion. Probably the default first is best and we can change the order if we change the default in the future? Whatever you think is best!

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx

kirkwaiblinger

JoshuaKGoldberg

🚀 Very nice how this ended up!

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.23.0 | 8.24.0 | | npm | @typescript-eslint/parser | 8.23.0 | 8.24.0 | ## [v8.24.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8240-2025-02-10) ##### 🚀 Features - **eslint-plugin:** \[no-unnecessary-condition] make `allowConstantLoopConditions` more granular ([#10639](typescript-eslint/typescript-eslint#10639)) ##### 🩹 Fixes - **eslint-plugin:** \[no-misused-spread] correct and elaborate string spread report message ([#10751](typescript-eslint/typescript-eslint#10751)) - **eslint-plugin:** \[restrict-plus-operands] report adding bigints to strings when `allowNumberAndString` is `false` ([#10737](typescript-eslint/typescript-eslint#10737)) ##### ❤️ Thank You - Josh Goldberg ✨ - noah - Ronen Amiel You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

ronami added 3 commits January 11, 2025 18:51

initial implementation

5541aa6

docs

82a5fa9

add test

47f7226

ronami added 2 commits January 11, 2025 22:35

update snapshot

010c4b7

add a literal false test

2760709

ronami marked this pull request as ready for review January 11, 2025 20:59

JoshuaKGoldberg requested changes Jan 19, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx Outdated Show resolved Hide resolved

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 19, 2025

neverExceptWhileTrue => only-true

b557fce

ronami requested a review from JoshuaKGoldberg January 21, 2025 19:54

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 21, 2025

change option name and include typescript's allowed literals

b30ccd2

kirkwaiblinger reviewed Jan 24, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx Outdated Show resolved Hide resolved

kirkwaiblinger reviewed Jan 24, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx Outdated Show resolved Hide resolved

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Jan 24, 2025

change docs

b48f6d7

ronami requested a review from kirkwaiblinger January 27, 2025 18:18

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 27, 2025

kirkwaiblinger reviewed Feb 2, 2025

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Feb 2, 2025

ronami added 2 commits February 4, 2025 18:55

adjust examples

54e54b0

update docs

cd8c3a7

ronami requested a review from kirkwaiblinger February 4, 2025 17:26

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 4, 2025

update snapshots

10c8e97

kirkwaiblinger approved these changes Feb 10, 2025

View reviewed changes

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 Feb 10, 2025

JoshuaKGoldberg approved these changes Feb 10, 2025

View reviewed changes

JoshuaKGoldberg merged commit ac8dde0 into typescript-eslint:main Feb 10, 2025
62 of 63 checks passed

ronami deleted the no-unnecessary-condition-allowConstantLoopConditions branch February 10, 2025 16:51

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(eslint-plugin): [no-unnecessary-condition] make `allowConstantLoopConditions` more granular #10639

feat(eslint-plugin): [no-unnecessary-condition] make `allowConstantLoopConditions` more granular #10639

Uh oh!

ronami commented Jan 11, 2025 •

edited

Loading

Uh oh!

typescript-eslint bot commented Jan 11, 2025

Uh oh!

netlify bot commented Jan 11, 2025 •

edited

Loading

Uh oh!

nx-cloud bot commented Jan 11, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 11, 2025 •

edited

Loading

Uh oh!

JoshuaKGoldberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kirkwaiblinger Feb 2, 2025

Uh oh!

ronami Feb 4, 2025 •

edited

Loading

Uh oh!

kirkwaiblinger Feb 10, 2025

Uh oh!

Uh oh!

kirkwaiblinger left a comment

Uh oh!

JoshuaKGoldberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(eslint-plugin): [no-unnecessary-condition] make allowConstantLoopConditions more granular #10639

feat(eslint-plugin): [no-unnecessary-condition] make allowConstantLoopConditions more granular #10639

Uh oh!

Conversation

ronami commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Jan 11, 2025

Uh oh!

netlify bot commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud bot commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kirkwaiblinger Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

ronami Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feat(eslint-plugin): [no-unnecessary-condition] make `allowConstantLoopConditions` more granular #10639

feat(eslint-plugin): [no-unnecessary-condition] make `allowConstantLoopConditions` more granular #10639

ronami commented Jan 11, 2025 •

edited

Loading

netlify bot commented Jan 11, 2025 •

edited

Loading

nx-cloud bot commented Jan 11, 2025 •

edited

Loading

codecov bot commented Jan 11, 2025 •

edited

Loading

ronami Feb 4, 2025 •

edited

Loading