[joehan]

Description

Resolves Buganizer b/576834850

Promotes ESLint rules no-fallthrough and no-constant-condition by removing their temporary warn overrides from .eslintrc.js:

• no-fallthrough: Deleted the *.ts override "no-fallthrough": "warn", allowing the recommended preset default ("error") to take effect. Moved the comment between empty case labels in src/init/features/hosting/index.ts inline onto the case "svelekit": line, resolving the single false-positive violation.
• no-constant-condition: Deleted the *.ts override "no-constant-condition": "warn", and added "no-constant-condition": ["error", { checkLoops: false }] to the top-level rules block in its alphabetical slot above "no-restricted-globals". This permits intentional loops (while (true) / do ... while (true) polling, retry, and REPL loops) while erroring on constant condition bugs in if and ternary statements across all files, and avoids merge conflicts with sibling PRs.
• Cleaned up two now-dead // eslint-disable-next-line no-constant-condition comments in src/deploy/functions/containerCleaner.ts and src/deploy/functions/runtimes/node/versioning.ts that previously suppressed intentional loops.

Scenarios Tested

• node_modules/.bin/prettier --check .eslintrc.js src/deploy/functions/containerCleaner.ts src/deploy/functions/runtimes/node/versioning.ts src/init/features/hosting/index.ts: Passed cleanly.
• npx eslint --report-unused-disable-directives: Verified 0 unused disable directives.
• npm run build: Passed cleanly.
• npm run test:compile: Passed cleanly without type errors.
• npm run lint:quiet: Passed cleanly across the entire repository.
• Full-repo ESLint scan verified 0 no-fallthrough and 0 no-constant-condition violations.
• Targeted mocha tests: npx mocha src/init/features/hosting/index.spec.ts passed (26 passing).

Sample Commands

N/A

[gemini-code-assist]

Code Review

This pull request updates the ESLint configuration to enforce the no-constant-condition rule as an error while allowing intentional while (true) loops, and removes deprecated overrides for no-constant-condition and no-fallthrough. Additionally, in src/init/features/hosting/index.ts, a comment regarding the svelekit case is refactored into an inline comment. There are no review comments, and I have no feedback to provide.

[joehan]

🤖 Adversarial AI review (review skill)

Requested by joehan. This PR's worker agent should address every comment in this review: fix it, or reply on the thread with the reason it doesn't apply.
Decision: approved_with_suggestions · 0 blocking · 2 suggestions · 2 inline comments

No blocking issues found; the config does what the PR claims. Effective config from eslint --print-config, base vs head: for .ts/.spec.ts, no-fallthrough goes from warn to error, and no-constant-condition goes from warn to ["error", {checkLoops: false}]. For .js, no-fallthrough stays at error, and no-constant-condition goes from error with the default checkLoops: true to checkLoops: false. That .js loosening is deliberate per b/576834850; the 21 linted .js files have no constant loops. No preset or remaining overrides block re-sets either rule. firebase-vscode has root: true and is unaffected. checkLoops: false is valid on the installed ESLint 8.57.0, whose schema is boolean-only. "allExceptWhileTrue" is rejected there ("should be boolean"). It only exists in ESLint >= 9.1, and even there it wouldn't cover the 3 do...while (true) loops. The option is broader than while (true): it also stops checking for (; true; ), while (1), while (false), and constant-RHS assignments like while (x = 5), and no-cond-assign is off via the google preset. for (;;) is never reported either way, because it has no test. Anything narrower means editing the loops, which the task forbids. So the PR's end state is the cleanest one available: no overrides, one top-level line, and no-fallthrough inherited from eslint:recommended. That end state is consistent with the learnings skill, since the new line carries a non-default option rather than being a redundant "error". The comment move is correct. no-fallthrough flags an empty case only when the next case isn't on the adjacent line, and the own-line comment broke that adjacency (base hit at index.ts:44:7). The trailing comment doesn't match /falls?\s?through/iu, so it can't mask a real fallthrough (checked with probes), and it still annotates the real svelekit framework key. Verification: full npm run lint:quiet (cold cache), tsc --noEmit -p tsconfig.json, and npm run test:compile all pass, and mocha src/init/features/hosting/index.spec.ts gives 26 passing. A type-free scan of all 1411 linted files finds 0 hits of either rule at head. With the default checkLoops, it finds exactly the bug's 18 (15 while, 3 do...while). origin/main equals GitHub main, which equals the merge-base 4a32371151, so merging main is a no-op. CI is green. Two optional suggestions on .eslintrc.js:33: a guaranteed merge conflict with sibling PR #11344, and two now-dead eslint-disable-next-line no-constant-condition comments. One PR-body nit: the cited targeted test is src/init/features/apphosting.spec.ts, not the touched file's src/init/features/hosting/index.spec.ts.

[joehan]

SUGGESTION (readability)

Optional: checkLoops: false turns two existing inline suppressions of this rule into dead comments. Both are // eslint-disable-next-line no-constant-condition, at src/deploy/functions/containerCleaner.ts:23 and src/deploy/functions/runtimes/node/versioning.ts:41, and both sit above while (true) loops.

At base they suppressed real reports: with --no-inline-config, the base config reports no-constant-condition at containerCleaner.ts:24:10 and versioning.ts:42:10. At head, npx eslint --report-unused-disable-directives on those two files reports Unused eslint-disable directive (no problems were reported from 'no-constant-condition') for both.

Lint stays green today, because the repo doesn't enable unused-directive reporting. But these are leftover cruft for the rule this PR settles, and ESLint 9 warns on unused disable directives by default. These two loops are also missing from the bug's list of 18, so the repo actually has 20 intentional constant loops, which further supports checkLoops: false.

Either delete the two comment lines in this PR or leave them for a follow-up. Deleting them goes slightly past the bug's "only these two files" scope boundary, so it's the maintainer's call.

Rule: general/dead-suppressions

[joehan]

Removed both dead // eslint-disable-next-line no-constant-condition comments from src/deploy/functions/containerCleaner.ts and src/deploy/functions/runtimes/node/versioning.ts in commit e1e8c2c. Verified with npx eslint --report-unused-disable-directives and full npm run lint:quiet.

[joehan]

SUGGESTION (other)

Optional: this entry is inserted directly above "no-useless-escape", and sibling PR #11344 deletes that line. The two PRs therefore conflict in .eslintrc.js, whichever one merges second. Verified with git merge-tree --write-tree 9aebf15c58 33cf4bba10, which reports a conflict in .eslintrc.js. The other Wave 2 siblings (#11343, #11346, #11347) merge cleanly with this PR.

The fix is to move the entry to its alphabetical slot just above "no-restricted-globals" (currently line 17; the first eight top-level rules are alphabetized). With that placement:

• git merge-file against the shared merge-base 4a32371151 merges cleanly with all four siblings.
• prettier --check and ESLint still pass.
• --print-config output is byte-identical for .ts, .js, and .spec.ts files.

While moving it, consider mentioning do ... while (true) in the comment. 3 of the 18 exempted loops are do-while: scripts/storage-emulator-integration/conformance/firebase-js-sdk.test.ts:447, src/apphosting/secrets/dialogs.ts:238, and src/apphosting/secrets/index.ts:178. ESLint >= 9.1 defaults to checkLoops: "allExceptWhileTrue", which exempts only while (true). With the current wording, this line will look redundant during an ESLint 9 migration, and deleting it would break lint. This is a move outside the diff hunk, so it can't be a one-click GitHub suggestion.

Suggested change:

-    "no-restricted-globals": ["error", "name", "length"],
+    "no-constant-condition": ["error", { checkLoops: false }], // Allow intentional `while (true)` and `do ... while (true)` loops.
+    "no-restricted-globals": ["error", "name", "length"],
-    "no-constant-condition": ["error", { checkLoops: false }], // Allow intentional `while (true)` polling/retry loops.

Rule: general/merge-hygiene

[joehan]

Moved "no-constant-condition" to its alphabetical slot above "no-restricted-globals" and updated the comment to mention do ... while (true) loops in commit e1e8c2c. This cleanly resolves the merge conflict with sibling PR #11344. Verified with prettier --check, npm run lint:quiet, and npm run test:compile.

[joehan]

Addressed review feedback in commit e1e8c2c:

1. Merge hygiene: Moved "no-constant-condition" to its alphabetical slot above "no-restricted-globals", and updated the comment to mention do ... while (true) loops. This cleanly resolves the merge conflict with sibling PR [AI Improvement] [Task] Remove useless escapes (no-useless-escape), fix three prompt/widget typos, and remove its warn override #11344.
2. Dead suppressions: Removed dead // eslint-disable-next-line no-constant-condition comments from src/deploy/functions/containerCleaner.ts:23 and src/deploy/functions/runtimes/node/versioning.ts:41.
3. Verification: Full npm run lint:quiet, npm run build, npm run test:compile, and npx mocha src/init/features/hosting/index.spec.ts (26 passing) all pass cleanly.