Uh oh!
There was an error while loading. Please reload this page.
lint: gate unused imports at error and clear the measured sites (#6467) - #7332
Draft
yinlianghui wants to merge 3 commits into
Draft
lint: gate unused imports at error and clear the measured sites (#6467)#7332yinlianghui wants to merge 3 commits into
yinlianghui wants to merge 3 commits into
Conversation
Promotes the unused-IMPORT subclass of `@typescript-eslint/no-unused-vars` to `error` via a new delegating local rule, and removes the 108 unused imports that promotion finds (82 files, removal-only). Every other subclass of the rule keeps the severity it had. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGMDbrVa8JjZcCQ7DWYH1b
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
The file existed in the working tree when `check-changeset-presence.mjs` was run locally, so the gate went green while it was still untracked; CI, which sees only committed content, went red on the same check. Committed now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGMDbrVa8JjZcCQ7DWYH1b
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6467
R2 of the unused-code program anchored on #4806. Promotes the unused-IMPORT
subclass of
@typescript-eslint/no-unused-varstoerrorand clears everysite the promotion finds, in one PR. No other subclass changes severity.
Before / after
Measured repo-wide with
pnpm exec eslint . --format jsonfrom the repo root(one run covers everything: there is exactly one flat config in the repo, and
the per-package
lintscripts are alleslint .resolving up to it).dd5b01b59eslint .exit code@typescript-eslint/no-unused-varsfindingsBoth deltas are 108, and they are the same 108: the warn half loses exactly
the import subclass and keeps all 102 of everything else at the severity it
already had.
The charter's figure was ~114, taken ten days earlier; re-measured here at
108 in 82 files. The population was derived twice, independently, and the two
instruments agree on the count, the per-area distribution and the per-site
list: (1) an AST classification of the repo-wide JSON report, matching each
finding's reported position against the local identifier of every import
specifier in that file; (2) a second, minimal flat config running only
no-unused-vars, classified the same way.The gate
New local rule
object-ui/no-unused-imports(eslint-rules/no-unused-imports.js),set to
errorineslint.config.jsdirectly below the rule it narrows. Itruns
@typescript-eslint/no-unused-varsunmodified and forwards only thereports whose node is an import binding. Nothing in it decides what "unused"
means, so type-only usage, re-exports, declaration merging, the JSX pragma and
the ignore patterns are all handled once, upstream.
The two lines are configured from an identical options object on purpose: they
are one analysis at two severities, and that is what stops them drifting into
two different opinions of the same word.
Alternatives considered, both recorded in the rule's header:
eslint-plugin-unused-imports(v4.4.1). It would have installed cleanly —peer ranges are
eslint ^10 || ^9 || ^8and@typescript-eslint/eslint-plugin ^8,both satisfied here. Rejected because its intended shape is to also replace
the base rule with its own vendored fork of
no-unused-varsin order to avoiddouble-reporting; that fork tracks typescript-eslint on its own schedule and is
a much larger change to this repo's lint semantics than this card charters.
no-unused-varsitself. Verified against the installed rule'sschema:
vars,varsIgnorePattern,args,caughtErrors,ignoreRestSiblings,destructuredArrayIgnorePattern,reportUsedIgnorePattern,ignoreClassWithStaticInitBlock,ignoreUsingDeclarations,enableAutofixRemoval. They narrow by NAME and bydeclaration KIND; not one of them can single out an import. There is no
imports-only severity to configure.
An unused import is now reported twice — a warning from the base rule and an
error from this one. That is inherent to the chartered shape rather than an
oversight: the base rule cannot be told to skip imports, so silencing the
warning half would mean replacing it.
.github/workflows/lint.ymlsets no--max-warnings, so the warning half is inert in CI and only the error can faila build. Written up beside both rules.
How the sites were removed
Removal-only, and not by hand: the sweep is the upstream rule's own fixer,
run through a temporary minimal config with
enableAutofixRemoval: { imports: true }and
--fix-type problem. That config is not part of this PR; the shipped gatedeclares no autofix option, so
eslint --fixdoes not delete imports.--fix-type problemis load-bearing. A first attempt without it touched 115files instead of 82: ESLint also removed 33 files' worth of
eslint-disabledirectives, which are unused under the minimal probe config and entirely live
under the real one. That run was reverted wholesale before the narrowed one.
The result was then checked against the measured list: 82 files changed, 82
files expected, zero unexpected files touched and zero expected files
missed. Every removed line is either an
importstatement or a specifiercontinuation line of a multi-line one; every added line is a rewritten import
statement. The only hand edit is whitespace — the fixer leaves
import { x }with a double space when it removes the first specifier, normalised in the four
lines where that happened.
23 of the 108 are the sole specifier of their declaration, so removal deletes
the whole
importstatement and could in principle drop a module imported forits side effects. All 23 were read individually; none is a side-effect import.
The two that looked like one are not:
packages/layout/src/index.tsstill loads./SidebarNav— the file hasexport * from './SidebarNav'four lines below the import.packages/app-shell/.../PageShell.tsxstill loads@object-ui/components—it imports
Badgefrom the same package on the line above.Deferred sites
None. The lane exclusions cost nothing, which was measured rather than
assumed. #7291 was already merged (it is this branch's base commit). #7304,
#7305, #7306 and #7309 change only
scripts/**,vitest.config.mts,vitest.setup.*ande2e/*.spec.ts; the #6580 exclusion coversapps/console/src/**/*.test.tsx,packages/app-shell/src/index.tsandapps/console/vite.config.ts. Not one of those files carries a site — thepopulation contains no
apps/consolefile at all — so no site had to be heldback and no file in the sweep is touched by an open PR on this lane.
Itemised site list — all 108 removals, 82 files
Format:
path:line localName [kind from module]. "sole" marks the 23 thatwere the only specifier of their declaration, so the whole
importstatement went.The
_-prefixed-but-dormant class: EXCLUDED, deliberatelyThe class #4806's R1 note flagged — names that carry the
_marker honestly butare dormant code rather than deliberate declarations, e.g.
let _projectRootinpackages/cli/src/commands/dev.ts— is excluded from this card, and it isexcluded by construction rather than by choice: it is not an import class at
all. Every one of its members is a local, so it sits in the 102 findings this PR
leaves at
warn, untouched. Measured on the population here: 0 of the 108unused imports carry a
_prefix, so the two sets do not even overlap.Deciding what to do with it needs a per-site judgement of what the dormant code
was for — the opposite of this card's removal-only, mechanical population — and
it applies to a subset of the 613 findings R1's ignore patterns closed, which is
a different measurement from the one this PR re-took. It stays on #4806 for R4
to size. Recorded here so it does not fall out silently.
Also not touched, per the charter: the
jsxPragmaquestion (the dead Reactimports visible only to tsc) is a separate review, and R3/R4 stay on #4806.
Verification
Every exit code below was captured by redirect before any pipe.
Non-vacuity — the gate really fails, on real sites. Three files this PR
swept were restored to their pre-sweep content from the base commit, linted, and
restored, all inside one shell invocation:
10 errors, naming exactly the 10 sites those three files carried (1 + 8 + 1).
The mutation was proven on disk before the lint (each reintroduced import text
grepped, count 1); the restore was proven after it by an empty
git diff HEADand by
git hash-objecton each path equalling that path's HEAD blob hash.Control — the warn half is unchanged. A scratch file with one unused import
and one unused local:
The import errors; the local still only warns.
eslint --print-configconfirmsthe pair resolves as
[2, {...}]and[1, {...}]with identical options.Gates, all on the merged head unless noted.
eslint .repo-wide (merged head)no-unused-vars102turbo run buildTasks: 44 successful, 44 totalturbo run type-checkTasks: 81 successful, 81 totalcheck-changeset-presence.mjscheck-changeset-no-major.mjsmajorbump."check-control-bytes.mjscheck-lint-coverage.mjscheck-entry-guard.mjstsc -p tsconfig.scripts.jsoncheck-phantom-dependencies.mjsThe gate list was derived from
.github/workflows/lint.ymland the touchedpaths, not from a hand-kept list.
scripts/__tests__/lint-workflow.test.tsisthe one that judges a new
error-levelobject-ui/*rule; it passes, and itneeded no edit — it asserts the config sets such rules generically and
deliberately keeps no enumeration to update.
Tests, all from the repo root,
Test Files N passed / Tests N passed,exit 0 in every batch:
no-unused-imports.test.js+ lint-workflow, turbo-lint-inputs, vitest-invocation-guard, check-vi-mock-inheritThe new rule's own suite is 21 cases. Its
validhalf is the load-bearing one:this rule throws most of the upstream rule's output away, so the way it breaks
is not "misses an import" but "stops throwing the rest away", which would
promote 102 locals, parameters and caught errors to errors in one commit. Every
non-import construct the base rule reports is pinned valid.
The seven
packages/*/vitest.config.tsfiles in the sweep lost onlyimport path from 'path'. They are standalone configs used solely bypackage-cwd runs, which
assertCanonicalVitestInvocationrefuses; the rootconfig reaches those packages' tests directly.
Declared narrowing.
packages/app-shell's full suite is the one batch notrun locally: it exceeds the container's ~10-minute foreground cap, twice, with
no failure printed. Its seven swept source files are covered on the compile side
by
turbo run type-check(81/81, and each package's task istsc --noEmit && tsc -p tsconfig.test.json, so its test files are type-checkedtoo), and its five swept test files ran green in the first batch. CI runs the
suite in full regardless. Recorded as narrowed rather than passed.
Scheduling
Per the #3535 semantic-collision discipline recorded on #4806,
origin/mainwasmerged into this branch as the last step —
dd5b01b59toc93b4d5f3, ninecommits, no conflicts,
pnpm-lock.yamlunchanged — and the gate was re-run onthe merged head, where it reports 0 errors and 0 unused-import findings. The
commits that landed meanwhile brought no new unused import.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BGMDbrVa8JjZcCQ7DWYH1b
Generated by Claude Code