Skip to content

chore: adopt noImplicitOverride and noFallthroughCasesInSwitch repo-wide - #691

Open
lmorchard wants to merge 1 commit into
mainfrom
fix/688-adopt-two-strict-flags
Open

lmorchard wants to merge 1 commit into
mainfrom
fix/688-adopt-two-strict-flags

Conversation

@lmorchard

Copy link
Copy Markdown
Collaborator

Takes two of the three strict flags WXT 0.21 introduced, so that #688 becomes a single question about noUncheckedIndexedAccess instead of three tangled ones. Refs #688 (does not close it).

Why these two and not the third

I measured each flag separately, deduplicated across packages (cli/server/extension all typecheck core via paths, so raw per-package counts double-count):

Flag Unique sites
noFallthroughCasesInSwitch 0
noImplicitOverride 5 — 3 files, all core/src
noUncheckedIndexedAccess 55 as CI is configured today, +145 more in core/test

This PR is the two free ones. The noUncheckedIndexedAccess breakdown is 21 extension/test, 14 core/src, 10 cli/test, 9 extension/src, 1 extension/e2e — 31 of 55 in tests, where the flag buys close to nothing.

The five override annotations are real

Not appeasement — each is a genuine override of a base-class member:

  • core/src/events.ts:547emit over EventEmitter.emit. This one already had an "Override emit to also trigger wildcard listeners" doc comment while lacking the keyword.
  • core/src/loggers/filter.tsinitialize, dispose over LoggerWrapper
  • core/src/loggers/metricsCollector.tsinitialize, dispose over LoggerWrapper

Verification

I checked the flags actually bite rather than sitting inert in config:

# with one 'override' keyword removed:
src/loggers/filter.ts(39,3): error TS4114: This member must have an 'override' modifier...
# with a deliberate switch fallthrough added:
src/m688probe.ts(3,5): error TS7029: Fallthrough case in switch.

Both probes reverted; tree clean. Also green: full pnpm run check (typecheck across all packages, schema drift gate, 1600 tests) and pnpm run build + CLI startup on node 24.

Note on placement

Enabled in each package's tsconfig because there's no shared base config — core/cli/server are standalone and the extension extends WXT's generated one. Introducing a shared base is a bigger refactor than this warrants, but it does mean four files to touch for the next flag.

What's left on #688

Only noUncheckedIndexedAccess. My read, from having now read all 24 production sites across two audit passes: zero are real bugs, so the case for it is forward-looking (catching future unguarded indexing) rather than remedial. Worth deciding on those terms. Also surfaced while measuring: core/tsconfig.json excludes **/*.test.ts, so core's 993 tests are never typechecked by anything — clean at baseline today, but that's where 145 of the 200 sites live.

Two of the three strict flags WXT 0.21's generated tsconfig introduced. Both
are cheap enough to take now, which leaves #688 as a single question about
noUncheckedIndexedAccess rather than three tangled ones.

Measured cost, deduplicated across packages (cli/server/extension all
typecheck core via `paths`, so raw per-package counts double-count):

  noFallthroughCasesInSwitch     0 sites
  noImplicitOverride             5 sites, 3 files, all core/src
  noUncheckedIndexedAccess      55 sites (+145 more in core/test)

So this change is the two free ones. The five `override` annotations are
genuinely correct documentation, not appeasement — every one is a real
override of a base-class member, and events.ts even carried an "Override
emit to also trigger wildcard listeners" comment saying so while lacking
the keyword.

Enabled per package because there is no shared base tsconfig; introducing
one is a larger refactor than this warrants. The extension keeps only its
noUncheckedIndexedAccess opt-out.

Verified both flags actually bite rather than being inert config: removing
one `override` keyword produces TS4114, and a deliberate switch fallthrough
produces TS7029. Full `pnpm run check` green (typecheck, schema drift, 1600
tests) plus the node 24 release build. Refs #688.
@lmorchard
lmorchard requested review from a team and a lite review from Copilot September 15, 2026 23:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved blocking issues were identified.

Pull request overview

Enables noImplicitOverride and noFallthroughCasesInSwitch across the repository.

Changes:

  • Updates TypeScript configurations across packages.
  • Removes obsolete extension opt-outs.
  • Adds five required override annotations in core.
File summaries
File Summary
packages/server/tsconfig.json Enables strict compiler flags.
packages/extension/tsconfig.json Aligns extension configuration with the adopted flags.
packages/core/tsconfig.json Enables strict compiler flags.
packages/core/src/loggers/metricsCollector.ts Marks logger lifecycle methods as overrides.
packages/core/src/loggers/filter.ts Marks logger lifecycle methods as overrides.
packages/core/src/events.ts Marks emit as an override.
packages/cli/tsconfig.json Enables strict compiler flags.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants