From 8eb0970b3f2defb8290a4f7d6c2339aafc1c88c2 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 11:00:06 -0400 Subject: [PATCH 01/20] Add `typescript-typing` skill for `any`-handling and type derivation --- CHANGELOG.md | 4 ++ .../coding/skills/typescript-typing/skill.md | 58 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 domains/coding/skills/typescript-typing/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8557ff8f..3fe0994a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `typescript-typing` skill (coding domain): the reasoning layer beneath `coding-guidelines`' TypeScript section — `any` as a type-checking-off directive with substitute-by-position guidance (assignee → `unknown`, assigned → `never`) and the generic-constraint exception, plus derive-types-from-authoritative-sources over ad-hoc declarations. Experimental. + ## [0.2.0] ### Added diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md new file mode 100644 index 00000000..b3e9cc64 --- /dev/null +++ b/domains/coding/skills/typescript-typing/skill.md @@ -0,0 +1,58 @@ +--- +name: typescript-typing +description: TypeScript typing discipline — treat `any` as a directive that disables type checking (substitute by position: assignee → `unknown`, assigned → `never`), and derive types from authoritative sources rather than hand-writing ad-hoc ones that duplicate and drift. +maturity: experimental +--- + +# TypeScript Typing Discipline + +Deepens the TypeScript section of `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This skill is the reasoning layer underneath those bullets: why `any` is dangerous and how to replace it by position, and how to avoid hand-writing a type that an authoritative source already defines. Grounded in `docs/typescript.md` (§ Avoid `any`, § Prefer type inference). + +## `any` is not a type — it is a directive that disables type checking + +The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: + +- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. +- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. +- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. +- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. + +## Substitute `any` by position — assignee vs assigned + +Identify which side of an assignment the `any` sits on: + +- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. + - 🚫 `type Fn = () => any; const xs: any[]` + - ✅ `type Fn = () => unknown; const xs: unknown[]` +- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. + +## The one acceptable exception — generic *constraints* + +`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. + +```typescript +class BaseController< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Messenger extends RestrictedMessenger, +> // ... +``` + +Three conditions attach: + +- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. +- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. +- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. + +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." + +## Derive types from authoritative sources — don't hand-write ad-hoc ones + +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. This is the structural form of the contributor-docs rule *Prefer type inference over annotations and assertions*: inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: + +- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. +- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. +- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. + +**Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From 9e20763e8775838730e59883de4f96be684317e7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 12:52:34 -0400 Subject: [PATCH 02/20] Ground the derive rule in a real `metamask-extension` #42583 counterexample --- .../coding/skills/typescript-typing/skill.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md index b3e9cc64..1b4d23d9 100644 --- a/domains/coding/skills/typescript-typing/skill.md +++ b/domains/coding/skills/typescript-typing/skill.md @@ -55,4 +55,30 @@ An ad-hoc type — one hand-defined to describe a value an authoritative type al - **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. - **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. +**A grounded example (`metamask-extension` #42583).** A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. + +🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: + +```typescript +type NetworkControllerState = { + networkConfigurationsByChainId?: Record< + string, + { + defaultRpcEndpointIndex?: number; + rpcEndpoints?: { networkClientId?: string }[]; + } + >; +}; +``` + +✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: + +```typescript +import type { NetworkState } from '@metamask/network-controller'; + +type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +``` + +A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. + **Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From be1dc98c5c1dfafaddde569ec0fb07181ff1a31e Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 13:09:53 -0400 Subject: [PATCH 03/20] Split typescript-typing into a `typescript` domain: avoid-any, derive-types, decompose-large-files --- CHANGELOG.md | 2 +- .../coding/skills/typescript-typing/skill.md | 84 ------------------- domains/typescript/skills/avoid-any/skill.md | 46 ++++++++++ .../skills/decompose-large-files/skill.md | 46 ++++++++++ .../typescript/skills/derive-types/skill.md | 51 +++++++++++ 5 files changed, 144 insertions(+), 85 deletions(-) delete mode 100644 domains/coding/skills/typescript-typing/skill.md create mode 100644 domains/typescript/skills/avoid-any/skill.md create mode 100644 domains/typescript/skills/decompose-large-files/skill.md create mode 100644 domains/typescript/skills/derive-types/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fe0994a..65fe0f20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript-typing` skill (coding domain): the reasoning layer beneath `coding-guidelines`' TypeScript section — `any` as a type-checking-off directive with substitute-by-position guidance (assignee → `unknown`, assigned → `never`) and the generic-constraint exception, plus derive-types-from-authoritative-sources over ad-hoc declarations. Experimental. +- Add `typescript` domain (experimental) with three skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; the one exception is a generic constraint), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), and `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration). ## [0.2.0] diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md deleted file mode 100644 index 1b4d23d9..00000000 --- a/domains/coding/skills/typescript-typing/skill.md +++ /dev/null @@ -1,84 +0,0 @@ ---- -name: typescript-typing -description: TypeScript typing discipline — treat `any` as a directive that disables type checking (substitute by position: assignee → `unknown`, assigned → `never`), and derive types from authoritative sources rather than hand-writing ad-hoc ones that duplicate and drift. -maturity: experimental ---- - -# TypeScript Typing Discipline - -Deepens the TypeScript section of `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This skill is the reasoning layer underneath those bullets: why `any` is dangerous and how to replace it by position, and how to avoid hand-writing a type that an authoritative source already defines. Grounded in `docs/typescript.md` (§ Avoid `any`, § Prefer type inference). - -## `any` is not a type — it is a directive that disables type checking - -The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: - -- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. -- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. -- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. -- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. - -## Substitute `any` by position — assignee vs assigned - -Identify which side of an assignment the `any` sits on: - -- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. - - 🚫 `type Fn = () => any; const xs: any[]` - - ✅ `type Fn = () => unknown; const xs: unknown[]` -- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. - -## The one acceptable exception — generic *constraints* - -`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. - -```typescript -class BaseController< - // eslint-disable-next-line @typescript-eslint/no-explicit-any - Messenger extends RestrictedMessenger, -> // ... -``` - -Three conditions attach: - -- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. -- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. -- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. - -When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." - -## Derive types from authoritative sources — don't hand-write ad-hoc ones - -When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. This is the structural form of the contributor-docs rule *Prefer type inference over annotations and assertions*: inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." - -An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: - -- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. -- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. -- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. - -**A grounded example (`metamask-extension` #42583).** A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. - -🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: - -```typescript -type NetworkControllerState = { - networkConfigurationsByChainId?: Record< - string, - { - defaultRpcEndpointIndex?: number; - rpcEndpoints?: { networkClientId?: string }[]; - } - >; -}; -``` - -✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: - -```typescript -import type { NetworkState } from '@metamask/network-controller'; - -type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; -``` - -A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. - -**Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md new file mode 100644 index 00000000..b7a207d9 --- /dev/null +++ b/domains/typescript/skills/avoid-any/skill.md @@ -0,0 +1,46 @@ +--- +name: avoid-any +description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`); the one exception is a generic constraint, declared with an inline eslint-disable. +maturity: experimental +--- + +# Avoid `any` + +Deepens the TypeScript guidance in `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This is the reasoning layer beneath that bullet: why `any` is dangerous, and how to replace it by position. Grounded in `docs/typescript.md` (§ Avoid `any`). + +## `any` is not a type — it is a directive that disables type checking + +The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: + +- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. +- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. +- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. +- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. + +## Substitute `any` by position — assignee vs assigned + +Identify which side of an assignment the `any` sits on: + +- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. + - 🚫 `type Fn = () => any; const xs: any[]` + - ✅ `type Fn = () => unknown; const xs: unknown[]` +- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. + +## The one acceptable exception — generic *constraints* + +`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. + +```typescript +class BaseController< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Messenger extends RestrictedMessenger, +> // ... +``` + +Three conditions attach: + +- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. +- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. +- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. + +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md new file mode 100644 index 00000000..99a749d5 --- /dev/null +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -0,0 +1,46 @@ +--- +name: decompose-large-files +description: Decompose a large file into coherent, independently-mergeable modules to improve modularity, maintainability, reviewability, and code organization — and to unblock incremental TS migration. Extract by coherent subject/domain cluster; never fragment for its own sake. +maturity: experimental +--- + +# Decompose Large Files By Coherent Units + +A file that has grown to thousands of lines is hard to review, hard to maintain, and — if it is still `.js` — hard to migrate to TypeScript in one pass. Decompose it by moving coherent subject/domain clusters into their own modules. The goal is **modularity, maintainability, reviewability, and code organization**; unblocking an incremental JS→TS migration is a direct benefit, because each extracted module becomes a small, independently-typable unit. + +Reference application: `metamask-extension` #41735 (`MetamaskController` decomposition, 9,260 → ~3,500 lines). + +## The decision that matters: what is a coherent unit? + +Extraction is worth it only when the extracted piece is a **coherent unit that can move independently**. Apply this judgment _before_ proposing any module: + +- **Coherent subject.** The cluster is about one thing — one domain, one lifecycle, one concern (phishing detection, metrics emission, badge rendering). A reader can state its responsibility in one sentence. +- **Independently mergeable.** It can be lifted behind a defined seam — injected dependencies, or a messenger action — without dragging half the file with it. Its coupling to shared state and to the composition root is small and nameable. +- **Not fragmentation.** Extraction for its own sake — splitting a cohesive routine across files, or pulling out a 20-line helper that only one caller uses and has no independent identity — makes the code _harder_ to follow, not easier. If pulling the piece out means the two halves must still change together, leave it inline. **Refactoring is a means to modularity, not an end; a change that raises the file count without raising coherence is a regression.** + +Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. + +## How to extract one unit (self-contained, per #41735) + +Each extraction is one self-contained change — no separate "final deletion" or "integration" ticket: + +1. **Scaffold** the module (its own file/dir, TypeScript from the start). +2. **Port** the bodies in, unchanged in behavior. +3. **Define the seam** — inject the dependencies the module needs (or register its public methods as messenger actions) instead of reaching back into the file's globals. This is where the human judgment is. +4. **Rewire** the call sites to go through the seam. +5. **Delete the original** in the same change; leave no forwarding stub. +6. **Add a structural unit test** against a stub/mock of the seam — especially valuable when the original file had no tests. + +Steps 1, 2, 5, 6 are largely mechanical (codemod territory — `jscodeshift` on the source, `ts-morph` on the module). Step 3/4 is the part that needs a person. + +## Sizing and sequencing + +- **Size each unit S / M / L / XL** by body size × coupling to rewire. Ship one module (or one subject area) per PR — reviewer context window is usually the binding constraint, so a focused S/M PR merges where an XL one stalls. +- **Sequence lowest-coupling-first.** Extract the clusters with the smallest, cleanest seam first: they establish the pattern and shrink the file so later, more-entangled extractions are easier to see. Save the most coupled cluster (often the core lifecycle) for last, or leave it as the composition root. +- **Enforce the new boundary** so the file cannot silently re-absorb the module — an ESLint `import/no-restricted-paths` rule on the module directory (per #41735). + +## When NOT to decompose + +- The file is large but **already cohesive** — one subject, read top to bottom. Size alone is not a reason. +- The only available splits are **arbitrary** (by line count, or a catch-all `utils`) rather than by subject. That produces fragments, not modules. +- The extraction **cannot get a clean seam** — everything it touches is shared mutable state with the rest of the file. Fix the coupling first, or leave it inline and say so. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md new file mode 100644 index 00000000..84bdbe7c --- /dev/null +++ b/domains/typescript/skills/derive-types/skill.md @@ -0,0 +1,51 @@ +--- +name: derive-types +description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer`) instead of hand-writing ad-hoc types that duplicate, run too wide, and drift. +maturity: experimental +--- + +# Derive Types From Authoritative Sources + +Deepens the TypeScript guidance in `mms-coding-guidelines`, and is the structural counterpart to the contributor-docs rule *Prefer type inference over annotations and assertions* (`MetaMask/contributor-docs` `docs/typescript.md`). Where inference handles values, derivation handles types that reference other types. + +## Derive, don't re-declare + +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: + +- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. +- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. +- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. + +## A grounded example (`metamask-extension` #42583) + +A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. + +🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: + +```typescript +type NetworkControllerState = { + networkConfigurationsByChainId?: Record< + string, + { + defaultRpcEndpointIndex?: number; + rpcEndpoints?: { networkClientId?: string }[]; + } + >; +}; +``` + +✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: + +```typescript +import type { NetworkState } from '@metamask/network-controller'; + +type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +``` + +A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. + +## Rule + +Before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From a89095d6301960892cce4e1b7a619d6c1c9a0da0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 05:39:05 -0400 Subject: [PATCH 04/20] Add "why decompose first" rationale to `decompose-large-files`: boundary-identification is step one of any migration --- domains/typescript/skills/decompose-large-files/skill.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 99a749d5..081192b5 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -10,6 +10,12 @@ A file that has grown to thousands of lines is hard to review, hard to maintain, Reference application: `metamask-extension` #41735 (`MetamaskController` decomposition, 9,260 → ~3,500 lines). +## Why decompose first — even if the file could convert in one pass + +Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. + +And a single-pass conversion is impractical even for a capable AI. Converting a multi-thousand-line file in one PR means holding the entire file in context **and** progressively loading every upstream file whose source types the code should derive from (see `derive-types`) **and** every downstream file that imports it and must be updated. That context fan-in (source types) and fan-out (consumers) is the real cost — not the line count of the file itself. Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. + ## The decision that matters: what is a coherent unit? Extraction is worth it only when the extracted piece is a **coherent unit that can move independently**. Apply this judgment _before_ proposing any module: From 388dd98f3debcb003d1ac78ae27a393d8980419b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 05:51:52 -0400 Subject: [PATCH 05/20] Add `migration-context-cost` skill (fan-in/fan-out) + avoid-any second exception (bivariant callback `any`) --- CHANGELOG.md | 2 +- domains/typescript/skills/avoid-any/skill.md | 32 +++++++++++++++++-- .../skills/decompose-large-files/skill.md | 2 +- .../skills/migration-context-cost/skill.md | 27 ++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 domains/typescript/skills/migration-context-cost/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 65fe0f20..8c839195 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript` domain (experimental) with three skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; the one exception is a generic constraint), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), and `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration). +- Add `typescript` domain (experimental) with four skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; two narrow exceptions: generic constraints and bivariant callback parameters), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration), and `migration-context-cost` (a file's JS→TS migration cost is dominated by context fan-in + fan-out, not line count — scope and sequence tickets by it). ## [0.2.0] diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index b7a207d9..b9256e87 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -1,6 +1,6 @@ --- name: avoid-any -description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`); the one exception is a generic constraint, declared with an inline eslint-disable. +description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`). Two narrow exceptions: a generic constraint, and a callback parameter caught in a bivariant position between two fixed, irresolvable function-type constraints — both declared with an inline eslint-disable. maturity: experimental --- @@ -26,7 +26,7 @@ Identify which side of an assignment the `any` sits on: - ✅ `type Fn = () => unknown; const xs: unknown[]` - **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. -## The one acceptable exception — generic *constraints* +## Acceptable exception 1 — generic *constraints* `any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. @@ -43,4 +43,32 @@ Three conditions attach: - **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. - **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. +## Acceptable exception 2 — a callback parameter between two irresolvable function-type constraints + +A callback's parameter may be `any` when all three hold: + +1. **Bivariant position** — the callback is _assignable to_ a wider function type **and** an _assignee of_ a narrower one. +2. **Irresolvable** — no concrete type satisfies both directions, because the wider function type is not a supertype of the narrower (`WideParam extends T extends NarrowParam` has no solution). +3. **Fixed** — neither constraint can be redesigned without breaking callers or losing accuracy. + +Under `--strictFunctionTypes` parameters are contravariant, so the callback's parameter must be a _supertype_ of the outer slot's (outward: `unknown` ✓, `never` ✗) **and** a _subtype_ of the incoming value's (inward: `never` ✓, `unknown` ✗). `any` is the only inhabitant of both the top and bottom of the assignability lattice, so it is the only escape. (Return types stay covariant — keep `unknown`.) + +🚫 If the constraints are **redesignable**, the contravariance error is a real design smell — fix it (usually by parametrizing with a generic), don't suppress: + +```typescript +declare const acceptGeneric: (handler: (event: E) => void) => void; +acceptGeneric(onA); // no `any` needed +``` + +✅ Only when the constraints are **fixed and irresolvable**: + +```typescript +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Bivariant position with irresolvable, fixed constraints +let bridge: (x: any) => void; +``` + +Like the generic-constraint case, this `any` is **not infectious** — it is scoped to one parameter position, and both constraint types re-impose their signatures at each use site. Annotate the `eslint-disable` with the criteria so a reviewer can check them. Caveat: the safety claim holds only if the constraint types are accurate — a fixed constraint that is itself imprecise (a library type that is `any` internally) still forces the bridge `any` but no longer preserves safety at the use sites. + +Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. + When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 081192b5..5d91e44f 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -14,7 +14,7 @@ Reference application: `metamask-extension` #41735 (`MetamaskController` decompo Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. -And a single-pass conversion is impractical even for a capable AI. Converting a multi-thousand-line file in one PR means holding the entire file in context **and** progressively loading every upstream file whose source types the code should derive from (see `derive-types`) **and** every downstream file that imports it and must be updated. That context fan-in (source types) and fan-out (consumers) is the real cost — not the line count of the file itself. Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. +And a single-pass conversion is impractical even for a capable AI — the binding constraint is the file's **context fan-in and fan-out** (upstream source types + downstream consumers), not its line count (see `migration-context-cost`). Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. ## The decision that matters: what is a coherent unit? diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md new file mode 100644 index 00000000..9ff9b6d6 --- /dev/null +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -0,0 +1,27 @@ +--- +name: migration-context-cost +description: A file's JS→TS migration cost is dominated by context fan-in (upstream files whose source types to derive from) + fan-out (downstream files that import it and must update), not its line count. Scope and sequence migration tickets by this cost. +maturity: experimental +--- + +# TypeScript Migration Context Cost — Fan-In and Fan-Out + +The cost of converting a file to TypeScript is not its line count. It is the **context the conversion pulls in** — everything the converter, human or AI, must load to do it correctly. + +## The two axes + +- **Fan-in (upstream source types).** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from — controller state, function returns, library exports, schemas (see `derive-types`). A file that touches many upstream types has high fan-in even if it is short. +- **Fan-out (downstream consumers).** Changing the file's types ripples to every module that imports it; each may need its own update or re-type. A widely-imported file has high fan-out even if it is small. + +The real work — and the real review surface — is the sum of these, not the target file's length. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. + +## Why it matters even for AI + +Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** progressively load every upstream source-type file **and** every downstream consumer. That context fan-in/fan-out is the binding constraint, not the model's ability to read the file itself. + +## How to use it + +- **Scope tickets by context cost, not LOC.** Estimate fan-in (upstream types the file derives from) + fan-out (`grep -rl` importer count) before sizing a migration ticket. Size by the import-rewrite / re-type surface, not the line count. +- **Sequence low-cost first.** Convert leaf / low-fan-out files early (few downstream updates), and files whose upstream types are already TypeScript (low fan-in), so later conversions have more typed ground to derive from. +- **When the cost won't fit one PR, reduce it before converting.** High fan-in → the upstream types may need to land first. High fan-out through a hub → decompose the hub into coherent units so each unit's fan-out is bounded (see `decompose-large-files`). Decomposition is one response to high context cost — not the only place the cost applies. +- **Keep the fan-out map honest.** A barrel / re-export file inflates apparent fan-out and hides real dependencies; importing from the actual source file (not a barrel) keeps the dependency graph — and the cost estimate — accurate. From 8483d91e835f30a2f8297e7b96f319992afc1ac1 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 06:09:45 -0400 Subject: [PATCH 06/20] Fix migration-context-cost inaccuracies, reframe decompose how-to around unit conversion, swap in the stronger derive-types example, slim CHANGELOG - migration-context-cost: line count is a factor not a non-factor; fan-in is a reading cost (not a change/review surface), fan-out is the change surface; drop the wrong "upstream types land first" and off-topic barrel bullet - decompose-large-files: the point is converting to TS in small self-contained units, not extraction - derive-types: replace the NetworkState restatement with the reinvented-messenger + hand-copied-return example (derive via `ReturnType`) - CHANGELOG: list the domain, not each skill --- CHANGELOG.md | 2 +- .../skills/decompose-large-files/skill.md | 4 +- .../typescript/skills/derive-types/skill.md | 39 ++++++++++++------- .../skills/migration-context-cost/skill.md | 21 +++++----- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c839195..7f0f46d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript` domain (experimental) with four skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; two narrow exceptions: generic constraints and bivariant callback parameters), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration), and `migration-context-cost` (a file's JS→TS migration cost is dominated by context fan-in + fan-out, not line count — scope and sequence tickets by it). +- Add `typescript` domain (experimental) — TypeScript authoring and JS→TS migration guidance. ## [0.2.0] diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 5d91e44f..f8a13708 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -26,9 +26,9 @@ Extraction is worth it only when the extracted piece is a **coherent unit that c Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. -## How to extract one unit (self-contained, per #41735) +## How to convert one unit (self-contained, per #41735) -Each extraction is one self-contained change — no separate "final deletion" or "integration" ticket: +The extraction is not the point — **converting the file to TypeScript in small, self-contained units is.** Each unit is a piece you can type and review on its own instead of holding the whole file at once; pulling it into its own module is just what makes that unit self-contained and independently convertible. Each unit is one self-contained change — no separate "final deletion" or "integration" ticket: 1. **Scaffold** the module (its own file/dir, TypeScript from the start). 2. **Port** the bodies in, unchanged in behavior. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md index 84bdbe7c..242238d3 100644 --- a/domains/typescript/skills/derive-types/skill.md +++ b/domains/typescript/skills/derive-types/skill.md @@ -20,31 +20,44 @@ An ad-hoc type — one hand-defined to describe a value an authoritative type al ## A grounded example (`metamask-extension` #42583) -A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. +A `wallet-services` module hand-rolled a messenger type, re-declaring each controller action's signature and **hand-copying its return shape** inline. -🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: +🚫 Reinvents the controller's messenger and re-states its action returns: ```typescript -type NetworkControllerState = { - networkConfigurationsByChainId?: Record< - string, - { - defaultRpcEndpointIndex?: number; - rpcEndpoints?: { networkClientId?: string }[]; - } +type TokenResolutionMessenger = { + call( + action: 'AssetsContractController:getTokenStandardAndDetails', + address: string, + // … + ): Promise< + | { + balance?: string | number | bigint | { toString(radix?: number): string }; + decimals?: string | number | bigint | { toString(radix?: number): string }; + standard?: string; + symbol?: string; + } + | undefined >; + // the other action's return is discarded entirely: + call(action: 'AssetsContractController:getBalancesInSingleCall' /* … */): Promise; }; ``` -✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: +✅ Derive each return from the controller's exported action type; don't hand-copy it: ```typescript -import type { NetworkState } from '@metamask/network-controller'; +import type { AssetsContractControllerGetTokenStandardAndDetailsAction } from '@metamask/assets-controllers'; -type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +// the action already types its own return — derive it +type TokenDetails = ReturnType< + AssetsContractControllerGetTokenStandardAndDetailsAction['handler'] +>; ``` -A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. +The messenger itself should extend the controller's `RestrictedMessenger` parameterized with those exported action types, so every `call` signature comes from the controller rather than a hand-rolled overload. The hand-rolled version is worse than a plain duplicate: one return is hand-copied (already looser than the controller's real type), the other (`Promise`) discards the type entirely. + +The same PR also typed a dependency `getMetaMaskState: () => Record`, which forced every consumer to re-cast the shape by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. That downstream cast tax is what a too-wide type always imposes; deriving the dependency from the authoritative state type deletes it. Notably the same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand — the discipline is extending it to every referenced type. ## Rule diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md index 9ff9b6d6..ea1ca3b4 100644 --- a/domains/typescript/skills/migration-context-cost/skill.md +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -1,27 +1,26 @@ --- name: migration-context-cost -description: A file's JS→TS migration cost is dominated by context fan-in (upstream files whose source types to derive from) + fan-out (downstream files that import it and must update), not its line count. Scope and sequence migration tickets by this cost. +description: A file's JS→TS migration cost is driven mostly by context fan-in (upstream types it must read to derive from) and fan-out (downstream files that import it and must be updated), not by its line count. Scope and sequence migration tickets by it. maturity: experimental --- # TypeScript Migration Context Cost — Fan-In and Fan-Out -The cost of converting a file to TypeScript is not its line count. It is the **context the conversion pulls in** — everything the converter, human or AI, must load to do it correctly. +A file's line count is a factor in how hard it is to convert to TypeScript, but usually not the dominant one. The dominant cost is the **context the conversion pulls in** — everything the converter, human or AI, must read or touch to do it correctly. -## The two axes +## The two axes are different kinds of cost -- **Fan-in (upstream source types).** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from — controller state, function returns, library exports, schemas (see `derive-types`). A file that touches many upstream types has high fan-in even if it is short. -- **Fan-out (downstream consumers).** Changing the file's types ripples to every module that imports it; each may need its own update or re-type. A widely-imported file has high fan-out even if it is small. +- **Fan-in (upstream source types) — a _reading_ cost.** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from: controller state, function returns, library exports, schemas (see `derive-types`). You read these; you do not change them, and they are usually already TypeScript. A file that derives from many sources is expensive to hold in context even if it is short. +- **Fan-out (downstream consumers) — a _change_ cost.** Re-typing the file ripples to every module that imports it; each may need its own update. These are files you actually edit, so fan-out — together with the file itself — is the **change and review surface**. A widely-imported file has a large change surface even if it is small. -The real work — and the real review surface — is the sum of these, not the target file's length. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. +The two are not the same kind of cost: fan-in is what you must _read_ to get the types right; fan-out is what you must _modify_. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. ## Why it matters even for AI -Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** progressively load every upstream source-type file **and** every downstream consumer. That context fan-in/fan-out is the binding constraint, not the model's ability to read the file itself. +Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** read every upstream source-type file **and** edit every downstream consumer. That context load plus change surface — not the model's ability to read the file itself — is the binding constraint. ## How to use it -- **Scope tickets by context cost, not LOC.** Estimate fan-in (upstream types the file derives from) + fan-out (`grep -rl` importer count) before sizing a migration ticket. Size by the import-rewrite / re-type surface, not the line count. -- **Sequence low-cost first.** Convert leaf / low-fan-out files early (few downstream updates), and files whose upstream types are already TypeScript (low fan-in), so later conversions have more typed ground to derive from. -- **When the cost won't fit one PR, reduce it before converting.** High fan-in → the upstream types may need to land first. High fan-out through a hub → decompose the hub into coherent units so each unit's fan-out is bounded (see `decompose-large-files`). Decomposition is one response to high context cost — not the only place the cost applies. -- **Keep the fan-out map honest.** A barrel / re-export file inflates apparent fan-out and hides real dependencies; importing from the actual source file (not a barrel) keeps the dependency graph — and the cost estimate — accurate. +- **Scope tickets by context cost, not LOC.** Before sizing a migration ticket, estimate fan-in (how many upstream types it must derive from) and fan-out (`grep -rl` importer count). Size by the read-context plus the change surface, not the line count. +- **Sequence low-fan-out first.** Convert leaf / low-fan-out files early — few downstream edits per PR. Files whose upstream types are already TypeScript are cheaper on the fan-in side and give later conversions more typed ground to derive from. +- **When the change surface won't fit one PR, reduce it first.** A hub with high fan-out is the case to decompose: split it into coherent units so each unit's fan-out — and thus each PR — is bounded (see `decompose-large-files`). Decomposition is one response to high context cost, not the only place the cost applies. From 801758fa0f682296ac676f1e1b90a1dbd69622f0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 06:50:52 -0400 Subject: [PATCH 07/20] Reframe decompose how-to: identifying the boundaries is the key; extraction is optional --- domains/typescript/skills/decompose-large-files/skill.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index f8a13708..dc1ccbf5 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -26,9 +26,13 @@ Extraction is worth it only when the extracted piece is a **coherent unit that c Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. -## How to convert one unit (self-contained, per #41735) +## Identifying the boundaries is the key — extraction is optional -The extraction is not the point — **converting the file to TypeScript in small, self-contained units is.** Each unit is a piece you can type and review on its own instead of holding the whole file at once; pulling it into its own module is just what makes that unit self-contained and independently convertible. Each unit is one self-contained change — no separate "final deletion" or "integration" ticket: +**The key step is identifying the coherent boundaries.** Once you know them, you can convert the file to TypeScript in small, self-contained units — type and review one cluster at a time — instead of holding the whole file at once. That is true whether or not you physically move anything: the boundary map is what makes incremental conversion possible, and documenting it (one unit per ticket) is the deliverable. + +**Extraction — moving a unit into its own module — is optional.** It buys modularity, reviewability, and a bounded per-unit change surface, so it is often worth doing, but you convert in units because you identified the boundaries, not because you moved the code. Extract where the move adds value; leave a cluster in place where it doesn't. + +When you *do* extract a unit, it is one self-contained change — no separate "final deletion" or "integration" ticket: 1. **Scaffold** the module (its own file/dir, TypeScript from the start). 2. **Port** the bodies in, unchanged in behavior. From 712d17a406665e6357221f3cb0796ab75ef1ca88 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:09:53 -0400 Subject: [PATCH 08/20] =?UTF-8?q?Drop=20the=20CHANGELOG=20entry=20?= =?UTF-8?q?=E2=80=94=20it=20is=20for=20the=20CLI=20package,=20not=20skills?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG.md tracks consumer-facing changes to the `@metamask/skills` package, per CONTRIBUTING's "CLI / tooling changes" section. No merged skill-only PR adds an entry (#80, #78, #70, #62, #61 all touch zero changelog lines). It was also the sole source of this branch's conflict with `main`, since every skill PR edits the same `[Unreleased]` block. --- CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f0f46d6..8557ff8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added - -- Add `typescript` domain (experimental) — TypeScript authoring and JS→TS migration guidance. - ## [0.2.0] ### Added From 8dc5b86b0da7720f0d6270f8a969a2570679eb74 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 11:22:59 -0400 Subject: [PATCH 09/20] Fold in `typescript-compiler-blindspots`, moved from the `testing` domain Was a separate PR against `domains/testing`. It belongs here: its subject is whether a hand-written type agrees with its authoritative source, which is the question `derive-types` answers from the authoring side, and it shares this domain's premise that a green `tsc` is not evidence the types are correct. Directory name and frontmatter `name` already agree; only the domain moved. --- .../references/false-negatives.md | 259 +++++++++++++++++ .../references/metamask-extension.md | 54 ++++ .../references/worked-example.md | 92 ++++++ .../scripts/substitution-ab.sh | 51 ++++ .../typescript-compiler-blindspots/skill.md | 272 ++++++++++++++++++ 5 files changed, 728 insertions(+) create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md create mode 100755 domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/skill.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md new file mode 100644 index 00000000..5bcf97b8 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md @@ -0,0 +1,259 @@ +# The standing blind spots — where `tsc` returns a false negative + +Defects the compiler cannot report, independent of any particular PR. Unlike the +restated-type class, these need no substitution to find: they are properties of +the language and the config, and they are present in every file. + +**Verified, not asserted.** The demonstration file below was typechecked against +`metamask-extension` at `7fafda0` with the repo's own `tsconfig.json`: + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Every block in it is wrong. `tsc` reports **zero errors** on all ten. + +--- + +## A. Unsoundness in the type system + +### 1. Index access is not `| undefined` + +```ts +const parts = host.split('.'); +return parts[9]; // typed `string`; `undefined` at runtime +``` + +`arr[i]` and `record[key]` are typed as if the element always exists. Easy to underestimate how much surface this covers: every `.split()[n]`, every +lookup table, every `find`-then-index is an instance. + +- **Flag:** `noUncheckedIndexedAccess` (not enabled in `metamask-extension`). +- **In review:** any index or dynamic key access on a path that can be empty or + short. `parts[parts.length - 1]` is only safe if the array is provably non-empty. + +### 2. Optional property vs. explicit `undefined` + +```ts +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; // accepted +``` + +`?:` means "may be absent"; without the strict flag it *also* accepts +present-and-undefined. Code that distinguishes the two (`'k' in obj`, +`Object.keys().length`, serialization that drops vs. writes `null`) breaks on a +distinction the type cannot express. + +- **Flag:** `exactOptionalPropertyTypes` (not enabled). +- **In review:** persisted state and message payloads, where absent and + `undefined` serialize differently. + +### 3. Method parameters are bivariant + +```ts +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) { … } }; // accepted +``` + +`strictFunctionTypes` makes function *properties* contravariant but exempts +**method shorthand** — deliberately, for DOM/array compatibility. So a handler +that only accepts a narrow subtype satisfies a wide handler type and receives +values it declared it would not. + +- **Fix:** declare callbacks as properties (`handle: (msg: …) => void`), which + *is* checked. +- **In review:** any interface with method-shorthand callbacks, especially + message/event handlers. + +### 4. Arrays are covariant + +```ts +const bases: Base[] = specials; // Special[] → Base[], accepted +bases.push(new Base()); // `specials` now holds a non-Special +``` + +- **In review:** a narrower array widened and then mutated. `readonly T[]` blocks it. + +### 5. Excess-property checking only fires on fresh literals + +```ts +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +const params: CreateParams = draft; // no error — not a fresh literal +``` + +Assigning the literal directly would catch the typo. Through a variable, the +extra property is silently ignored — and the intended one is missing. + +- **In review:** config/params objects built up in a variable before being passed. + This is how a misspelled option key survives to runtime. + +### 6. Structural typing erases domain distinctions + +```ts +type AccountAddress = string; +type TransactionHash = string; +fetchBalance(txHash); // accepted — both are `string` +``` + +Aliases are not nominal. Two semantically incompatible values are interchangeable +whenever their structure matches. + +- **Fix:** branded types, or a template-literal type where the format differs + (`Hex` = `` `0x${string}` `` genuinely does discriminate). +- **In review:** same-primitive parameters, especially adjacent ones in a + signature, where swapping the arguments would still compile. + +## B. Boundaries the compiler does not cross + +### 7. `any` absorbs any annotation + +```ts +declare function readPersisted(key: string): any; +const meta: VaultMeta = readPersisted('meta'); // asserted, never validated +meta.version.toFixed(2); // may be a string at runtime +``` + +An `any` satisfies every annotation silently. Sources: untyped dependencies, +`JSON.parse`, generics that default to `any`, and `as any`. + +- **In review:** trace where a confidently-typed value *entered* the program. If + it entered as `any`, its type is a wish. + +### 8. Ambient `declare module` is an unverified assertion + +```ts +declare module '@ensdomains/content-hash' { + const contentHash: { decode: (h: string) => string /* … */ }; + export default contentHash; +} +``` + +Hand-written module declarations are believed unconditionally — nothing compares +them to the package. Getting a return type wrong here is invisible forever, and +the declaration is **global**, so it also shadows any real types the package +later ships. + +- **In review:** read the package's actual source at the installed version when a + `declare module` is added or changed. Prefer `@types/*` or a PR upstream. + +### 9. External data is asserted, not validated + +```ts +const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +Every `as` on data crossing a boundary — RPC responses, `fetch().json()`, +`chrome.storage` reads, persisted state written by an *older version of the app* — +is a claim the compiler cannot evaluate. + +- **In review:** highest stakes for persisted state and migrations, where the real + input was produced by code that no longer exists. A runtime validator + (`@metamask/superstruct`, zod) is the only thing that actually checks. + +### 10. A JS caller is not checked at all + +With `checkJs` off (the default, and the case in `metamask-extension`), a type +written for a function whose callers are still `.js` is compared against no call +site, ever. See the restated-type class in the main skill — this is why that class +exists. + +## C. Config-level blind spots + +Check these before trusting a green build. Values shown are `metamask-extension` +at the time of writing. + +| Setting | Effect | Here | +|---|---|---| +| `skipLibCheck` | Errors *inside* `.d.ts` files are suppressed, including conflicts between library types | **`true`** (from `@tsconfig/node22`) | +| `exclude` | Excluded files are never typechecked | `**/*.stories.tsx`, `**/*.stories.ts` | +| `include` | Anything outside it is invisible to `tsc` | `app`, `development`, `shared`, `test`, `types`, `ui`, `*.ts` | +| `checkJs` | Off ⇒ `.js` callers unchecked | unset | +| `noEmit` + bundler | `tsc` never produces the shipped artifact; webpack/swc transpiles **without** typechecking, so a type error cannot break the build — only the separate `lint:tsc` job reports it | `noEmit: true` | +| `@ts-expect-error` / `@ts-ignore` | Point suppressions | grep before trusting a clean file | + +The last row is worth stating plainly: **type errors do not break the build.** They +break a CI job. If that job is skipped, filtered, or its output is not read, the +types were never checked at all. + +--- + +## The demonstration file + +Drop this anywhere inside the `include` paths and typecheck. Zero errors is the +expected — and alarming — result. + +```ts +/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars */ + +// 1. Index access is not `| undefined` +function lastSegment(host: string): string { + const parts = host.split('.'); + return parts[9]; // typed `string`; `undefined` at runtime +} +export const seg = lastSegment('foo.eth').toUpperCase(); + +// 2. Record lookup claims the value always exists +declare const gateways: Record; +export const gw = gateways.definitelyNotAKey.url; + +// 3. Optional property accepts an explicit undefined +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; +export const idPlusOne = (cleared.currentPopupId ?? 0) + 1; + +// 4. Method-shorthand parameters are bivariant +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) {} }; +export { narrow }; + +// 5. Arrays are covariant +class Base {} +class Special extends Base { + special() { + return 1; + } +} +const specials: Special[] = [new Special()]; +const bases: Base[] = specials; +bases.push(new Base()); +export const boom = () => specials.map((s) => s.special()); + +// 6. Excess-property checking only fires on fresh literals +type CreateParams = { url: string; justification: string }; +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +export const params: CreateParams = draft; + +// 7. `any` absorbs any annotation +declare function readPersisted(key: string): any; +type VaultMeta = { version: number; storageKind: 'data' | 'split' }; +export const meta: VaultMeta = readPersisted('meta'); +export const ver = meta.version.toFixed(2); + +// 8. An ambient `declare module` is an unverified assertion +import contentHash from '@ensdomains/content-hash'; + +export const decoded: string = contentHash.decode('0x'); + +// 9. Structural typing erases domain distinctions +type AccountAddress = string; +type TransactionHash = string; +declare function fetchBalance(addr: AccountAddress): Promise; +declare const txHash: TransactionHash; +export const wrong = fetchBalance(txHash); + +// 10. A type assertion on external data is unchecked by construction +declare const rpcResult: unknown; +export const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +## How to use the catalog in a review + +Don't run all ten as a checklist. Pick by what the diff touches: + +- **New indexing / destructuring** → 1, 2 +- **New message, event, or callback types** → 3, 5, 6 +- **New `declare module`, new dependency, `@types` change** → 8 +- **Anything reading persisted state, storage, or an RPC response** → 7, 9 +- **A JS→TS conversion** → 10, plus the restated-type class in the main skill +- **Any PR whose safety argument is "CI is green"** → section C, first diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md new file mode 100644 index 00000000..36a67496 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md @@ -0,0 +1,54 @@ +# Repo notes — metamask-extension + +Specifics for running the two-arm proof in `MetaMask/metamask-extension`. (Kept +here rather than in `repos/` on purpose: a `repos/` subdir containing only an +extension overlay would make this skill *skip* installs for mobile and core.) + +## Typecheck invocation + +```bash +NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +``` + +`package.json`'s `lint:tsc` uses `--max-old-space-size=6144`, which **OOMs** on a +full run on a 16 GB machine — and the OOM exits non-zero with no type diagnostics, +so a naive exit-code check reads it as "errors found." Raise the heap and read the +output. A full run takes roughly 3–5 minutes. + +## Where to put probes + +Anywhere under the `include` list — `app`, `development`, `shared`, `test`, +`types`, `ui`. `app/scripts/derive-probe/` works. Delete it afterwards; it is +inside the build's include paths. + +## What the compiler is *not* checking + +- **`checkJs` is unset** and there is no `// @ts-check` in `app/scripts/background.js`. + `background.js` is the sole caller of much of `app/scripts/lib/**`, so a type + written for those functions is validated against **nothing**. This is where + migration PRs accumulate silent divergence, and where this skill pays. +- `tsconfig.json` sets `lib: ["DOM", "es2023"]`, overriding the base. **`webworker` + is absent**, so service-worker globals (`clients`, `Clients`, `Client`) have no + authoritative type in scope — hand-declaring them is legitimate here. +- Strictness comes from `@tsconfig/node22` (`strict: true`), so `strictNullChecks` + is on and nullability divergences do surface in a probe. + +## Authoritative sources worth knowing + +| Looking for | Derive from | +|---|---| +| current chain id | `ReturnType` (`shared/lib/selectors/networks.ts`) → `Hex`, not `string` | +| the EIP-1193 provider | `ReturnType['provider']` — note the `\| undefined` | +| a controller method's params | `SomeController['methodName']` | +| persisted state root | `MetaMaskStorageStructure` (`shared/lib/stores/base-store.ts`) | +| a `browser.*` listener payload | `browser.WebRequest.OnErrorOccurredDetailsType` and siblings, from `webextension-polyfill` | +| offscreen message targets/events | the enums in `shared/constants/offscreen-communication.ts` | + +## Before "fixing" a `chrome.*` type error + +Read the declaration in `node_modules/@types/chrome/index.d.ts` first. Several +parameters are template-literal **string** types, not the enums they mirror — e.g. +`ContextFilter.contextTypes?: ` `` `${ContextType}`[] `` and +`CreateParameters.reasons: ` `` `${Reason}`[] ``. A plain string literal already +satisfies them, so swapping in `chrome.offscreen.Reason.X` is an unnecessary +runtime change, not a type fix. diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md b/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md new file mode 100644 index 00000000..a7f1f978 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md @@ -0,0 +1,92 @@ +# Worked example — a JS→TS migration PR + +[metamask-extension#44397](https://github.com/MetaMask/metamask-extension/pull/44397), +head `7fafda0`. 11 files converted, +153/−72, described as *"mostly mechanical +JS→TS with equivalent runtime logic"*, four files *"rename only"*. All CI green, +including `lint:tsc`. + +Twelve hand-written types. Nine had an authoritative source. Five disagreed with it. + +## Arm A + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Silent — so every diagnostic below is attributable to the substitution. + +## Arm B + +Six probe files, each substituting the derived type and calling it as the real +code does: + +``` +probe-1-get-obj-structure.ts(19,47): error TS2345: Argument of type + 'MetaMaskStorageStructure | undefined' is not assignable to parameter of type + 'Record'. +probe-2-set-current-popup-id.ts(22,21): error TS2345: Argument of type 'undefined' + is not assignable to parameter of type 'number'. +probe-2-set-current-popup-id.ts(29,21): error TS2345: Argument of type + 'number | undefined' is not assignable to parameter of type 'number'. +probe-3-ens-provider.ts(27,32): error TS18048: 'provider' is possibly 'undefined'. +probe-3-ens-provider.ts(44,14): error TS2322: Type + 'SwappableProxy> | undefined' is not + assignable to type 'HandWrittenEthProvider'. +probe-4-offscreen-message.ts(43,14): error TS2322: Types of property 'target' are + incompatible. Type 'string' is not assignable to type 'OffscreenCommunicationTarget'. +probe-6-chain-id-widening.ts(31,50): error TS2322: Type '"1"' is not assignable to + type '`0x${string}`'. +``` + +A seventh probe compiled the PR's *original* string literals with **zero** errors — +which is how the two unnecessary runtime changes below were established. + +## Findings + +| Hand-written | Authoritative source | Shape | +|---|---|---| +| `_setCurrentPopupId: ((id: number \| undefined) => void)` | `AppStateController['setCurrentPopupId']` → `(id: number) => void` | widening | +| `getCurrentChainId: () => string` | `ReturnType` → `` `0x${string}` `` | widening | +| `target: string` on a received message | the sender, TypeScript in-repo → `OffscreenCommunicationTarget` | widening | +| `EthProvider` (written twice, unshared) | `ReturnType['provider']` | duplication + dropped nullability | +| `obj: Record` | already in a JSDoc `@type` on the argument at the call site: `MetaMaskStorageStructure \| undefined` | placeholder + dropped nullability | + +Two of these had a consequence beyond tidiness: + +- The widened setter is what let `setter?.(undefined)` compile. Deriving it fails — + usefully, because it surfaces a genuine mismatch between a controller method's + declared parameter and how its callers actually use it. +- The dropped nullability sat in the same edit that deleted a `= {}` default + parameter, i.e. the guard that existed *for* the nullable case. (Traced: inert + today, because a fallback upstream guarantees an object by the time the path runs.) + +**Separately**, reading `@types/chrome` before trusting a type error found two +runtime changes the types never required: `contextTypes` and `reasons` are declared +as template-literal **string** types (`` `${ContextType}`[] ``, `` `${Reason}`[] ``), +so the original `['OFFSCREEN_DOCUMENT']` / `['IFRAME_SCRIPTING']` already compiled. +The PR replaced both with runtime enum lookups, and added a redundant cast. + +## Clearances — and one that mattered + +Five claims the probes **cleared**: + +- `Promise` as a provider `request` return looked unsound. It isn't: the + real `request` is generic in its result, so a call + site may legitimately fix `Result = string`. +- Two `declare module` blocks: neither package ships types, and no `@types/*` is + installed → no authoritative source exists, so hand-writing is correct. +- A hand-declared `clients?: { matchAll }`: the authoritative `Clients` lives in + `lib.webworker.d.ts`, which is not in this repo's `tsconfig.lib` → out of scope. +- Two dependency callbacks (`() => string`, `() => boolean`) matched their sources + exactly. +- A dropped argument (`_getPopup(id)` → `_getPopup()`) was a genuine no-op: the + base function declared no parameters, so the argument was already discarded. + +**The provider clearance is the reason Step 5 exists.** The first Arm B run +reported `TS18048: 'provider' is possibly 'undefined'` on that probe — an error on +the line the return-type claim lived on, which reads as confirmation if you count +exit codes. The nullability error fired one property *ahead* of the claim. Setting +it aside with `NonNullable<…>` and re-running showed the return type compiles +clean. Reported as a finding, it would have been wrong. diff --git a/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh new file mode 100755 index 00000000..32d76bd1 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# Two-arm type proof: does a hand-written type agree with the authoritative one? +# +# substitution-ab.sh [probe-dest] +# +# repo checked out at the PR head, deps installed +# directory of probe-*.ts files (see skill.md Step 3) +# [probe-dest] where to stage them, relative to ; must sit inside +# the tsconfig `include` paths. Default: src/__type-probe__ +# +# Arm A must be silent. If it is not, stop — nothing in Arm B is attributable. +set -uo pipefail + +REPO=${1:?usage: substitution-ab.sh [probe-dest]} +PROBES=${2:?usage: substitution-ab.sh [probe-dest]} +DEST=${3:-src/__type-probe__} + +: "${NODE_OPTIONS:=--max-old-space-size=9216}" +export NODE_OPTIONS + +cd "$REPO" || exit 1 +[ -d "$PROBES" ] || { echo "no such probe dir: $PROBES" >&2; exit 1; } + +cleanup() { rm -rf "$REPO/$DEST"; } +trap cleanup EXIT INT TERM + +echo "=== Arm A — PR head as written (must be silent) ===" +A=$(npx tsc -p tsconfig.json --noEmit 2>&1) +A_STATUS=$? +if [ -n "$A" ]; then + echo "$A" + echo + echo "!! Arm A is NOT silent (exit $A_STATUS). The comparison is INCONCLUSIVE:" + echo "!! Arm B's diagnostics cannot be attributed to the substitution." + echo "!! Fix the baseline (toolchain, lockfile, heap, project scope) before reading Arm B." + exit 2 +fi +echo "0 diagnostics — baseline clean, Arm B is attributable." +echo + +echo "=== Arm B — same commit, derived types substituted ===" +mkdir -p "$DEST" +cp "$PROBES"/probe-*.ts "$DEST"/ 2>/dev/null || { + echo "no probe-*.ts found in $PROBES" >&2; exit 1; } + +npx tsc -p tsconfig.json --noEmit 2>&1 +echo +echo "=== Each diagnostic above is a disagreement the hand-written type concealed. ===" +echo "=== Before believing any of them: confirm the diagnostic is the one the ===" +echo "=== claim needs, not an earlier cause short-circuiting it (skill.md Step 5).===" diff --git a/domains/typescript/skills/typescript-compiler-blindspots/skill.md b/domains/typescript/skills/typescript-compiler-blindspots/skill.md new file mode 100644 index 00000000..90c1bc4f --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/skill.md @@ -0,0 +1,272 @@ +--- +name: typescript-compiler-blindspots +description: >- + Find the type defects `tsc` is structurally unable to report — a green build is + not evidence the types are correct. Covers the two classes: (1) hand-written + types that restate an authoritative source and disagree with it, caught by + substituting the derived type at a fixed commit and diffing `tsc` output; and + (2) the standing blind spots in the language and config — unchecked array/record + indexing, bivariant method parameters, covariant arrays, `any` absorption at + untyped boundaries, ambient `declare module` assertions, excess-property checks + that only fire on fresh literals, and external data asserted rather than + validated. Also audits typing edits that quietly change runtime behavior: + stripped `| undefined`, deleted default parameters, literals swapped for runtime + enum lookups, calls made optional so a throw becomes a silent no-op. Use when + reviewing a JS→TS migration, a PR that hand-writes types for values that already + have them, a "rename-only" refactor, or any PR claiming a change is mechanical. + Trigger phrases include "validate this TypeScript migration", "is this type + right", "does this type match the real shape", "why didn't CI catch this type", + "derive vs define", and "what can tsc not check". +maturity: experimental +--- + +# TypeScript compiler blind spots + +A hand-written type is a **claim about a value's shape**, and it compiles whether +or not the claim is true. `tsc` checks declarations for internal **consistency** — +never for **correspondence** to the source they restate. Across a JavaScript +boundary (`checkJs` off) it checks nothing at all. + +Those are the blind spots. This skill finds what is hiding in them. + +Two classes, two methods: + +| Class | What it is | Method | +|---|---|---| +| **Restated types** | A type hand-written to describe a value that already has an authoritative type | **Substitution A/B** — swap in the derived type, diff `tsc` output | +| **Standing blind spots** | Defects the language and config cannot report at all, in any codebase | **Targeted audit** — [references/false-negatives.md](references/false-negatives.md) | + +The second class is the one that surprises people: a file of genuinely broken +code can typecheck clean. The reference includes exactly that — a demonstration +file where every block is wrong and `tsc` reports zero errors. + +Companion to the authoring rule it enforces — *derive types from authoritative +sources instead of re-declaring them* in +[contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md). +This skill is the review-side proof; that document is the write-side guidance. + +## When to use + +- A **JS→TS migration** PR, or one whose body says *mechanical*, *rename-only*, or *no behavior change*. +- A PR that **hand-writes a type for a value that already has one** — a controller method's parameters, a selector's return, a message payload, a package's exported shape. +- A reviewer asks "is this type actually right?" and the answer so far is "it compiles." + +Out of scope: code correctness (use a normal review), runtime behavior (use e2e / +visual proof), and lint/format (CI owns those). + +## Prerequisites + +- The repo checked out at the PR head, dependencies installed, `tsc` runnable. +- Enough heap for a full typecheck on large repos (see Troubleshooting). +- A scratch directory inside the `tsconfig` `include` paths for probe files. + +## The core idea: two arms, one commit + +Both arms sit at the **same commit**. They differ by a *substitution*, not by a +ref — so there is no build, no rebase, and no merge boundary to confound. + +| | What it is | What it must show | +|---|---|---| +| **Arm A** | The PR exactly as written | **Silent.** Zero diagnostics | +| **Arm B** | Same tree + probes that use the *derived* type, exercised as the real code exercises it | Each new diagnostic = a disagreement the hand-written type concealed | + +**Arm A must be silent or the run is inconclusive.** If the untouched tree +already emits diagnostics, nothing in Arm B is attributable to the substitution — +"N errors in Arm B" is then a count, not a finding. Publish Arm A's result +verbatim as the delivery check. + +## Instructions + +### Step 1: Inventory every type the PR hand-wrote + +```bash +gh pr diff | grep -nE '^\+.*(type [A-Z]|interface [A-Z]|: (Record<|string|number|boolean|unknown|any)\b)' +``` + +List them. Each one is a claim you are about to test. + +### Step 2: Find the authoritative source for each + +Work down this list — the first hit wins. In the worked example 9 of the 12 +hand-written types had a source, each found in under a minute: + +1. **The call site.** What is actually passed? In a JS caller, check for a JSDoc + `@type {import('…').Foo}` annotation on the variable — the answer is sometimes + literally already written down there. +2. **The class or method being wrapped** → `MyController['someMethod']`. +3. **A package already imported in the same file** — e.g. `webextension-polyfill` + defines every listener payload; if the file calls the API, the type is in reach. +4. **The sender**, for a message or event payload. If the sender is TypeScript, the + shape is derivable, not guessable. +5. **A selector's return** → `ReturnType`. +6. **`@types/*` for a platform API.** Read the actual declaration before "fixing" + a type error — many are template-literal *string* types (`` `${SomeEnum}`[] ``), + which already accept a plain string literal. +7. **No source exists** — an untyped dependency, a lib absent from `tsconfig.lib`, + a genuinely new boundary the repo owns. Hand-writing is then **correct**. Record + it as a cleared falsifier with the reason; do not report it as a finding. + +### Step 3: Write one probe per claim + +One file per claim, in a scratch dir inside the `include` paths. Each probe names +its authoritative source in a header comment and calls the derived type **the way +the real call site calls it**: + +```ts +// PROBE — src/thing.ts hand-wrote `setFoo: (id: number | undefined) => void`. +// Authoritative: FooController['setFoo'] (foo-controller.ts:120) — param is `number`. +// The real code calls it as below. +import type { FooController } from '../controllers/foo-controller'; + +declare const setFoo: FooController['setFoo']; + +export function asCalledByTheRealCode() { + setFoo(undefined); // thing.ts:105 +} +``` + +### Step 4: Run both arms + +```bash +./scripts/substitution-ab.sh +``` + +Or by hand — Arm A first, and stop if it is not silent. + +### Step 5: Isolate diagnostics that fire for the wrong reason + +A checker reports the *first* failure it reaches, so an unrelated earlier cause +can short-circuit the claim under test — and an exit-code read scores that as a +confirmation. **Assert on the specific diagnostic** (code + message + line), and +where an earlier cause intervenes, neutralise it and re-probe: + +```ts +const defined = value as NonNullable; // set nullability aside +// …now the return-type claim is the only thing left to fail +``` + +This is not hypothetical — see the worked example, where a claim that a return +type was unsound turned out **sound** once the nullability error ahead of it was +isolated. + +### Step 6: Report findings *and* clearances + +Give each claim a verdict, and say which ones the probes **cleared**. A +substitution sweep that only ever confirms is indistinguishable from one that +never isolated anything. + +## The four divergence shapes + +Four shapes to check for. All five divergences in the worked example were one of +these, which is a small sample — treat the list as a starting checklist, not a +partition: + +1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, + `number | undefined` for `number`. Admits values the real type rejects; worst + when a guard downstream depends on the narrower form. +2. **Dropped nullability** — the source says `| undefined`, the hand-written type + doesn't. Erases the compiler's record of why a runtime guard exists. +3. **Duplication** — the same shape written out in two files, unshared. Both copies + now need every future change. +4. **Placeholder** — `Record`, `any`, or `unknown` standing in for + a shape that is known. Pushes a cast to every use site. + +## Escape hatches are the tell + +When a diff adds a hand-written type *and* an `as`, a `!`, a new `?.`, or an +`eslint-disable` in the same region, check whether the escape hatch exists to service +the type rather than the runtime. Count them — a cluster marks where to probe first. + +## A typing change should not change runtime behavior + +The second axis, and the one whose defects reach runtime rather than staying in +the type layer. A migration PR is +allowed to add annotations; it is not allowed to change what the program *does*. +Four patterns to grep the diff for, all of which look like typing work: + +1. **A literal replaced by a runtime lookup.** `['IFRAME_SCRIPTING']` becoming + `[SomeApi.Reason.IFRAME_SCRIPTING]` adds a dependency on that object existing at + runtime. **Read the declaration first** — if the parameter is a template-literal + string type (`` `${SomeEnum}`[] ``), the literal already type-checked and the + swap bought nothing. +2. **A default parameter or fallback deleted.** `function f(x = {})` → `function f(x: T)` + removes a guard. Ask what the guard was *for*: a `| undefined` the new type just + dropped is the first candidate. Then check reachability rather than assuming either way. +3. **A call made optional.** `obj.method()` → `obj.method?.()`, added to satisfy a + hand-written `| undefined`, converts a **throw into a silent no-op**. The loud + failure was load-bearing; now the same state produces no signal at all. +4. **A widened local to keep a check alive.** `let name: string | undefined` on a + value the authoritative type calls `string`, so that an `=== undefined` branch + still compiles. If the runtime check is genuinely needed, the *input* type is + wrong — fix that instead of widening downstream. + +For each hit: state whether it is reachable, and say so plainly either way. "I +traced it and it is inert today" is a useful review finding. "This might be a bug" +is not. + +### Silent failure modes deserve their own pass + +Ask where a newly-introduced failure would *surface*. A change inside a +`try { … } catch { captureException(e); return; }` degrades a feature without +crashing — nothing goes red, no test fails, and the only signal is an error-tracker +entry nobody is watching. The same edit in a hot path would be caught in minutes. +Weight findings by observability, not just by likelihood: **an unlikely failure in a +swallowed path can outrank a likely one in a loud path.** + +## Why the build stays green regardless + +- The hand-written type **compiles by construction** — that is why it was written. +- With `checkJs` off, a type written for a function whose callers are still `.js` + is checked against **nothing** and can drift indefinitely. +- A value that arrives as `any` silently satisfies any annotation. + +So cite the green build as the *premise* of the finding, never as counter-evidence. + +## Examples + +**Worked example** — 12 hand-written types across a JS→TS migration PR, 5 confirmed +divergences and 5 cleared falsifiers, with the verbatim two-arm output: +[references/worked-example.md](references/worked-example.md). + +**Repo notes** for MetaMask Extension (heap, probe location, `checkJs` status): +[references/metamask-extension.md](references/metamask-extension.md). + +``` +User: "Validate this TS migration PR — is it really mechanical?" +Agent: inventories the 12 new types → finds the authoritative source for 9 → + writes 6 probes → Arm A silent, Arm B reports 6 diagnostics → isolates + one that fired for the wrong reason → reports 5 findings, 5 clearances. +``` + +## Troubleshooting + +### Arm A is not silent + +**Problem:** the untouched tree already emits diagnostics, so Arm B is unattributable. +**Fix:** pin the toolchain, install against the PR's own lockfile, raise the heap, or +narrow the project. If it cannot be made silent, the lane is **inconclusive** — say +so; do not report Arm B's count as findings. + +### `tsc` runs out of memory + +**Problem:** `FATAL ERROR: Ineffective mark-compacts near heap limit`. +**Fix:** raise the heap — `NODE_OPTIONS='--max-old-space-size=9216'`. Note the OOM +exits non-zero *without* type diagnostics, so a naive exit-code check reads it as +"errors found." Always look at the output, not just the status. + +### A probe errors, but not for the claimed reason + +**Problem:** the diagnostic is about an earlier property, not the claim. +**Fix:** neutralise the earlier cause (`NonNullable<…>`, a narrow assertion) and +re-run. If the claim then compiles clean, the claim was **wrong** — report it as cleared. + +### There is no authoritative source + +Not a failure. Hand-writing is correct where nothing defines the shape; record the +reason (package ships no types, lib not in `tsconfig.lib`, new boundary) so the next +reviewer doesn't re-litigate it. + +## Related + +- [contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md) — the write-side rule this proves. +- `unit-testing`, `integration-test` — for behavior claims; this skill proves *types*. From ab926b82bfbe0ca4abe2cb29ea1d981169ef32f7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 08:32:19 -0400 Subject: [PATCH 10/20] Rename `typescript-compiler-blindspots` to `compiler-blindspots` The skill lives in the `typescript` domain, so the prefix repeated information already carried by the path and by every discovery surface that shows it. Installed as `mms-compiler-blindspots`. --- .../references/false-negatives.md | 0 .../references/metamask-extension.md | 0 .../references/worked-example.md | 0 .../scripts/substitution-ab.sh | 0 .../skill.md | 2 +- 5 files changed, 1 insertion(+), 1 deletion(-) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/false-negatives.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/metamask-extension.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/worked-example.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/scripts/substitution-ab.sh (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/skill.md (99%) diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/compiler-blindspots/references/false-negatives.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md rename to domains/typescript/skills/compiler-blindspots/references/false-negatives.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/compiler-blindspots/references/metamask-extension.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md rename to domains/typescript/skills/compiler-blindspots/references/metamask-extension.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md b/domains/typescript/skills/compiler-blindspots/references/worked-example.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md rename to domains/typescript/skills/compiler-blindspots/references/worked-example.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh rename to domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh diff --git a/domains/typescript/skills/typescript-compiler-blindspots/skill.md b/domains/typescript/skills/compiler-blindspots/skill.md similarity index 99% rename from domains/typescript/skills/typescript-compiler-blindspots/skill.md rename to domains/typescript/skills/compiler-blindspots/skill.md index 90c1bc4f..0cfaa6c3 100644 --- a/domains/typescript/skills/typescript-compiler-blindspots/skill.md +++ b/domains/typescript/skills/compiler-blindspots/skill.md @@ -1,5 +1,5 @@ --- -name: typescript-compiler-blindspots +name: compiler-blindspots description: >- Find the type defects `tsc` is structurally unable to report — a green build is not evidence the types are correct. Covers the two classes: (1) hand-written From e170b394c277906fb5ba3b17e62d60eca7542fee Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 10:44:06 -0400 Subject: [PATCH 11/20] Rename `compiler-blindspots` to `tsc-blindspots` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills install flat as `mms-`, so `domains/typescript/` does not disambiguate the name at the callsite — and "compiler" reads as React Compiler in a repo where that is a live subject. The skill is about `tsc` specifically: its own first clause is "the type defects `tsc` is structurally unable to report". Also adds the slash trigger to the description, which listed only prose phrases. --- .../references/false-negatives.md | 0 .../references/metamask-extension.md | 0 .../references/worked-example.md | 0 .../scripts/substitution-ab.sh | 0 .../{compiler-blindspots => tsc-blindspots}/skill.md | 9 +++++---- 5 files changed, 5 insertions(+), 4 deletions(-) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/false-negatives.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/metamask-extension.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/worked-example.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/scripts/substitution-ab.sh (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/skill.md (98%) diff --git a/domains/typescript/skills/compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/false-negatives.md rename to domains/typescript/skills/tsc-blindspots/references/false-negatives.md diff --git a/domains/typescript/skills/compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/metamask-extension.md rename to domains/typescript/skills/tsc-blindspots/references/metamask-extension.md diff --git a/domains/typescript/skills/compiler-blindspots/references/worked-example.md b/domains/typescript/skills/tsc-blindspots/references/worked-example.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/worked-example.md rename to domains/typescript/skills/tsc-blindspots/references/worked-example.md diff --git a/domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh similarity index 100% rename from domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh rename to domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh diff --git a/domains/typescript/skills/compiler-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md similarity index 98% rename from domains/typescript/skills/compiler-blindspots/skill.md rename to domains/typescript/skills/tsc-blindspots/skill.md index 0cfaa6c3..495142b4 100644 --- a/domains/typescript/skills/compiler-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -1,5 +1,5 @@ --- -name: compiler-blindspots +name: tsc-blindspots description: >- Find the type defects `tsc` is structurally unable to report — a green build is not evidence the types are correct. Covers the two classes: (1) hand-written @@ -14,9 +14,10 @@ description: >- enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Trigger phrases include "validate this TypeScript migration", "is this type - right", "does this type match the real shape", "why didn't CI catch this type", - "derive vs define", and "what can tsc not check". + Triggers on /tsc-blindspots, or on phrases like "validate this TypeScript + migration", "is this type right", "does this type match the real shape", + "why didn't CI catch this type", "derive vs define", and "what can tsc not + check". maturity: experimental --- From d50d94fb71013440e22937836ce63c63eee9e7b0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 3 Aug 2026 12:18:50 -0400 Subject: [PATCH 12/20] Add exit-tracing, a runtime declaration check, and a dead-module check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four additions, each of which found something the existing sections did not point at, while reviewing a JS->TS conversion. `any` absorption gains its complement: the existing bullet traces where a confidently-typed value entered, which finds nothing when the value is a module's own return. Tracing where an `any` *exits* — assigning the return to two impossible types with a known-typed sibling as control — found `any` escaping a resolver into its caller. The control line is load-bearing: without it a silent probe is indistinguishable from one that cannot fail, and running outside the project tsconfig produces errors that are the harness rather than the finding. Ethers `Contract` dynamic methods are named as a source, since the ABI is runtime data and the call reads as an ordinary typed await. It is already tracked at #31973, where one consumer declares `Promise` with a disable comment and another lets it infer; the second is the dangerous form. Ambient `declare module` verification moves from reading package source to requiring the package and checking exports, return `typeof`, and whether a default export is legitimate under esModuleInterop. Inventory now begins by checking the module is referenced at all — a dead module's types are unfalsifiable, and its conversion is a deletion candidate rather than a typing exercise. --- .../references/false-negatives.md | 46 +++++++++++++++++-- .../typescript/skills/tsc-blindspots/skill.md | 12 +++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md index 5bcf97b8..fc494022 100644 --- a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -115,10 +115,38 @@ meta.version.toFixed(2); // may be a string at runtime ``` An `any` satisfies every annotation silently. Sources: untyped dependencies, -`JSON.parse`, generics that default to `any`, and `as any`. +`JSON.parse`, generics that default to `any`, `as any` — and the one that hides +best in a typed-looking file: **dynamic methods on an ethers `Contract`**, where +the ABI is runtime data so every call returns `any` while reading as an ordinary +typed `await`. This is a known source in `metamask-extension` +([#31973](https://github.com/MetaMask/metamask-extension/issues/31973)): +`shared/lib/token-util.ts` declares it as `Promise` with a disable comment, +which is the honest form. The dangerous form is letting it infer — nothing +annotates it, so it propagates into the module's return type unflagged. - **In review:** trace where a confidently-typed value *entered* the program. If it entered as `any`, its type is a wish. +- **Also trace where an `any` *exits*.** The bullet above looks backwards from a + suspicious value; this looks forward from a module's public surface. Assign its + return to two impossible types, with a known-typed sibling as a control: + + ```ts + const { type, hash } = await resolveEnsToIpfsContentId(args); + const a: number = hash; // compiles ⇒ `hash` is any + const c: symbol = hash.whateverIWant.deeply; // compiles ⇒ ditto + const d: number = type; // MUST error ⇒ probe can discriminate + ``` + + If the nonsense compiles and the control errors, `any` is escaping into every + caller. **The control line is not optional** — without it, a probe that reports + nothing is indistinguishable from a probe that cannot fail. Run it under the + project's `tsconfig`, not a standalone `tsc` invocation, or missing ambient + declarations and `resolveJsonModule` will produce errors that are the harness + rather than the finding. +- **Precise signatures around an `any` source make it worse, not better.** A file + with a derived provider type and `hexValueIsEmpty(value: string | null | undefined)` + reads as a checked boundary while every value crossing it is unchecked. A + migration that adds those signatures is what creates the appearance. ### 8. Ambient `declare module` is an unverified assertion @@ -134,8 +162,20 @@ them to the package. Getting a return type wrong here is invisible forever, and the declaration is **global**, so it also shadows any real types the package later ships. -- **In review:** read the package's actual source at the installed version when a - `declare module` is added or changed. Prefer `@types/*` or a PR upstream. +- **In review:** verify the declaration against the installed package at runtime — + faster and more decisive than reading source: + + ```bash + node -e 'const m = require("@ensdomains/content-hash"); + console.log(Object.keys(m), "default:", typeof m.default); + console.log(typeof m.decode(m.encode("ipfs-ns", "Qm…")));' + ``` + + Check three things: **the exports exist**, **each declared signature's return + `typeof` matches**, and **whether `export default` is legitimate** — a CJS module + with `typeof m.default === 'undefined'` still warrants a default declaration *if* + `esModuleInterop` is on, and is a defect if it is not. Prefer `@types/*` or an + upstream PR over hand-writing. ### 9. External data is asserted, not validated diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 495142b4..8cdab2cc 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -86,6 +86,18 @@ gh pr diff | grep -nE '^\+.*(type [A-Z]|interface [A-Z]|: (Record<|string|n List them. Each one is a claim you are about to test. +**First, check each converted module is still referenced:** + +```bash +grep -rn "moduleName" --include=*.ts --include=*.js . | grep -v node_modules | grep -v '\.test\.' +``` + +If the only hits are the module's own definition and its test, the file is dead — +every type on it is unfalsifiable, because nothing constrains it and no divergence +can ever surface. This is the highest value-per-second check in a migration, and +it reorders the work: a dead module's conversion is a deletion candidate, not a +typing exercise. + ### Step 2: Find the authoritative source for each Work down this list — the first hit wins. In the worked example 9 of the 12 From a1ea24a681952793ec683115e5417a1f94fda3fa Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 05:34:03 -0400 Subject: [PATCH 13/20] Add false precision as a divergence shape and the `IsAny` probe that finds it A precise annotation fed `any` at every call site is reportable by neither `tsc` nor `no-explicit-any`, and an ambient `declare module` in the path re-mints the `any` as a confident `string`. Both arms of the probe verified against `metamask-extension`. Also fixes `avoid-any`'s frontmatter, which did not parse as YAML. --- domains/typescript/skills/avoid-any/skill.md | 22 +++++++- .../references/false-negatives.md | 52 ++++++++++++++++++- .../typescript/skills/tsc-blindspots/skill.md | 47 ++++++++++++++--- 3 files changed, 112 insertions(+), 9 deletions(-) diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index b9256e87..0fb55dcf 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -1,6 +1,13 @@ --- name: avoid-any -description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`). Two narrow exceptions: a generic constraint, and a callback parameter caught in a bivariant position between two fixed, irresolvable function-type constraints — both declared with an inline eslint-disable. +description: >- + Handle `any` correctly — it is not a type but a directive that disables type + checking. Substitute by position (assignee → `unknown`, assigned → `never`). + Two narrow exceptions: a generic constraint, and a callback parameter caught + in a bivariant position between two fixed, irresolvable function-type + constraints — both declared with an inline eslint-disable. Also covers the + `any` that is never written down: a precise signature fed `any` at every call + site, which no lint rule and no compiler check can report. maturity: experimental --- @@ -72,3 +79,16 @@ Like the generic-constraint case, this `any` is **not infectious** — it is sco Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." + +## Declared `any` beats absorbed `any` — and only one of them is countable + +The two exceptions above are both *declared*: the `any` is written down, an inline disable sits next to it, and a reviewer can find it with `grep`. The dangerous case is the one where **no `any` appears anywhere** and the value is `any` regardless: + +- 🚫 **Absorbed.** A precise annotation on a parameter or return that receives `any` at every call site — `hexValueIsEmpty(value: string | null | undefined)` fed ethers dynamic-method results. `no-explicit-any` never fires, because no `any` was written. `tsc` never fires, because `any` satisfies every annotation. The file reads as checked and none of it is. +- ✅ **Declared.** `): Promise` with an inline disable and a linked issue, as `shared/lib/token-util.ts` does for the same ethers API. Nothing is safer at runtime, but the claim is now honest, greppable, and countable by CI. + +The absorbed form is strictly worse than the declared one, and it is what a JS→TS conversion produces by default: the writer annotates what the value *ought* to be, and `any` accepts the annotation without comment. + +**A conversion is where the boundary's type is chosen, so an absorbed `any` is a decision, not an inheritance.** With `checkJs` off the predecessor asserted nothing; the precise signature is new. "The `any` is pre-existing" is true of the library and false of the annotation next to it. + +Detect it with `IsAny` at the call sites rather than by reading the signature — the probe, its controls, and the `declare module` composition that turns an `any` into a confident `string` are in the `tsc-blindspots` skill. The fix is to type or validate the value where it enters, so the precise annotations downstream are earned. diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md index fc494022..ea093572 100644 --- a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -147,6 +147,28 @@ annotates it, so it propagates into the module's return type unflagged. with a derived provider type and `hexValueIsEmpty(value: string | null | undefined)` reads as a checked boundary while every value crossing it is unchecked. A migration that adds those signatures is what creates the appearance. +- **To ask "is this specific value `any`", use `IsAny` rather than a nonsense + assignment.** The exit probe above traces a module's public surface; this answers + the question at any single site, and its verdict is a compile error rather than an + absence of one: + + ```ts + type IsAny = 0 extends 1 & T ? true : false; + + const resolverAddress = await registryContract.resolver(hash); + const a1: IsAny = true; // silent ⇒ it IS any + const known = 'x' as string; + const a3: IsAny = true; // MUST error ⇒ probe discriminates + ``` + + `0 extends 1 & T` holds only for `any`, because `1 & any` is `any` and every type + extends `any`. **Read the polarity carefully: silence is the finding here**, which + is the reverse of every other probe in this file — so the known-`string` control + is what separates "this value is `any`" from "the probe never ran." + + Verified against `metamask-extension` with the repo's own `tsconfig.json`: four + arms — an ethers dynamic method and an explicit `as any` both silent, a known + `string` and a laundered `string` (below) both `TS2322`. ### 8. Ambient `declare module` is an unverified assertion @@ -177,6 +199,32 @@ later ships. `esModuleInterop` is on, and is a defect if it is not. Prefer `@types/*` or an upstream PR over hand-writing. +#### 7 + 8 compose: a declaration launders `any` into a confident type + +The two blind spots above are usually audited apart, and the defect lives in their +composition. §7 says an `any` propagates; §8 says a hand-written declaration is +believed. Put them in sequence and the propagation **stops** — replaced by a type +nobody checked, which every reader downstream then trusts: + +| hop | site | resulting type | who asserted it | +|---|---|---|---| +| 1 | `await resolverContract.contenthash(hash)` | `any` | ethers `readonly [key: string]: ContractFunction \| any` | +| 2 | `contentHash.getCodec(rawContentHash)` | **`string`** | a hand-written `declare module` | +| 3 | `return { type, hash: decoded }` | `{ type: string; hash: any }` | inferred from hop 2 | +| 4 | `` `https://${hash}.${type.slice(0, 4)}.${gateway}` `` | `.slice` on a `string` | inferred from hop 3 | + +Hop 2 is declared `(contentHash: string) => string` and **called with an `any`** — +so it neither rejects its input nor earns its output. By hop 4 the value is a URL +segment, and the only claim it was ever a string is a line a human wrote. + +- **Audit rule:** for every `declare module`, list its call sites and run `IsAny` on + each **argument**. A parameter declared `string` and passed `any` is the laundering + point, and it is where the annotation or the validator belongs — not at hop 4, + where the value already looks trustworthy. +- Verified in the demonstration run above: hop 1 silent under `IsAny` (it is `any`), + hop 2 `TS2322` (it is `string`), with the declaration block supplied locally so the + only variable between the two arms is the `declare module`. + ### 9. External data is asserted, not validated ```ts @@ -293,7 +341,9 @@ Don't run all ten as a checklist. Pick by what the diff touches: - **New indexing / destructuring** → 1, 2 - **New message, event, or callback types** → 3, 5, 6 -- **New `declare module`, new dependency, `@types` change** → 8 +- **New `declare module`, new dependency, `@types` change** → 8, then **7 + 8** on + its call sites — a declaration is audited against the package by default and + against its *arguments* almost never - **Anything reading persisted state, storage, or an RPC response** → 7, 9 - **A JS→TS conversion** → 10, plus the restated-type class in the main skill - **Any PR whose safety argument is "CI is green"** → section C, first diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 8cdab2cc..e8006f05 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -7,9 +7,11 @@ description: >- substituting the derived type at a fixed commit and diffing `tsc` output; and (2) the standing blind spots in the language and config — unchecked array/record indexing, bivariant method parameters, covariant arrays, `any` absorption at - untyped boundaries, ambient `declare module` assertions, excess-property checks - that only fire on fresh literals, and external data asserted rather than - validated. Also audits typing edits that quietly change runtime behavior: + untyped boundaries, precise signatures fed `any` at every call site, ambient + `declare module` assertions that launder an `any` into a confident type, + excess-property checks that only fire on fresh literals, and external data + asserted rather than validated. Also audits typing edits that quietly change + runtime behavior: stripped `| undefined`, deleted default parameters, literals swapped for runtime enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already @@ -168,11 +170,34 @@ Give each claim a verdict, and say which ones the probes **cleared**. A substitution sweep that only ever confirms is indistinguishable from one that never isolated anything. -## The four divergence shapes +### Severity: "pre-existing" is about the upstream `any`, not about the annotation -Four shapes to check for. All five divergences in the worked example were one of -these, which is a small sample — treat the list as a starting checklist, not a -partition: +A conversion PR invites two reflexes that both understate a false-precision +finding, and the underlying `any` is what makes each of them sound reasonable: + +- **"It's pre-existing."** Split the claim. The library's `any` is genuinely + inherited — ethers has returned `any` from dynamic contract methods since long + before this diff. The *annotation beside it* was written here: check `git log + --diff-filter=A -- ` and, for a `declare module`, whether the block itself + is added by this PR. If the file is new, nothing in it is pre-existing, because + with `checkJs` off the predecessor asserted nothing at all. A conversion is the + moment the boundary's type is **chosen**. +- **"It's a nit."** A nit is a finding that is minor *in itself*. A value crossing + a boundary unchecked, under a signature that says it was checked, is a type-safety + defect — the class this whole skill exists to surface. Scope and severity are + separate axes: a substantive finding a PR need not fix is still substantive, and + saying so costs one sentence. + +The remedy follows from which one it is. Annotating the hole and filing a TODO is +right for the inherited `any`; it is not a fix for false precision, because the +misleading signature stays exactly as it was. Type or validate the value where it +enters, so the precise annotations downstream are earned. + +## The five divergence shapes + +Five shapes to check for. Treat the list as a starting checklist, not a partition — +the first four each account for at least one divergence in the worked example, and +the fifth was found on a later pass over the same PR: 1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, `number | undefined` for `number`. Admits values the real type rejects; worst @@ -183,6 +208,14 @@ partition: now need every future change. 4. **Placeholder** — `Record`, `any`, or `unknown` standing in for a shape that is known. Pushes a cast to every use site. +5. **False precision** — the inverse of a placeholder: the annotation is *narrower* + than what actually arrives. `hexValueIsEmpty(value: string | null | undefined)` + on a parameter fed `any` at every call site. `tsc` cannot report it, because + `any` satisfies every annotation, and `no-explicit-any` cannot either, because + no `any` was written. The narrower the type, the more confident the file reads + and the less any of it is checked. Find it with `IsAny` at the call sites, not + by reading the signature — see + [references/false-negatives.md](references/false-negatives.md) §7. ## Escape hatches are the tell From 995f3c83f3ab5008169c31a3e18db97725e0f3e0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 08:01:36 -0400 Subject: [PATCH 14/20] Name the installed command in `tsc-blindspots`'s description The installer emits `mms-tsc-blindspots`; the description advertised `/tsc-blindspots`. --- domains/typescript/skills/tsc-blindspots/skill.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index e8006f05..a62730d9 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -16,7 +16,7 @@ description: >- enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Triggers on /tsc-blindspots, or on phrases like "validate this TypeScript + Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript migration", "is this type right", "does this type match the real shape", "why didn't CI catch this type", "derive vs define", and "what can tsc not check". From 4f1ee6c996a6b67a2eef51d997ddedf92797b5cd Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 31 Aug 2026 08:54:12 -0400 Subject: [PATCH 15/20] Add per-repo overlays for the three base-candidate typescript skills `tsc-blindspots`, `derive-types` and `avoid-any` shipped without a `repos/` directory, so they installed into every repo by default rather than by decision, carrying content written against no repo in particular. A blind spot only exists where the setting that would catch it is off, so the overlays genuinely differ. Extension and mobile set `allowJs`, making the JS boundary a live unchecked surface; core does not, and its overlay says so rather than omitting the class. Core instead has `composite: true`, where a type can go stale across a project reference. Extension's `strict` is inherited from `@tsconfig/node22` rather than local, which is itself a blind spot for anyone auditing tsconfig.json. `@metamask/eslint-config-typescript` sets `no-explicit-any` and all five `no-unsafe-*` rules to 'off' in every resolved major (13.0.0 mobile, 14.1.1 extension, 15.0.0 core), so each repo's local 'error' is a deliberate re-enable rather than a default. Extension's lint block carrying `no-explicit-any` takes its file list from the parsed tsconfig program, so the ~300 `*.stories.ts(x)` excluded from `tsconfig.json` lose the rule and `tsc` together. Every line citation verified against `origin/main` rather than a working tree; none resolves to a blank line. Installs verified into all three repos with the correct overlay merged. --- .../typescript/skills/avoid-any/repos/core.md | 60 ++++++ .../avoid-any/repos/metamask-extension.md | 63 ++++++ .../skills/avoid-any/repos/metamask-mobile.md | 67 ++++++ .../skills/derive-types/repos/core.md | 102 +++++++++ .../derive-types/repos/metamask-extension.md | 106 ++++++++++ .../derive-types/repos/metamask-mobile.md | 125 +++++++++++ .../skills/tsc-blindspots/repos/core.md | 199 ++++++++++++++++++ .../repos/metamask-extension.md | 134 ++++++++++++ .../tsc-blindspots/repos/metamask-mobile.md | 140 ++++++++++++ 9 files changed, 996 insertions(+) create mode 100644 domains/typescript/skills/avoid-any/repos/core.md create mode 100644 domains/typescript/skills/avoid-any/repos/metamask-extension.md create mode 100644 domains/typescript/skills/avoid-any/repos/metamask-mobile.md create mode 100644 domains/typescript/skills/derive-types/repos/core.md create mode 100644 domains/typescript/skills/derive-types/repos/metamask-extension.md create mode 100644 domains/typescript/skills/derive-types/repos/metamask-mobile.md create mode 100644 domains/typescript/skills/tsc-blindspots/repos/core.md create mode 100644 domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md create mode 100644 domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md diff --git a/domains/typescript/skills/avoid-any/repos/core.md b/domains/typescript/skills/avoid-any/repos/core.md new file mode 100644 index 00000000..4e0aa1f3 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/core.md @@ -0,0 +1,60 @@ +--- +repo: core +parent: avoid-any +--- + +# `avoid-any` in core + +## The rule is a deliberate re-enable — and it does not travel with the code + +`@typescript-eslint/no-explicit-any` is `'error'` at `eslint.config.mjs:152`, in the `files: ['**/*.ts', '**/*.mts']` block (`:135`) that extends the shared config (`:136`). The comment above it says so outright: + +``` +// Enable rules that are disabled in `@metamask/eslint-config-typescript` +``` + +`@metamask/eslint-config-typescript@^15.0.0` (`package.json:70`) sets `no-explicit-any: 'off'` at `src/index.mjs:46`. The same is true of the majors extension and mobile run (`14.1.1:50`, `13.0.0:44`) — **the org's shared TypeScript config permits explicit `any`, and each repo bans it separately.** + +The consequence is for extraction, not for writing code here: **a package that leaves core and adopts `@metamask/eslint-config-typescript` without copying `eslint.config.mjs:152` silently permits explicit `any`.** Nothing warns at the split; the code lints clean in its new home and the rule is simply gone. Copy the line with the code. + +The block's glob is `**/*.ts` and `**/*.mts`. There are currently no `.tsx`, `.cts` or `.mts` sources under `packages/` (`main`, 2026-08-31), so nothing falls outside it — a first `.tsx` file would. + +## The absorbed case is unreported here too + +`src/index.mjs:69`–`:73` sets `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access` and `no-unsafe-return` all `'off'`, under `// Recommended rules that require type information`. `eslint.config.mjs` does not turn any of them back on. Those five are exactly the rules typescript-eslint ships for the absorbed case, so the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI; an absorbed `any` is reported by nothing.** + +Core is nonetheless the tightest of the three on the neighbouring rules. Unlike extension and mobile it carries **no ESLint-v9 reversion block**, so `no-floating-promises` and `no-unsafe-function-type` are both live from the shared config's `recommended` / `recommendedTypeChecked` (verified in `@typescript-eslint/eslint-plugin@8.54.0`, `dist/configs/flat/recommended.js`). Two gaps it shares with the others anyway: + +- `no-unsafe-enum-comparison` — the shared config disables it at `src/index.mjs:78`. +- `restrict-template-expressions` — on, at `src/index.mjs:171`–`:177`, but neither there nor here does anything set `allowAny`, whose default is `true` (`@typescript-eslint/eslint-plugin`, `dist/rules/restrict-template-expressions.js`, `defaultOptions`). `` `${value}` `` with an `any` operand is permitted. + +## Where the absorbed `any` enters here + +**Not through `.js`.** `allowJs` and `checkJs` appear in no tsconfig in the repo (`git grep -l 'allowJs\|checkJs' -- '*.json'` returns nothing on `main`). `.js` files exist — `eslint.config.mjs:128` has a `files: ['**/*.{js,cjs}']` block for them — but they are outside every TypeScript program, so a `.ts` file cannot import one without a declaration. **The route that carries most of extension's exposure does not exist here. Do not go looking for it.** + +**Through the root `types/` directory instead, and it reaches every package at once.** Ten shorthand `declare module` files live there, each typing its entire module as `any` with no `any` written and no diagnostic under `strict`: + +`types/@metamask/contract-metadata.d.ts` · `types/@metamask/eth-json-rpc-filters.d.ts` (`/subscriptionManager`) · `types/@metamask/ethjs-provider-http.d.ts` · `types/@metamask/ethjs-unit.d.ts` · `types/@metamask/metamask-eth-abis.d.ts` · `types/eth-ens-namehash.d.ts` · `types/eth-json-rpc-infura/src/createProvider.d.ts` · `types/ethereum-ens-network-map.d.ts` · `types/ethjs-query.d.ts` · `types/single-call-balance-checker-abi.d.ts` + +Package tsconfigs pull the directory in wholesale — `packages/network-controller/tsconfig.json` ends `"include": ["../../types", "../../tests", "./src", "./tests"]` — so all ten are in every package's program, not local to whichever package imports them. An ABI or a token-metadata blob entering a controller through one of these is `any` at the point of entry, and every precise signature downstream of it is absorbed. + +**And an absorbed `any` crosses package boundaries as a published type.** `tsconfig.base.json:6` sets `composite: true`, and the root `tsconfig.json` wires the packages together with project references. A dependent package sees its dependency through the emitted `.d.ts`, not through source — so an absorbed `any` on an exported signature is not a local imprecision, it is the interface every consuming package compiles against, and the extension and mobile bundles beyond them. + +## Two settings that are not `any` sources + +`tsconfig.base.json:12` sets `strict: true`, and it is the only `"strict"` key in any tsconfig in the repo — no package weakens `noImplicitAny`. `noUncheckedIndexedAccess` is not set, which widens indexing results rather than producing `any`; a replacement for one of the shorthand declarations typed `Record` will read as total when it is not. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +Neither finds the absorbed form. Probe it at the call site with `IsAny` — the probe, its controls, and the `declare module` composition that turns one of these shorthand declarations into a real type are in `tsc-blindspots`. + +## Open questions + +- Which of the ten shorthand declarations are actually imported, and from which packages? Settled by resolving importers per specifier, not by the file list. +- Do any package `src` files re-export a value sourced from one of them, publishing the `any` through a `.d.ts`? Settled by an `IsAny` probe against each package's built declaration output, not against its source. +- Whether every package tsconfig includes `../../types`, or only some. Checked here for `network-controller` only; settled by reading the `include` of each `packages/*/tsconfig.json`. diff --git a/domains/typescript/skills/avoid-any/repos/metamask-extension.md b/domains/typescript/skills/avoid-any/repos/metamask-extension.md new file mode 100644 index 00000000..cc954053 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/metamask-extension.md @@ -0,0 +1,63 @@ +--- +repo: metamask-extension +parent: avoid-any +--- + +# `avoid-any` in metamask-extension + +## The rule is a re-enable, and the rules that would catch the absorbed case are off + +`@typescript-eslint/no-explicit-any` is `'error'` at `.eslintrc.js:162`. That line **restores** a rule the shared config switches off. Extension is on `@metamask/eslint-config-typescript@^14.1.1` (`package.json:620`), whose `src/index.mjs` sets: + +- `:50` — `@typescript-eslint/no-explicit-any: 'off'` +- `:73`–`:77` — `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access`, `no-unsafe-return`, all `'off'`, under the comment `// Recommended rules that require type information` + +Those five are the rules typescript-eslint ships for the absorbed case. Nothing in `.eslintrc.js` turns any of them back on. So the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI, and an absorbed `any` is reported by nothing at all.** + +## Three further rules off — extension's distinguishing gap + +The comment at `.eslintrc.js:272`–`:276` introduces a block of reversions — *"removing changes to our shared ESLint config made after version v9 … a temporary measure to get us to ESLint v9 compatible versions, at which point we can restore the intended rules"* — and the rules it governs run from `:277` to `:306`. Three of them bear on `any`: + +- **`no-floating-promises: 'off'` (`:284`)** — with the `no-unsafe-*` rules also off, a call whose return is absorbed `any` and a call that drops a real promise are the same unreported line. This rule is the last one that would have noticed the value was thenable. +- **`no-unsafe-enum-comparison: 'off'` (`:289`)** — comparing an enum member against a value that is `any` typechecks. Note this one is **not** extension-specific: the shared config already disables it at `src/index.mjs:82` (`// Recommended rules that we do not want to use`), so `:289` is redundant and the gap exists in mobile and core too. +- **`no-unsafe-function-type: 'off'` (`:291`)** — **this one is extension-only.** `Function` accepts any argument list and returns `any`, so `const f: Function` is an `any`-producing annotation with no `any` written. Mobile and core do not disable it, and it ships in typescript-eslint v8's `recommended` (verified in `@typescript-eslint/eslint-plugin@8.54.0`, `dist/configs/flat/recommended.js`). + +A fourth rule looks tightened and is not. `restrict-template-expressions` gets a local config at `:262` (`allowBoolean`, `allowNumber`) — the same option object the shared config already passes at `src/index.mjs:171`–`:177`. Neither sets `allowAny`, whose default is `true` (`@typescript-eslint/eslint-plugin`, `dist/rules/restrict-template-expressions.js`, `defaultOptions`). So `` `${value}` `` with an `any` operand is permitted. Same conclusion in core, by the same default; in mobile the rule is `'off'` outright. + +## Where the absorbed `any` enters here + +**The `.js` boundary is the largest of the three repos.** `tsconfig.json` sets `allowJs: true` and never sets `checkJs`. Inside its `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) there are **1,182 `.js`/`.jsx` files against 7,373 `.ts`/`.tsx`** (`origin/main`, 2026-08-31). An untyped JS export's parameters and return are `any` at every TS call site, with zero diagnostics — verified by probe under `--strict` (`IsAny[0]>` resolves `true`, `tsc` exits 0). + +**Two bodyless ambient module declarations**, of 16 `declare module` lines in the repo: + +- `shared/lib/declare-modules.d.ts:1` — `declare module 'human-standard-token-abi';` +- `types/lavamoat__lavadome-core.d.ts:1` — `declare module '@lavamoat/lavadome-core';` + +A shorthand declaration types **every** import from that module as `any`, silently, under `strict` (verified by the same probe). Both files are inside `include`. + +**The 300 story files get neither `tsc` nor the rule, and the two exclusions compound.** `.eslintrc.js:26`–`:28` builds a `tsconfig` object from `tsconfig.json` with the TypeScript API — `ts.findConfigFile`, `ts.readConfigFile`, `ts.parseJsonConfigFileContent` — and `:148` uses `tsconfig.fileNames` as the `files` list for the block that carries `no-explicit-any`. So the rule's scope *is* the tsconfig program. `tsconfig.json` excludes `**/*.stories.ts` and `**/*.stories.tsx` (`:31`–`:32`) — **300 files on `origin/main`** — which puts them outside `fileNames` and therefore outside the rule as well. The repo says so itself, in the docblock at `.eslintrc.js:684`–`:691`: *"Storybook (JavaScript only) … This block is for overriding settings from the base config. It's JavaScript-only because the Storybook TypeScript files don't have the base config applied."* In those 300 files an explicit `any` is not an error and an absorbed `any` is not even typechecked. + +The same mechanism generalises: any `.ts`/`.tsx` file outside `tsconfig.json`'s `include` list is outside `no-explicit-any` too, silently, with no entry in `.eslintrc.js` naming it. Beyond the stories that is currently 3 files, all under `.devcontainer/` (7,379 `.ts`/`.tsx` on `origin/main`; 7,373 in the `include` dirs, 3 at repo root matching `*.ts`). The hole is real and small — it is the stories that carry it. + +The parent skill's own absorbed example (`shared/lib/token-util.ts`, ethers dynamic-method results) is extension code; it is the shape to expect wherever a precise signature sits downstream of ethers, a `.js` module, or one of the two shorthand declarations above. + +## Two settings that are not `any` sources, so they don't belong in this hunt + +- `useUnknownInCatchVariables: true` is set explicitly in `tsconfig.json` (already implied by the inherited `strict`). Catch bindings are `unknown`, so no `catch (e: any)` is needed. +- `noUncheckedIndexedAccess` is not set. That widens indexing results, not `any` — a replacement for a bodyless declaration typed `Record` will read as total when it is not. + +`strict` is inherited from `@tsconfig/node22` (`node_modules/@tsconfig/node22/tsconfig.json`), not declared locally — a local `tsconfig.json` edit that changed `extends` would drop `noImplicitAny` with nothing in the file mentioning it. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +The absorbed form is invisible to both. Probe it at the call site with `IsAny` — the probe, its controls, and the `declare module` composition that repairs one are in `tsc-blindspots`. + +## Open questions + +- How many of the 1,182 in-program `.js` files are actually imported from `.ts`? Settled by resolving each `.js` file's importers, not by the file count above. +- Do the two shorthand declarations still need to be shorthand, or do the packages now ship types? Settled by checking each package's `types`/`exports` field at the installed version. diff --git a/domains/typescript/skills/avoid-any/repos/metamask-mobile.md b/domains/typescript/skills/avoid-any/repos/metamask-mobile.md new file mode 100644 index 00000000..ccafe744 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/metamask-mobile.md @@ -0,0 +1,67 @@ +--- +repo: metamask-mobile +parent: avoid-any +--- + +# `avoid-any` in metamask-mobile + +## The rule is a re-enable, and the rules that would catch the absorbed case are off + +`@typescript-eslint/no-explicit-any` is `'error'` at `.eslintrc.js:170`, inside the `files: ['*.{ts,tsx}']` override (`:155`) that extends `@metamask/eslint-config-typescript` (`:156`). That line **restores** a rule the shared config switches off. Mobile is on `^13.0.0` (`package.json:635`), whose `src/index.js` sets: + +- `:44` — `@typescript-eslint/no-explicit-any: 'off'` +- `:67`–`:71` — `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access`, `no-unsafe-return`, all `'off'` + +Those five are the rules typescript-eslint ships for the absorbed case, and nothing in `.eslintrc.js` turns any of them back on. So the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI, and an absorbed `any` is reported by nothing at all.** + +One thing mobile gets right that extension does not: the block's `files` is a plain glob, `'*.{ts,tsx}'`. Extension derives its equivalent list from the tsconfig program, so a file excluded from typechecking there loses `no-explicit-any` with it. Here the two scopes are independent — excluding a file from `tsconfig.json` does not remove the rule from it. + +## Three more rules off, and one that stays on + +The comment at `.eslintrc.js:186` introduces the same ESLint-v9 reversion block extension carries (*"a temporary measure to get us to ESLint v9 compatible versions"*); the rules it governs run from `:191` to `:223`. Three of them bear on `any`: + +- **`no-floating-promises: 'off'` (`:199`)** — with the `no-unsafe-*` rules also off, a call returning absorbed `any` and a call dropping a real promise are the same unreported line. +- **`no-unsafe-enum-comparison: 'off'` (`:206`)** — redundant with the shared config, which already disables it; the gap is not mobile-specific. +- **`restrict-template-expressions: 'off'` (`:221`)** — `` `${value}` `` with an `any` operand goes unremarked. Extension and core keep the rule on but leave `allowAny` at its default `true`, so the outcome is the same in all three; only the reason differs. + +`no-unsafe-function-type` is **not** disabled here, unlike extension. It ships in typescript-eslint v8's `recommended` and mobile is on `@typescript-eslint/eslint-plugin@^8.1.0` (`package.json:692`), so `const f: Function` — an `any`-producing annotation with no `any` written — is caught in mobile and not in extension. + +## The per-path override is the lever for tightening this + +`.eslintrc.js:471` scopes `no-floating-promises: 'error'` (`:628`) back on for `app/**/*-method-action-types*.ts`, under the Perps Core-alignment comment at `:459`. Whatever its motivation, it is the worked example: a team wanting the five `no-unsafe-*` rules on in their own directory does it with an `overrides` entry on their glob, not a repo-wide flip. + +## Where the absorbed `any` enters here + +**`app/declarations/index.d.ts` is the largest bodyless-declaration surface of the three repos** — **14 of its 38 `declare module` lines are shorthand**, and a shorthand declaration types every import from that module as `any`, silently, under `strict` (verified by probe: `IsAny` resolves `true`, `tsc` exits 0): + +- `:2`–`:5` — four `react-native-safe-area-context/src/*` deep imports +- `:9` `*.mp4` · `:11` `@metamask/react-native-payments/lib/js/__mocks__` · `:13` `react-native-fade-in-image` · `:15` `react-native-fast-crypto` · `:17` `react-native-minimizer` · `:19` `xhr2` +- `:307` `@metamask/react-native-search-api` · `:439`–`:440` `@tommasini/react-native-scrollable-tab-view` and its `/DefaultTabBar` · `:442` `react-native-tcp-socket` + +The same file also carries an **untyped ambient const** at `:302`–`:305` — `declare module '@metamask/react-native-actionsheet' { const ActionSheet; export default ActionSheet; }`. In an ambient context a missing annotation is `any` and `noImplicitAny` does not fire; the probe confirms it. This is the absorbed form wearing a declaration's clothes, and grepping for shorthand `declare module` will not find it. + +**The `.js` boundary exists but is narrow.** `tsconfig.json:6` sets `allowJs: true` and `checkJs` is never set. Under `app/` there are **237 `.js`/`.jsx` files against 12,797 `.ts`/`.tsx`** (`main`, 2026-08-31) — proportionally about a seventh of extension's exposure. `app/core/InpageBridgeWeb3.js` is in `exclude` (`tsconfig.json:94`), so it is outside the program entirely. + +**`lib: ["es2022"]` with no DOM (`tsconfig.json:5`) routes web-shaped code back into hand-written declarations.** A value with no ambient type either fails to compile or gets served by `app/declarations/`, which is where the shorthand list above lives. `index.d.ts:317` augmenting the global `Crypto` interface is this pressure showing up in the file. + +**`paths` maps 20 specifiers to a chosen declaration file** (`tsconfig.json:15`), several of them deep into `node_modules/**/dist/**/*.d.cts`, and one (`tsconfig.json:18`) to a local `app/declarations/@keystonehq/ur-decoder.d.ts`. The type at every call site of those imports is decided by `tsconfig.json` rather than by the package — so re-reading the import statement tells you nothing about what was resolved. + +## One setting that is not an `any` source + +`noUncheckedIndexedAccess` is not set. That widens indexing results, not `any`; a replacement for a shorthand declaration typed `Record` will read as total when it is not. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -nE "^\s+(const|let|var) [A-Za-z_$][\w$]*;$" -- '*.d.ts' # untyped ambient = also any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +None of these finds the absorbed form in ordinary source. Probe it at the call site with `IsAny`; the probe, its controls, and the `declare module` composition that repairs one are in `tsc-blindspots`. + +## Open questions + +- Do any of the 20 `paths` targets resolve to `any` at their call sites? Settled by an `IsAny` probe on one import from each mapped specifier, not by reading the mapping. +- Which of the 14 shorthand declarations still need to be shorthand? Settled by checking each package's `types`/`exports` field at the installed version. +- Whether mobile's resolved `@typescript-eslint` version treats `no-unsafe-function-type` as `recommended` — `^8.1.0` was read from `package.json`, not from `yarn.lock`. Settled by reading the lockfile entry. diff --git a/domains/typescript/skills/derive-types/repos/core.md b/domains/typescript/skills/derive-types/repos/core.md new file mode 100644 index 00000000..b9d24dbb --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/core.md @@ -0,0 +1,102 @@ +--- +repo: core +parent: derive-types +--- + +# Deriving in `core` + +Paths, line numbers and counts below are verified against `origin/main` (2026-08-31), not a working +tree — a local checkout can sit far enough behind that every line number in this file is wrong. + +`core` is where the authoritative types are *written*. Everything in this repo is a source that +`metamask-extension` and `metamask-mobile` derive from, so a loose or unexported type here becomes +a hand-written copy in two consumers. + +## Where the authoritative source lives + +Package granularity. `packages//src/index.ts` is the contract — read it before defining +anything. + +| Source | Where | +| --- | --- | +| Controller state, action union, event union, messenger | `packages//src/Controller.ts`, re-exported from `src/index.ts` | +| Per-method action types | `packages//src/Controller-method-action-types.ts` (generated, 88 files) | +| Get-state action and state-change event builders | `@metamask/base-controller` — `ControllerGetStateAction`, `ControllerStateChangeEvent` | +| Action/event extraction utilities | `@metamask/messenger` — `ExtractActionResponse`, `ExtractActionParameters`, `ExtractEventPayload`, `ActionHandler`, `MessengerActions`, `MessengerEvents` | +| Schema-backed types | `Infer` from `@metamask/superstruct` | + +## The generated action types are the source, not a convenience + +A controller lists its exposed methods in `MESSENGER_EXPOSED_METHODS` (94 files under `packages/` +do). The generator turns each into an action whose handler is an indexed access on the class: + +```typescript +// packages/accounts-controller/src/AccountsController-method-action-types.ts (generated) +export type AccountsControllerGetAccountAction = { + type: `AccountsController:getAccount`; + handler: AccountsController['getAccount']; +}; +``` + +`yarn messenger-action-types:check` runs inside the root `lint` script, so a hand-written +handler signature that disagrees with the method is a lint failure, not a silent drift. Add the +method name to `MESSENGER_EXPOSED_METHODS` and regenerate; never write the action type by hand. + +The state action and change event are derived the same way, from `base-controller` generics +rather than restated: + +```typescript +// packages/accounts-controller/src/AccountsController.ts +export type AccountsControllerGetStateAction = ControllerGetStateAction< + typeof controllerName, + AccountsControllerState +>; +``` + +## Prefer the messenger's own extractors + +`@metamask/messenger` exports `ExtractActionResponse` and `ExtractActionParameters`, +which do what the parent skill's `ReturnType` / `Parameters<...>` do but are +matched to the action shape. Reach for them first inside `core`. + +Measured caveat, so you know what you are adopting: outside `packages/messenger` these are almost +unused — `ExtractEventPayload` has one non-test call site (`AccountsController.ts:933`) and +`ExtractActionResponse` has none. They are exported and correct; they are not yet the prevailing +idiom, and `ReturnType` is what you will find in review. + +## Deriving across a package boundary + +`tsconfig.base.json` sets `composite: true`, and `tsconfig.packages.json` maps +`"@metamask/*": ["../*/src"]`. So a cross-package type import typechecks against the sibling's +**uncompiled source** — no build step stands between you and the authoritative type, and there is +no reason to copy a shape because "the other package isn't built." + +One edit makes a new cross-package derivation legal, and the rest is generated: add the package to +`dependencies` (or `devDependencies`) in `packages//package.json`, then run +`yarn lint:tsconfigs:fix`. `scripts/lint-tsconfigs/lint-package-tsconfigs.mts` derives the expected +`references` in both `tsconfig.json` and `tsconfig.build.json` from the manifest's dependencies and +writes them; `yarn lint:tsconfigs:all` enforces the result inside the root `lint` script. Do not +hand-maintain the `references` arrays. + +Those entries do not affect resolution — `paths` already did that. They order `tsc --build`. + +`packages/bridge-controller/src/types.ts` is the worked example: it derives against types imported +by package name from eleven sibling `@metamask/*` packages, and derives 13 more with +`Infer`. + +## Config that bears on derivation + +- `module` / `moduleResolution` are `Node16`, so package `exports` subpaths resolve natively. +- `strict: true`; `noUncheckedIndexedAccess` is **not** set. `State['someRecord'][key]` is the + value type, not `value | undefined` — a type derived out of a `Record` will claim more than the + runtime guarantees. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `@typescript-eslint/no-explicit-any` is `error` (`eslint.config.mjs:152`). The legal escape from + that rule is a too-wide hand-written type, which the parent skill's cast-tax argument covers; + passing the lint rule is not evidence the type was derived. + +## Open + +- Whether `ExtractActionResponse` / `ExtractActionParameters` are intended to become the house + idiom or stay internal to `packages/messenger`. Settled by asking the messenger package owners, + or by a decision record in `MetaMask/decisions`. diff --git a/domains/typescript/skills/derive-types/repos/metamask-extension.md b/domains/typescript/skills/derive-types/repos/metamask-extension.md new file mode 100644 index 00000000..fed595e9 --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/metamask-extension.md @@ -0,0 +1,106 @@ +--- +repo: metamask-extension +parent: derive-types +--- + +# Deriving in `metamask-extension` + +Most authoritative types this repo uses are **not in this repo**. `package.json` carries 134 +`@metamask/*` dependencies at caret ranges, resolving through `node_modules` to built declarations +(`dist/index.d.cts`). There is no `paths` mapping to sibling source. + +Paths, line numbers and counts below are verified against `origin/main` (2026-08-31), not a working +tree — a local checkout can sit far enough behind that every line number in this file is wrong. + +That is what makes deriving non-optional here. Upstream shapes change on a lockfile bump the +extension *performs* but does not *author*. A derived type fails the build at that bump, where you +can see it. A hand-written copy keeps compiling and is now wrong. + +## Where to look, in order + +1. `node_modules/@metamask/` — the published state, action and event types from `core`. +2. `app/scripts/controllers/` — 40 extension-local controllers, including 15 generated + `*-method-action-types.ts` files that derive `handler: Controller['method']`. +3. `shared/types/background.ts` and `ui/store/types.ts` — the already-derived spine below. + +## The spine is already derived — join it, don't re-derive it + +**Background state.** `shared/types/background.ts` builds the redux `metamask` slice type twice and +asserts the two agree: + +- `ControllerStatePropertiesEnumerated` — one indexed access per property into an upstream state + type (`transactions: TransactionControllerState['transactions']`, 255 rows in all). +- `ControllerStateTypesMerged` — the intersection of those same `*State` types. +- `FlattenedBackgroundStateProxy` — `IsEquivalent extends true ? … : never`. + +That last line is a drift alarm, not decoration. When an upstream package adds or removes a state +property, the two sides disagree and the whole redux slice type collapses to `never`, which fails +loudly across the UI. `IsEquivalent` lives in `shared/types/type-level-utils.ts`. + +**Redux state.** `ui/store/types.ts` derives the root from the store rather than declaring it: + +```typescript +type Store = ReturnType; +export type MetaMaskReduxState = ReturnType; +export type MetaMaskReduxDispatch = Store['dispatch']; +``` + +So UI types derive from `MetaMaskReduxState['metamask'][…]`, never from a fresh import of the +controller state — that path already carries the `IsEquivalent` guard. `ui/store/actions.ts` uses +`MetaMaskReduxState['metamask']` as the background-call return type in several places. +`configureStore`'s input type uses `Omit` to override the slices redux +infers as `never`, which is derive-then-override rather than restate. + +**Messenger clients.** `app/scripts/messenger-client-init/controller-list.ts` declares the +`MessengerClient` union over imported controller classes, then +`MessengerClientFlatState = AccountOrderController['state'] & AccountsController['state'] & …`. +`types.ts` derives the rest from the union — `MessengerClientName = MessengerClient['name']`, +`MessengerClientByName` as a mapped type, `MessengerClientPersistedState` from that. + +**Root messenger.** `app/scripts/lib/messenger.ts` derives the root action and event unions from +the factory table: + +```typescript +type ChildMessengers = ReturnType< + (typeof MESSENGER_FACTORIES)[keyof typeof MESSENGER_FACTORIES]['getMessenger'] +>; +export type RootMessengerActions = MessengerActions | DefaultActions; +``` + +Registering a factory in `MESSENGER_FACTORIES` +(`app/scripts/messenger-client-init/messengers/index.ts`) widens the root union with no hand edit. +`messenger-client-init/utils.ts` derives each init function's messenger parameters the same way, +via `ReturnType<(typeof MESSENGER_FACTORIES)[Name]['getMessenger']>`. + +## Config that bears on derivation + +- Extends `@tsconfig/node22`, which supplies `strict`, `skipLibCheck` and `module`/ + `moduleResolution: node16`. Strictness is inherited, not set locally — do not read its absence + from `tsconfig.json` as it being off. Node16 means subpath exports resolve, so + `@metamask/profile-sync-controller/auth` is a legal place to find an authoritative type + (`app/scripts/lib/state-utils.ts:1`). +- `noUncheckedIndexedAccess` is **not** set. Deriving an element type out of a `Record` gives the + value type, not `value | undefined`. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `lib: ["DOM", "es2023"]`, `jsx: react`, `allowJs: true`. `allowJs` means a `typeof` derivation can + land on an untyped JS module and silently widen; check what the source file actually is. +- `import-x/no-restricted-paths` is `error` (`.eslintrc.js:838`, architectural zones per ADR 0021 + `modularize-routes`). Deriving across a zone boundary needs `import type` plus the documented + disable — `shared/types/background.ts` opens with exactly that, on the grounds that type imports + are stripped at runtime. +- `@typescript-eslint/no-explicit-any` is `error` (`.eslintrc.js:162`). `Record` + passes that rule and is the shape the parent skill's `getMetaMaskState` example was rejected for; + a clean lint run is not evidence a type was derived. + +## The generated action types are enforced here too + +The 15 `app/scripts/controllers/*-method-action-types.ts` files derive `handler: Controller['method']` +exactly as `core`'s do, and they run the same tool: `@metamask/messenger-cli` (published from +`core`'s `packages/messenger-cli`) supplies the `messenger-action-types` binary, and +`yarn messenger-action-types:check` is the last step of this repo's `lint` script. Regenerate with +`yarn messenger-action-types:generate`; never hand-edit an action's handler signature. Tooling also +knows these files are generated (`.eslintrc.js:872`, `oxfmt.config.mts:13`). + +That the extension and `core` share one generator is the practical form of this skill's cross-repo +point: the action types on both sides of the package boundary are produced from the same class +methods by the same binary, so the only way they can disagree is if someone writes one by hand. diff --git a/domains/typescript/skills/derive-types/repos/metamask-mobile.md b/domains/typescript/skills/derive-types/repos/metamask-mobile.md new file mode 100644 index 00000000..b450bff5 --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/metamask-mobile.md @@ -0,0 +1,125 @@ +--- +repo: metamask-mobile +parent: derive-types +--- + +# Deriving in `metamask-mobile` + +Read via the GitHub API against `MetaMask/metamask-mobile` `main` on 2026-08-31, not from a local +checkout. Every path, line number and count below was re-verified against `main` at that date; +confirm before quoting a line number in review, and never verify one against a working tree. + +As in the extension, most authoritative types are upstream: `package.json` carries 132 +`@metamask/*` dependencies at caret ranges. Mobile and the extension sit on **different versions of +the same packages**, so a shape copied by hand here diverges from the same shape copied by hand +there, and neither repo can see the other drift. Deriving is what keeps both pinned to whatever +`core` actually says. + +## Where the authoritative source lives + +`app/core/Engine/types.ts` is the hub. Two hand-maintained maps sit at the top, and everything else +in the file is derived from them: + +- `MessengerClients` (line 854) — controller name to controller **class**, one row each. +- `EngineState` (line 979) — controller name to the upstream `*State` type, one row each. + +The rows are written by hand; the values are imported, not restated. So the shapes are already +authoritative — what is hand-maintained is the *membership* of the maps. + +Everything downstream derives: + +| Derived type | Line | Form | +| --- | --- | --- | +| `MessengerClientName` | 1060 | `keyof MessengerClients` | +| `MessengerClient` | 1065 | `MessengerClients[MessengerClientName]` | +| `MessengerClientsByName` | 1068 | mapped over `MessengerClientName` | +| `RequiredControllers` | 560 | `Omit` | +| `OptionalControllers` | 576 | `Pick` | +| `EngineContext` | 971 | `RequiredControllers & Partial` | +| `MessengerClientPersistedState` | 1188 | mapped, `MessengerClientsByName[Name]['state']` | +| `MessengerClientMessengersByName` | 1198 | `typeof MESSENGER_FACTORIES` | +| `Permissions` | 589-590 | `ReturnType[keyof …]` | + +`MESSENGER_FACTORIES` itself is at `app/core/Engine/messengers/index.ts`. + +## No completeness guard on the engine maps + +The extension asserts its two constructions of background state agree, and collapses the type to +`never` when they do not. Mobile has no equivalent: a GitHub code search for `IsEquivalent` across +this repo returns 0 hits (positive control — `MessengerClientsByName` returns 4 — so the search is +reaching the code). + +Consequence for derivation work here: adding a controller and forgetting its `EngineState` row +typechecks. Nothing fails. Derive *from* `EngineState` freely; do not assume `EngineState` is +complete because the build is green. + +## Redux runs the opposite direction from the extension + +`app/reducers/index.ts:66` declares `export interface RootState` by hand, with +`engine: { backgroundState: EngineState }` at line 71, and passes it *into* +`combineReducers` at line 193. The extension derives its root state out of the +configured store; mobile supplies its root state to the store. + +So when deriving a UI type here, `RootState['engine']['backgroundState'][…]` does reach the upstream +controller state types and is the right path. The rest of `RootState` — the ~24 non-engine slices +under `app/reducers/` — is a hand-written boundary, and a derivation that bottoms out there has +reached a declaration rather than a source. + +## The strongest derivation in the repo + +`app/messengers/ui-messenger.ts` derives the entire UI-facing action and event surface from +`GlobalActions` / `GlobalEvents` by transformation rather than by restatement — asynchronizing every +handler with `infer`, filtering to JSON-serializable actions with a conditional on +`Parameters`, and deriving the exclusion list off a const array +(`(typeof MESSENGERS_WITH_EXCLUSIONS)[number]['EXCLUDED_CAPABILITIES']['actions'][number]`). It is +also the only file across the three repos that uses `ExtractActionResponse` from +`@metamask/messenger`. Copy its shape when a consumer needs a *modified* view of an upstream union; +the alternative is re-declaring the union with the modification baked in. + +## `moduleResolution` is `node`, and that hides exported types + +`tsconfig.json` sets `"moduleResolution": "node"`, not `Node16`. Package `exports` subpaths +therefore do not resolve, and the repo shims each one it needs with an explicit `paths` entry into +`node_modules/@metamask//dist/**/*.d.cts` — `@metamask/keyring-api/v2`, +`@metamask/perps-controller/types`, `@metamask/delegation-controller/types` and others, under a +`// TODO: Remove these once we use Node16 module resolution.` comment. + +**Before concluding an authoritative type is not exported, check `paths`.** A type that lives behind +a subpath will not resolve until an entry exists, and the failure looks like the type not existing. +Adding the `paths` entry is the fix; hand-writing the shape because the import "doesn't work" is the +failure this skill is about. + +## Other config that bears on derivation + +- `strict: true` set explicitly; `noUncheckedIndexedAccess` **not** set, so an element type derived + out of a `Record` is the value type, not `value | undefined`. +- `lib: ["es2022"]` with **no `DOM`**. A derived type that transitively references a DOM type will + not resolve here even though the same derivation compiles in the extension, whose `lib` includes + `DOM`. This is the most common way a type that works upstream fails to land in mobile. +- `skipLibCheck: true` — errors inside upstream `.d.cts` files are not reported. The derived type is + still checked where you use it, so this suppresses noise rather than the signal you want. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `target: esnext`, `module: commonjs`, `jsx: react-native`, `allowJs: true`. As in the extension, + `allowJs` means a `typeof` derivation can land on an untyped JS module and widen silently. +- `@typescript-eslint/no-explicit-any` is `error` (`.eslintrc.js:170`); a too-wide hand-written type + passes it. + +## The generated action types come from the same binary as `core`'s + +Generated `*-method-action-types.ts` files exist here (imported by +`app/core/Engine/controllers/rewards-controller/` and +`app/components/UI/Predict/controllers/PredictController.ts`, and recognized in `.eslintrc.js` and +`.eslintignore`), and they are enforced: `yarn messenger-action-types:check` is the second half of +this repo's `lint` script, running the `messenger-action-types` binary from +`@metamask/messenger-cli ^0.2.0` — the same tool, from `core`'s `packages/messenger-cli`, that +`core` and the extension run. Regenerate with `yarn messenger-action-types:generate`. + +So all three repos derive their action handlers from the same class methods with one shared +generator. A hand-written handler signature is the only thing that can put them out of step. + +## Open + +- The selector layer was not surveyed. Whether mobile's selectors derive from + `RootState['engine']['backgroundState']` or re-import controller state types directly is unknown, + and it decides which path this skill should point at for UI work. Settled by a sweep of + `app/selectors/`. diff --git a/domains/typescript/skills/tsc-blindspots/repos/core.md b/domains/typescript/skills/tsc-blindspots/repos/core.md new file mode 100644 index 00000000..8b768208 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/core.md @@ -0,0 +1,199 @@ +--- +repo: core +parent: tsc-blindspots +--- + +# Blind spots — core + +What `MetaMask/core`'s configuration does **not** check. Two of the parent skill's +classes do not exist here and one exists only here, so do not carry the extension +or mobile playbook across unchanged. + +## The JS boundary does not exist here — say so, do not omit it + +`tsconfig.base.json` does not set `allowJs`, and it is absent from the resolved +options for a package (`npx tsc -p packages/assets-controllers/tsconfig.json +--showConfig`). Independently, `packages/*/src` contains **0** `.js` files against +**1,380** `.ts`. + +So the parent skill's central premise — *"across a JavaScript boundary (`checkJs` +off) it checks nothing at all"* — has no instance in core. A review that reports +"the callers are still `.js`, so this type is validated against nothing" is wrong +here, and a `checkJs` finding copied from an extension review does not transfer. + +State this explicitly in a core review rather than leaving it out. Its absence is +what makes the two classes below the ones worth spending the time on. + +## Unchecked indexing — and it is not uniform across packages + +`tsconfig.base.json` does not set `noUncheckedIndexedAccess`, so `record[key]` and +`arr[i]` yield `T`, never `T | undefined`. But three configs opt in: + +| Config | Line | +|---|---| +| `packages/json-rpc-engine/tsconfig.json` | :9 | +| `packages/eth-json-rpc-provider/tsconfig.json` | :9 | +| `tsconfig.scripts.json` | :19 | + +That is **2 of the 75 package tsconfigs**, plus the scripts config. Read the +package's own file before concluding an index expression is unchecked — the answer +differs by directory, which no other MetaMask repo here requires you to check. + +The same expression appears both guarded and unguarded in two files of one package, +`packages/assets-controllers`: + +```ts +// packages/assets-controllers/src/TokenBalancesController.ts:510-512 (also :526-528) +const networkConfig = networkConfigurationsByChainId[chainId]; +const { networkClientId } = + networkConfig.rpcEndpoints[networkConfig.defaultRpcEndpointIndex]; + +// packages/assets-controllers/src/AccountTrackerController.ts:590-596 +.map((hexChainId) => { + const networkConfig = networkConfigurationsByChainId[hexChainId]; + return networkConfig?.rpcEndpoints[ + networkConfig.defaultRpcEndpointIndex + ]?.networkClientId; +}) +.filter((id): id is NetworkClientId => id !== undefined); +``` + +Two index operations each, both typed non-nullable under `strict: true`. The second +site guards both with `?.` and then needs the type-predicate `.filter` to remove +the `undefined` it introduced by doing so; the first guards neither and destructures +straight through. `tsc` has no opinion about either, so consistency on this axis is +a review property in core, not a checked one. + +### A package's source is checked under stricter settings than its emitted types + +`packages/json-rpc-engine/tsconfig.json` sets `noUncheckedIndexedAccess` and +`exactOptionalPropertyTypes`; its `tsconfig.build.json` sets neither — it extends +`tsconfig.packages.build.json`, which does not. **0 of the build configs set +either.** So a package's source is checked under stricter settings than the `.d.ts` +consumers read is emitted under. + +**Open question:** whether that asymmetry changes any emitted signature — an +inferred return type of a function returning `arr[i]` is the candidate. I did not +test it. Settle it by building the package and diffing the emitted `dist/*.d.ts` +against one emitted with the flags added to `tsconfig.build.json`. + +## Bodiless ambient shims — verified `any` at live call sites + +Ten files under `types/` are a single line of the form `declare module 'x';` with no +body, which types the entire module `any`. Every package includes them — +`"include": ["../../types", "./src", "../../tests"]`. + +Two cases, needing different remedies: + +**1. The shim overrides a package that ships its own types.** +`@metamask/metamask-eth-abis` has `"types": "dist/index.d.ts"` in its +`package.json`, and `types/@metamask/metamask-eth-abis.d.ts` shadows it. Verified +with the parent skill's substitution method, replicating a package's arrangement +(`strict`, `module`/`moduleResolution` `Node16`, `target ES2020`, +`lib ES2020 + DOM`, core's `node_modules`): + +```ts +import { abiERC20 } from '@metamask/metamask-eth-abis'; +import contractMap from '@metamask/contract-metadata'; +type IsAny = 0 extends 1 & T ? true : false; +const abiIsAny: IsAny = true; +const mapIsAny: IsAny = true; +``` + +- With `types/**/*.d.ts` in `include`: **clean**, and flipping either to `false` + errors `TS2322` — so both are `any`. +- With `types/` removed from `include`: `abiIsAny` errors (`Type 'true' is not + assignable to type 'false'`) — the shipped declarations resolve and `abiERC20` is + **not** `any`. + +The shim is what makes it `any`, at these importers under +`packages/assets-controllers/src/` — `Standards/ERC20Standard.ts:6`, +`TokensController.ts:31`, `Standards/NftStandards/ERC1155/ERC1155Standard.ts:11`. +This is parent §7 false precision with a source you can delete: remove the shim. + +**2. The shim stands in for a package with no types.** +`@metamask/contract-metadata` has no `types`/`typings` field; with `types/` removed +the import is `TS7016`. Hand-writing is the parent's Step 2 case 7 — correct in +principle. The defect is that the shim asserts `any` rather than a shape, so +`contractMap` is `any` at `packages/assets-controllers/src/TokensController.ts:16` +and `.../TokenDetectionController.ts:10`. `single-call-balance-checker-abi` is the +same case, at `.../AssetsContractController.ts:19`. + +`@typescript-eslint/no-explicit-any` is `'error'` in `eslint.config.mjs`, in the +block commented *"Enable rules that are disabled in +`@metamask/eslint-config-typescript`"* — and it cannot see either case, because no +`any` is written anywhere. Neither can `skipLibCheck: false` (below): a bodiless +`declare module` is well-formed, so checking declarations finds nothing wrong with +it. + +## Project references — two entry points that resolve differently + +`composite: true` in `tsconfig.base.json`. Every package tsconfig lists its +dependencies under `references`, and every `tsconfig.build.json` references the +other packages' `tsconfig.build.json`. This is the class the other two repos do not +have, and the first thing to get right is which config you ran. + +**The `paths` mapping is not inherited by every entry point.** +`tsconfig.packages.json` sets `"@metamask/*": ["../*/src"]`, commented *"we ensure +that TypeScript resolves `@metamask/*` imports to the uncompiled source code."* + +| Config | Extends | Carries the `src` mapping | +|---|---|---| +| `packages/*/tsconfig.json` | `tsconfig.packages.json` | yes | +| `packages/*/tsconfig.build.json` | `tsconfig.packages.build.json` → `tsconfig.packages.json` | yes | +| `tsconfig.json` (root, `noEmit`) | `tsconfig.base.json` **directly** | **no** | + +So the root config is the one whose cross-package imports fall through to normal +`Node16` resolution and the package's `types` field, and the per-package configs — +lint *and* build alike — are the ones pointed at source. + +**Open question:** which file each entry point actually loads for a cross-package +import, since project references also redirect to declaration output. I did not +test it. Settle it before citing any typecheck as evidence about a cross-package +type: + +```bash +npx tsc -p packages//tsconfig.json --explainFiles | grep -i '' +npx tsc --build tsconfig.build.json --verbose --traceResolution 2>&1 | grep -i '' +``` + +**Four packages are in the build graph and absent from the root config.** +`tsconfig.json` lists **71** references; `tsconfig.build.json` lists **75**. The +difference is `eip-5792-middleware`, `eip-7702-internal-rpc-middleware`, +`logging-controller` and `storage-service` — reachable transitively through other +packages' `references`, but not named at the root. + +**And no script runs the root config.** The only `tsc` invocation in `package.json` +is `build:types` (`tsc --build tsconfig.build.json --verbose`); `build` is +`ts-bridge --project tsconfig.build.json`. `tsconfig.json`'s own comment says it is +*"used by the `lint` script in `package.json`, and by editors such as VSCode"*, and +`lint` runs eslint, prettier, constraints, depcheck and two scripts — no `tsc`. +**Do not cite "core typechecks clean" without naming the command you ran**, and do +not assume CI ran the config you are reading. + +That is not the same as "types are never checked": eslint's type-aware rules build +a program through the parser (`eslint.config.mjs` sets `parserOptions.tsconfigRootDir`; +the `project` setting comes from `@metamask/eslint-config-typescript`, which I did +not read). Type information is loaded — but a rule set is not `tsc` reporting every +diagnostic, and a probe's `TS2322` has nothing there to surface it. + +**Open question:** whether `tsc --build` here can report clean over an out-of-date +`dist/*.d.ts`. `build:types` already passes `--verbose`, which prints the +up-to-date decision per project — read that log rather than the exit code. + +What holds regardless of the resolution question: **consumers outside the repo — +extension and mobile — read `dist/*.d.ts`**, and no check inside core exercises +that path from a consumer's position. A type that is correct against `src` and +stale in `dist` is invisible here and breaks there. + +`skipLibCheck` is set in `tsconfig.packages.build.json` and **not** in +`tsconfig.packages.json`, so a per-package typecheck checks declaration files that +the build skips — the opposite of extension, where `skipLibCheck: true` is +inherited repo-wide. + +## Probe note + +`lib` is `["ES2020", "DOM"]`, so a DOM-typed probe compiles here. It does not in +mobile. `module` and `moduleResolution` are `Node16`, matching extension and not +mobile — a probe is portable between core and extension, and is not portable to +mobile without rewriting its imports. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md new file mode 100644 index 00000000..95f113ed --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md @@ -0,0 +1,134 @@ +--- +repo: metamask-extension +parent: tsc-blindspots +--- + +# Blind spots — metamask-extension + +What this repo's configuration does **not** check, and what that means for the +parent skill's five divergence shapes. How to *run* the two-arm proof here — heap, +probe location, the authoritative-source table, the `chrome.*` caveat — is +[references/metamask-extension.md](../references/metamask-extension.md). + +## `tsconfig.json` does not show you the strictness + +The local file names exactly one strictness flag, `useUnknownInCatchVariables`. +Everything else arrives through `"extends": "@tsconfig/node22/tsconfig.json"`. +Resolve it before concluding anything is off: + +```bash +npx tsc -p tsconfig.json --showConfig +``` + +That prints `strict`, `strictNullChecks`, `strictFunctionTypes`, `noImplicitAny`, +`alwaysStrict`, `strictBindCallApply`, `strictPropertyInitialization` and +`strictBuiltinIteratorReturn` all `true`, plus `target: es2022` and +`skipLibCheck: true` — none of which appear in `tsconfig.json`. + +**The failure this causes is under-reporting.** An auditor greps the local file for +`strict`, finds nothing, and downgrades a real nullability divergence to "tsc +wouldn't have caught it anyway." It would: `strictNullChecks` is on, so a dropped +`| undefined` (parent shape 2) *does* surface in a probe here. + +`noUncheckedIndexedAccess` is genuinely absent — checked across all three +tsconfigs in the repo (`tsconfig.json`, `development/webpack/tsconfig.webpack.json`, +`test/e2e/playwright/llm-workflow/tsconfig.json`), none of which set it. + +## The JS boundary, sized + +`allowJs: true` and `checkJs` unset. The parent reference explains why +`app/scripts/background.js` matters; the number is the part worth knowing: + +| Under `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) | Count | +|---|---| +| `.js` | 1,219 | +| `.ts` / `.tsx` | 7,245 | +| `.js` carrying `@ts-check` | **1** (`development/lib/build-type.js`) | + +Roughly one included file in seven asserts nothing and is checked against nothing. +A type hand-written for a function whose callers are all in that seventh is +unfalsifiable in +the parent skill's Step 1 sense — probe it, but expect Arm B to stay silent, and +report that as *unconstrained* rather than as *cleared*. + +## Unchecked indexing, in one function + +`noUncheckedIndexedAccess` is off, so `record[key]` and `arr[i]` both yield `T`, +never `T | undefined`. `shared/lib/network.utils.ts:238-256` shows the whole class +in nineteen lines: + +```ts +const enabledEip155Networks = + enabledNetworkMap[KnownCaipNamespace.Eip155] ?? {}; // :243 guard the author wrote + +const chainIds = Object.entries(enabledEip155Networks) + .filter(([_chainId, isEnabled]) => isEnabled) + .map(([chainId, _isEnabled]) => chainId) as Hex[]; // :247 escape hatch on the key type + +return chainIds + .map((chainId) => networkConfigurationsByChainId[chainId]) + .filter((config) => config !== undefined) // :251 guard the author wrote + .map( + (config) => + config.rpcEndpoints[config.defaultRpcEndpointIndex].networkClientId, // :254 no guard + ); +``` + +The record lookup at `:250` is typed `NetworkConfiguration`, so the `!== undefined` +filter at `:251` is guarding against something the compiler says cannot happen — the +author supplied it from knowledge of the data. Two lines later the array index at +`:254` is dereferenced immediately with no equivalent guard, and the compiler asked +for neither. If `defaultRpcEndpointIndex` is ever out of range the throw is at +runtime and `tsc` is green. + +**For review:** a hand-written guard on an index expression is evidence the author +knew the lookup could miss. Ask why the sibling index in the same expression has +none. `shared/lib/selectors/networks.ts:88` is the same shape with the cast form — +`networkConfigurationsByChainId[chainId as Hex]` on a parameter declared `string`. + +## `skipLibCheck: true` — declarations are not checked + +Inherited from the base. Every `.d.ts` is exempt: the twelve files in `types/` and +every dependency's declarations. + +`types/lavamoat__lavadome-core.d.ts` is a bodiless `declare module +'@lavamoat/lavadome-core';`, which types the entire module `any`. The package ships +no types (no `types`/`typings` field, no `.d.ts` in the package), so hand-writing is +the parent's Step 2 case 7 — correct in principle. The defect is that the shim +asserts `any` where it could assert a shape. I found no `.ts`/`.tsx` importer under +`app`, `shared` or `ui`; a `.js` importer would be invisible to `tsc` regardless. + +## The lint layer changes what you can grep for + +`.eslintrc.js:162` sets `@typescript-eslint/no-explicit-any` to `'error'`, inside the +override at `.eslintrc.js:148` scoped to `tsconfig.fileNames` filtered to `.tsx?` — +so it covers exactly the files `tsc` checks. + +**Consequence for parent §7 (false precision):** you cannot find an absorbed `any` +by grepping for `any`, because writing one is a lint error. The rule pushes authors +to `as` instead, which is why the parent's "escape hatches are the tell" section is +the productive search here. Use `IsAny` at the call sites, not a grep. + +`.eslintrc.js:278-296` disables a block of rules, commented *"removing changes to +our shared ESLint config made after version v9 … TODO: Remove these modifications +after the ESLint v9 update"*. Three of them matter to this skill: + +| Rule | Line | What stops being reported | +|---|---|---| +| `no-floating-promises` | :284 | an unawaited promise introduced while annotating | +| `no-unsafe-enum-comparison` | :289 | parent shape 1 — `string` typed where an enum belongs, then compared to an enum member | +| `no-unsafe-function-type` | :291 | a bare `Function` standing in for a call signature | + +`no-unsafe-enum-comparison` is the lint counterpart of the parent's first divergence +shape. With it off, a migration that widens an enum-valued field to `string` and +compares it to an enum member is green in **both** `tsc` and lint — so lint silence +is not a clearance here. These are marked temporary, so a finding they would have +caught is not a decision to accept the risk; say so when reporting one. + +## Probe note + +`incremental: true` with `tsBuildInfoFile: +node_modules/.cache/typescript/tsconfig.tsbuildinfo` means a cache persists between +Arm A and Arm B. **Open question:** whether that cache can mask a probe diagnostic +under `--noEmit`. I did not test it. If the two arms disagree in a way that does not +track the probe, delete that file and re-run before reporting anything. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md new file mode 100644 index 00000000..74766d18 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md @@ -0,0 +1,140 @@ +--- +repo: metamask-mobile +parent: tsc-blindspots +--- + +# Blind spots — metamask-mobile + +**Scope of this file.** Written from measured `tsconfig.json` and `.eslintrc.js` +values. The repo was not checked out on the machine this was written on, so there +are **no verified call sites below** — every statement about actual code is an open +question with the command that settles it. Add examples on the first real review; +the extension and core overlays show the form. + +## Strictness is local — the extension's audit trap does not apply + +`strict: true` is set in `tsconfig.json` itself, not inherited. A reader auditing +that file sees it, which is the opposite of extension, where `strict` arrives +through `@tsconfig/node22` and is invisible locally. + +Run `--showConfig` anyway, for one reason: **`noUncheckedIndexedAccess` is not part +of `strict`** and is not set here. + +```bash +npx tsc -p tsconfig.json --showConfig +``` + +## Unchecked indexing + +`record[key]` and `arr[i]` yield `T`, never `T | undefined`, everywhere the root +`tsconfig.json` governs. Under `strict: true` this is the one nullability hole left +open, and it is the one that reaches runtime — as a property access on `undefined`, +not as a type error. + +Where to look, as commands rather than claims. The source root is not asserted here; +take `` from the tsconfig's own `include`: + +```bash +python3 -c "import json,re,sys; print(json.loads(re.sub(r'//.*','',open('tsconfig.json').read()))['include'])" +``` + +Then find an index expression dereferenced immediately, and a hand-written runtime +guard on an index expression sitting beside one that has none: + +```bash +grep -rnE '\]\.[a-zA-Z]' --include=*.ts --include=*.tsx | grep -v '\.test\.' +grep -rnE '\?\.\[|\]\?\.' --include=*.ts --include=*.tsx | grep -v '\.test\.' +``` + +A guard the compiler did not ask for is evidence the author knew the lookup could +miss; the sibling index without one is the finding. Both core and extension carry +that exact pair on `networkConfigurationsByChainId[chainId]` followed by +`rpcEndpoints[defaultRpcEndpointIndex]`. **Open question:** whether mobile consumes +the same `NetworkController` state and whether that expression appears here — I did +not verify either. Settle both with `grep -rn "rpcEndpoints\[" --include=*.ts` +and a look at the `@metamask/network-controller` dependency in `package.json`. + +## No DOM lib — this changes how probes are written, not just what compiles + +`lib: ["es2022"]`, with `jsx: react-native`. Nothing from `lib.dom.d.ts` is in +scope from `lib`, so a probe borrowing a DOM type fails for a reason that has +nothing to do with the claim under test — the parent skill's Step 5 failure, and +here it is the default outcome of copying a probe from an extension review. + +`document`, `window`, `HTMLElement` and `Event` come from `lib.dom.d.ts` and are +therefore not supplied. `fetch`, `URL`, `AbortController` and `console` are also +`lib.dom.d.ts` members but are commonly re-declared by React Native's own type +packages or by `@types/node`. **Open question:** which of those resolve in this +repo. Settle each in one line — write it in a scratch file inside the `include` +paths and run `tsc`: + +```ts +type _Probe = HTMLElement; // TS2304 if absent +type _Probe2 = typeof fetch; // TS2304 if absent +``` + +Do the same before using any DOM type in a real probe. Do not infer it from the +extension or core overlays; core has `DOM` in `lib` and extension has `DOM` plus +`es2023`, so both compile things that will not compile here. + +## The JS boundary — live, with one input unmeasured + +`allowJs: true` puts `.js` files in the program. Whether they are **checked** +depends on `checkJs`, which was not among the measured values. The two are +different questions and only the second decides whether a type written for a JS +module is validated against anything. + +**Open question, and it gates the whole class.** Settle it before writing the +review: + +```bash +npx tsc -p tsconfig.json --showConfig | grep -i checkjs # absent => off +grep -rl '@ts-check' --include=*.js | wc -l # per-file opt-ins +``` + +If `checkJs` is off — the default, and what extension does — then a hand-written +type for a function whose callers are all `.js` is checked against nothing and can +drift indefinitely. Size the boundary before weighting the finding, the way the +extension overlay does: + +```bash +find -name '*.js' | wc -l +find \( -name '*.ts' -o -name '*.tsx' \) | wc -l +``` + +## The lint layer + +`.eslintrc.js:170` sets `@typescript-eslint/no-explicit-any` to `'error'`. As in +extension, that means **you cannot find an absorbed `any` by grepping for `any`** — +writing one is a lint error, so the pressure goes into `as` instead. Use `IsAny` at +the call sites (parent §7), and read escape-hatch clusters as the search index. + +`.eslintrc.js:206` turns off `@typescript-eslint/no-unsafe-enum-comparison`. That is +the lint counterpart of the parent's divergence shape 1 — a field widened from an +enum to `string` and then compared against an enum member is green in **both** `tsc` +and lint. Lint silence is not a clearance for that shape here. + +**Open question:** whether `no-floating-promises` and `no-unsafe-function-type` are +on. Extension disables both; mobile was measured only for +`no-unsafe-enum-comparison`, so their state here is unknown rather than on. Settle +with `grep -n "no-floating-promises\|no-unsafe-function-type" .eslintrc.js` and, if +absent, by checking the shared config the file extends. + +## Probe note + +`module: commonjs` and `jsx: react-native`, against `Node16` in both extension and +core. A probe is **not** portable from those repos without rewriting its imports. +`isolatedModules: true` additionally requires `import type` / `export type` for +type-only positions — a probe that re-exports a type without `type` errors for the +wrong reason, which is the parent's Step 5 again. + +## Open questions, collected + +| Question | Settles it | +|---|---| +| Is `checkJs` on? | `npx tsc -p tsconfig.json --showConfig \| grep -i checkjs` | +| How big is the `.js` surface? | `find -name '*.js' \| wc -l` | +| Which DOM-named globals resolve? | one-line `type _P = X;` probe per global | +| Does the `record[k]` then `arr[i]` pair appear? | `grep -rn "rpcEndpoints\[" --include=*.ts` | +| Are `no-floating-promises` / `no-unsafe-function-type` on? | `grep -n` in `.eslintrc.js`, then the shared config | +| Are there per-directory tsconfigs overriding these? | `find . -name 'tsconfig*.json' -not -path '*/node_modules/*'` | From 15407c0583318b12d212cc27fcb451e7ba94201c Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 31 Aug 2026 09:02:05 -0400 Subject: [PATCH 16/20] Re-verify `tsc-blindspots` overlays against `origin/main` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every count and line number in the three overlays was read from a local checkout that was behind `origin/main`, so the figures were stale: core reported 75 packages and 1,380 `.ts` files against an actual 98 and 2,052. Adds the two findings the re-read surfaced: `tsconfig.lint.json` declares its own 14 `references` rather than inheriting the root config's 98, so `lint:tsc` covers 14 of 98 packages; and extension's `no-explicit-any` block scopes to the parsed tsconfig program, so the 300 `.stories.ts(x)` excluded from `tsconfig.json` lose the lint rule and `tsc` together. Drops the note in `references/metamask-extension.md` explaining that the notes live outside `repos/` to avoid skipping mobile and core installs — the skill now ships all three overlays, so the reason is spent. --- .../references/metamask-extension.md | 7 +- .../skills/tsc-blindspots/repos/core.md | 156 +++++++++------ .../repos/metamask-extension.md | 126 +++++++++--- .../tsc-blindspots/repos/metamask-mobile.md | 186 ++++++++++-------- 4 files changed, 307 insertions(+), 168 deletions(-) diff --git a/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md index 36a67496..dd354cf5 100644 --- a/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md +++ b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md @@ -1,8 +1,9 @@ # Repo notes — metamask-extension -Specifics for running the two-arm proof in `MetaMask/metamask-extension`. (Kept -here rather than in `repos/` on purpose: a `repos/` subdir containing only an -extension overlay would make this skill *skip* installs for mobile and core.) +Specifics for running the two-arm proof in `MetaMask/metamask-extension`. The +split against `repos/metamask-extension.md` is by kind rather than by repo: which +blind spots bite this repo, and where they live, go in the overlay; run mechanics +— heap size, probe placement, timings — stay here. ## Typecheck invocation diff --git a/domains/typescript/skills/tsc-blindspots/repos/core.md b/domains/typescript/skills/tsc-blindspots/repos/core.md index 8b768208..1374ba68 100644 --- a/domains/typescript/skills/tsc-blindspots/repos/core.md +++ b/domains/typescript/skills/tsc-blindspots/repos/core.md @@ -5,24 +5,60 @@ parent: tsc-blindspots # Blind spots — core -What `MetaMask/core`'s configuration does **not** check. Two of the parent skill's -classes do not exist here and one exists only here, so do not carry the extension +What `MetaMask/core`'s configuration does **not** check. One of the parent skill's +classes does not exist here and two exist only here, so do not carry the extension or mobile playbook across unchanged. +Every line number and count below was read from `origin/main` +(`git show origin/main:`), not from a working tree. This matters more here +than in the other two: a local branch a few hundred commits behind reports a +different package count and different reference lists. + ## The JS boundary does not exist here — say so, do not omit it `tsconfig.base.json` does not set `allowJs`, and it is absent from the resolved options for a package (`npx tsc -p packages/assets-controllers/tsconfig.json --showConfig`). Independently, `packages/*/src` contains **0** `.js` files against -**1,380** `.ts`. +**2,052** `.ts`. So the parent skill's central premise — *"across a JavaScript boundary (`checkJs` off) it checks nothing at all"* — has no instance in core. A review that reports "the callers are still `.js`, so this type is validated against nothing" is wrong here, and a `checkJs` finding copied from an extension review does not transfer. -State this explicitly in a core review rather than leaving it out. Its absence is -what makes the two classes below the ones worth spending the time on. +State this explicitly in a core review rather than leaving it out. + +## The repo-wide typecheck covers 14 of 98 packages + +The largest blind spot here, and it is not a compiler setting — it is which files +the compiler is pointed at. + +`lint` begins with `lint:tsc`, which is `tsc --build tsconfig.lint.json`, and CI +runs it as a matrix entry in `.github/workflows/lint-build-test.yml:91`. But +`tsconfig.lint.json` declares its own `references` — **14 entries** — and that is +the whole graph: `references` is not inherited through `extends`, so extending +`tsconfig.json` (99 entries, 98 unique) does not widen it. Checked with a two-config +fixture and `--showConfig`: a child declaring one reference over a base declaring +two resolves to **one**, and a child declaring none resolves to **no `references` +key at all** — overridden, never merged. Independently, exactly **14** of the **98** +packages have a `tsconfig.lint.json` for it to point at. The config says so itself: *"This configuration +incrementally enables repository-wide type checking."* + +The 14: `announcement-controller`, `app-metadata-controller`, `base-controller`, +`build-utils`, `client-controller`, `foundryup`, `local-node-utils`, `messenger`, +`messenger-cli`, `platform-api-docs`, `preferences-controller`, +`rate-limit-controller`, `stellar-quickstart-up`, `storage-service`. + +The other 84 are typechecked by the **Build** job instead +(`lint-build-test.yml:194-231`), which runs `ts-bridge` over a tsconfig generated +from the changed packages (`scripts/generate-partial-build-tsconfig.mts`), falling +back to `yarn build` over all 98. Different entry point, different config chain, +different scope per PR. + +**So "core is green" is a claim about one of two graphs, and neither is the whole +repo on every run.** Name the command you ran. Before reporting that a probe was +silent, confirm the package you probed is in the graph you invoked — a probe in one +of the 84 is not covered by `lint:tsc` at all. ## Unchecked indexing — and it is not uniform across packages @@ -35,7 +71,7 @@ what makes the two classes below the ones worth spending the time on. | `packages/eth-json-rpc-provider/tsconfig.json` | :9 | | `tsconfig.scripts.json` | :19 | -That is **2 of the 75 package tsconfigs**, plus the scripts config. Read the +That is **2 of the 98 package tsconfigs**, plus the scripts config. Read the package's own file before concluding an index expression is unchecked — the answer differs by directory, which no other MetaMask repo here requires you to check. @@ -43,19 +79,20 @@ The same expression appears both guarded and unguarded in two files of one packa `packages/assets-controllers`: ```ts -// packages/assets-controllers/src/TokenBalancesController.ts:510-512 (also :526-528) +// packages/assets-controllers/src/TokenBalancesController.ts:590-592 (also :606-608) const networkConfig = networkConfigurationsByChainId[chainId]; const { networkClientId } = networkConfig.rpcEndpoints[networkConfig.defaultRpcEndpointIndex]; -// packages/assets-controllers/src/AccountTrackerController.ts:590-596 -.map((hexChainId) => { - const networkConfig = networkConfigurationsByChainId[hexChainId]; - return networkConfig?.rpcEndpoints[ - networkConfig.defaultRpcEndpointIndex - ]?.networkClientId; -}) -.filter((id): id is NetworkClientId => id !== undefined); +// packages/assets-controllers/src/AccountTrackerController.ts:629-636 +return popularEvmChainIds + .map((hexChainId) => { + const networkConfig = networkConfigurationsByChainId[hexChainId]; + return networkConfig?.rpcEndpoints[ + networkConfig.defaultRpcEndpointIndex + ]?.networkClientId; + }) + .filter((id): id is NetworkClientId => id !== undefined); ``` Two index operations each, both typed non-nullable under `strict: true`. The second @@ -106,31 +143,41 @@ const mapIsAny: IsAny = true; assignable to type 'false'`) — the shipped declarations resolve and `abiERC20` is **not** `any`. +The probe ran against an older checkout, but both inputs match `main`: the shim file +is byte-identical, and `main` declares `@metamask/metamask-eth-abis@^3.1.1` against +the probed 3.1.1 with `types: dist/index.d.ts`. + The shim is what makes it `any`, at these importers under `packages/assets-controllers/src/` — `Standards/ERC20Standard.ts:6`, -`TokensController.ts:31`, `Standards/NftStandards/ERC1155/ERC1155Standard.ts:11`. -This is parent §7 false precision with a source you can delete: remove the shim. +`TokensController.ts:31`, `Standards/NftStandards/ERC1155/ERC1155Standard.ts:11`, +`Standards/NftStandards/ERC721/ERC721Standard.ts:11` — and outside that package at +`packages/bridge-controller/src/bridge-controller.ts:8` and +`packages/bridge-controller/src/utils/balance.ts:5`. This is parent §7 false +precision with a source you can delete: remove the shim. **2. The shim stands in for a package with no types.** `@metamask/contract-metadata` has no `types`/`typings` field; with `types/` removed the import is `TS7016`. Hand-writing is the parent's Step 2 case 7 — correct in principle. The defect is that the shim asserts `any` rather than a shape, so -`contractMap` is `any` at `packages/assets-controllers/src/TokensController.ts:16` -and `.../TokenDetectionController.ts:10`. `single-call-balance-checker-abi` is the -same case, at `.../AssetsContractController.ts:19`. - -`@typescript-eslint/no-explicit-any` is `'error'` in `eslint.config.mjs`, in the -block commented *"Enable rules that are disabled in -`@metamask/eslint-config-typescript`"* — and it cannot see either case, because no -`any` is written anywhere. Neither can `skipLibCheck: false` (below): a bodiless -`declare module` is well-formed, so checking declarations finds nothing wrong with -it. +`contractMap` is `any` at `packages/assets-controllers/src/TokensController.ts:16`, +`.../TokenDetectionController.ts:10` and +`packages/client-utils/src/mappers/helpers/token-metadata.ts:1`. +`single-call-balance-checker-abi` is the same case, at +`.../AssetsContractController.ts:19`. + +Two gates that cannot see either case. `@typescript-eslint/no-explicit-any` is +`'error'` in `eslint.config.mjs`, under the comment *"Enable rules that are disabled +in `@metamask/eslint-config-typescript`"* — and no `any` is written anywhere, so it +has nothing to match. (That re-enable is not distinctive: extension and mobile do +the same, over the same shared default.) And `skipLibCheck` does not help either +way — a bodiless `declare module` is well-formed, so checking declarations finds +nothing wrong with it. ## Project references — two entry points that resolve differently `composite: true` in `tsconfig.base.json`. Every package tsconfig lists its dependencies under `references`, and every `tsconfig.build.json` references the -other packages' `tsconfig.build.json`. This is the class the other two repos do not +other packages' `tsconfig.build.json`. This is a class the other two repos do not have, and the first thing to get right is which config you ran. **The `paths` mapping is not inherited by every entry point.** @@ -154,42 +201,26 @@ type: ```bash npx tsc -p packages//tsconfig.json --explainFiles | grep -i '' -npx tsc --build tsconfig.build.json --verbose --traceResolution 2>&1 | grep -i '' +npx tsc --build tsconfig.lint.json --verbose --traceResolution 2>&1 | grep -i '' ``` -**Four packages are in the build graph and absent from the root config.** -`tsconfig.json` lists **71** references; `tsconfig.build.json` lists **75**. The -difference is `eip-5792-middleware`, `eip-7702-internal-rpc-middleware`, -`logging-controller` and `storage-service` — reachable transitively through other -packages' `references`, but not named at the root. - -**And no script runs the root config.** The only `tsc` invocation in `package.json` -is `build:types` (`tsc --build tsconfig.build.json --verbose`); `build` is -`ts-bridge --project tsconfig.build.json`. `tsconfig.json`'s own comment says it is -*"used by the `lint` script in `package.json`, and by editors such as VSCode"*, and -`lint` runs eslint, prettier, constraints, depcheck and two scripts — no `tsc`. -**Do not cite "core typechecks clean" without naming the command you ran**, and do -not assume CI ran the config you are reading. - -That is not the same as "types are never checked": eslint's type-aware rules build -a program through the parser (`eslint.config.mjs` sets `parserOptions.tsconfigRootDir`; -the `project` setting comes from `@metamask/eslint-config-typescript`, which I did -not read). Type information is loaded — but a rule set is not `tsc` reporting every -diagnostic, and a probe's `TS2322` has nothing there to surface it. - **Open question:** whether `tsc --build` here can report clean over an out-of-date -`dist/*.d.ts`. `build:types` already passes `--verbose`, which prints the -up-to-date decision per project — read that log rather than the exit code. - -What holds regardless of the resolution question: **consumers outside the repo — -extension and mobile — read `dist/*.d.ts`**, and no check inside core exercises -that path from a consumer's position. A type that is correct against `src` and -stale in `dist` is invisible here and breaks there. - -`skipLibCheck` is set in `tsconfig.packages.build.json` and **not** in -`tsconfig.packages.json`, so a per-package typecheck checks declaration files that -the build skips — the opposite of extension, where `skipLibCheck: true` is -inherited repo-wide. +`dist/*.d.ts`. Both `build:types` and `lint:tsc` use `--build`, and `build:types` +already passes `--verbose`, which prints the up-to-date decision per project — read +that log rather than the exit code. + +What holds regardless of the resolution question: **consumers outside the repo read +`dist/*.d.ts`**, and no check inside core exercises that path from a consumer's +position. Mobile goes further and hardcodes paths *into* that `dist/` in its own +`tsconfig.json` (see the mobile overlay). A type correct against `src` and stale in +`dist` is invisible here and lands there. + +**Every typecheck that actually runs sets `skipLibCheck`.** Both +`tsconfig.packages.lint.json` and `tsconfig.packages.build.json` set it `true`, as +does the root `tsconfig.lint.json`. The per-package `tsconfig.json` — the one an +editor picks up for a file under `packages/*/src` — does not. So declarations are +checked in the editor and skipped by both CI paths, which is the wrong way round +for catching a bad `.d.ts`. ## Probe note @@ -197,3 +228,6 @@ inherited repo-wide. mobile. `module` and `moduleResolution` are `Node16`, matching extension and not mobile — a probe is portable between core and extension, and is not portable to mobile without rewriting its imports. + +Put the probe in a package that is in the graph you are running. In one of the 84 +packages outside `tsconfig.lint.json`, `yarn lint:tsc` will not read it. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md index 95f113ed..00cdcef0 100644 --- a/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md @@ -10,6 +10,9 @@ parent skill's five divergence shapes. How to *run* the two-arm proof here — h probe location, the authoritative-source table, the `chrome.*` caveat — is [references/metamask-extension.md](../references/metamask-extension.md). +Every line number and count below was read from `origin/main` +(`git show origin/main:`), not from a working tree. + ## `tsconfig.json` does not show you the strictness The local file names exactly one strictness flag, `useUnknownInCatchVariables`. @@ -30,26 +33,73 @@ That prints `strict`, `strictNullChecks`, `strictFunctionTypes`, `noImplicitAny` wouldn't have caught it anyway." It would: `strictNullChecks` is on, so a dropped `| undefined` (parent shape 2) *does* surface in a probe here. -`noUncheckedIndexedAccess` is genuinely absent — checked across all three -tsconfigs in the repo (`tsconfig.json`, `development/webpack/tsconfig.webpack.json`, -`test/e2e/playwright/llm-workflow/tsconfig.json`), none of which set it. +`noUncheckedIndexedAccess` is genuinely absent — the repo has three tsconfigs +(`tsconfig.json`, `development/webpack/tsconfig.webpack.json`, +`test/e2e/playwright/llm-workflow/tsconfig.json`) and none sets it. + +## 300 Storybook files are outside `tsc` *and* outside the lint rule + +The strongest blind spot in this repo, because it removes both checks at once and +neither absence is visible from the file you are reviewing. + +`tsconfig.json` excludes them, with the reason stated: + +```jsonc +"exclude": [ + // don't typecheck stories, as they don't yet pass the type checker. + "**/*.stories.tsx", + "**/*.stories.ts" +], +``` + +**300** `.stories.ts`/`.stories.tsx` files on `main`, all under `include` paths. + +The lint half follows from how the config is built. `.eslintrc.js:26-28` parses the +tsconfig with the TypeScript API — + +```js +const tsconfigPath = ts.findConfigFile('./', ts.sys.fileExists); +const { config } = ts.readConfigFile(tsconfigPath, ts.sys.readFile); +const tsconfig = ts.parseJsonConfigFileContent(config, ts.sys, './'); +``` + +— and `.eslintrc.js:148` scopes the block carrying `no-explicit-any` to +`files: tsconfig.fileNames.filter((f) => /\.tsx?$/u.test(f))`. **The rule's scope +is the tsconfig program.** A file excluded from the tsconfig is excluded from the +rule by construction. + +The repo documents the consequence at `.eslintrc.js:687-688`, in the comment above +the `**/*.stories.js` override: *"This block is for overriding settings from the +base config. It's JavaScript-only because the Storybook TypeScript files don't have +the base config applied."* The one block that does match `.stories.ts(x)` +(`.eslintrc.js:674-683`) sets only `color-no-hex` and `storybook/no-redundant-story-name` +— it does not restore anything. + +So in a `.stories.tsx` file: no `tsc`, no `no-explicit-any`, and no +`no-unsafe-*` (those are off repo-wide anyway, below). A hand-written type there is +unfalsifiable in the parent skill's Step 1 sense, and an `any` there is invisible to +every gate. Note the asymmetry — the 156 `.stories.js` files **do** get the base +config; only the TypeScript ones lost it. + +**For review:** a migration PR that adds or edits a `.stories.tsx` has had none of +its types checked. Say so rather than treating a green CI as coverage. ## The JS boundary, sized `allowJs: true` and `checkJs` unset. The parent reference explains why -`app/scripts/background.js` matters; the number is the part worth knowing: +`app/scripts/background.js` matters; the number is the part worth knowing. Counts +under `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) on `main`: -| Under `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) | Count | +| | Count | |---|---| -| `.js` | 1,219 | -| `.ts` / `.tsx` | 7,245 | +| `.js` | 1,182 | +| `.ts` / `.tsx` | 7,374 (of which **300** are `.stories.ts(x)`, excluded above) | | `.js` carrying `@ts-check` | **1** (`development/lib/build-type.js`) | Roughly one included file in seven asserts nothing and is checked against nothing. A type hand-written for a function whose callers are all in that seventh is -unfalsifiable in -the parent skill's Step 1 sense — probe it, but expect Arm B to stay silent, and -report that as *unconstrained* rather than as *cleared*. +unfalsifiable — probe it, but expect Arm B to stay silent, and report that as +*unconstrained* rather than as *cleared*. ## Unchecked indexing, in one function @@ -88,7 +138,7 @@ none. `shared/lib/selectors/networks.ts:88` is the same shape with the cast form ## `skipLibCheck: true` — declarations are not checked -Inherited from the base. Every `.d.ts` is exempt: the twelve files in `types/` and +Inherited from the base. Every `.d.ts` is exempt: the eleven files in `types/` and every dependency's declarations. `types/lavamoat__lavadome-core.d.ts` is a bodiless `declare module @@ -100,30 +150,51 @@ asserts `any` where it could assert a shape. I found no `.ts`/`.tsx` importer un ## The lint layer changes what you can grep for -`.eslintrc.js:162` sets `@typescript-eslint/no-explicit-any` to `'error'`, inside the -override at `.eslintrc.js:148` scoped to `tsconfig.fileNames` filtered to `.tsx?` — -so it covers exactly the files `tsc` checks. +`.eslintrc.js:162` sets `@typescript-eslint/no-explicit-any` to `'error'`, in the +override at `.eslintrc.js:148` scoped to the tsconfig program. + +**That is a re-enable, not a repo quirk.** The shared config +`@metamask/eslint-config-typescript` sets `no-explicit-any` to `'off'`, and all +three repos here turn it back on locally — extension at `.eslintrc.js:162`, mobile +at `.eslintrc.js:170`, core in `eslint.config.mjs` under the comment *"Enable rules +that are disabled in `@metamask/eslint-config-typescript`"*. Do not build a contrast +out of it. **Consequence for parent §7 (false precision):** you cannot find an absorbed `any` by grepping for `any`, because writing one is a lint error. The rule pushes authors to `as` instead, which is why the parent's "escape hatches are the tell" section is the productive search here. Use `IsAny` at the call sites, not a grep. -`.eslintrc.js:278-296` disables a block of rules, commented *"removing changes to -our shared ESLint config made after version v9 … TODO: Remove these modifications -after the ESLint v9 update"*. Three of them matter to this skill: +### The five `no-unsafe-*` rules are off in all three repos -| Rule | Line | What stops being reported | +The shared config also sets `no-unsafe-argument`, `no-unsafe-assignment`, +`no-unsafe-call`, `no-unsafe-member-access` and `no-unsafe-return` to `'off'`, +across the majors each repo resolves (13.0.0 mobile, 14.1.1 extension, 15.0.0 core). + +Those are precisely the rules that report an `any` *flowing into* a typed position — +the thing `no-explicit-any` cannot see because no `any` was written. So the +repo-wide posture is: writing `any` is an error, and letting one arrive and spread +is unreported. That is the parent skill's §7 in configuration form, and it is why +this skill has work to do in all three repos. + +### Locally disabled rules + +`.eslintrc.js:278-296` disables a block, commented *"removing changes to our shared +ESLint config made after version v9 … TODO: Remove these modifications after the +ESLint v9 update"* — temporary, so a finding these would have caught is not a +decision to accept risk. + +| Rule | Line | Note | |---|---|---| -| `no-floating-promises` | :284 | an unawaited promise introduced while annotating | -| `no-unsafe-enum-comparison` | :289 | parent shape 1 — `string` typed where an enum belongs, then compared to an enum member | -| `no-unsafe-function-type` | :291 | a bare `Function` standing in for a call signature | +| `no-floating-promises` | :284 | also off in mobile (`.eslintrc.js:199`) — shared posture, not local | +| `no-unsafe-enum-comparison` | :289 | **redundant** — the shared config already disables it in every resolved major | +| `no-unsafe-function-type` | :291 | the genuinely local disable | -`no-unsafe-enum-comparison` is the lint counterpart of the parent's first divergence -shape. With it off, a migration that widens an enum-valued field to `string` and -compares it to an enum member is green in **both** `tsc` and lint — so lint silence -is not a clearance here. These are marked temporary, so a finding they would have -caught is not a decision to accept the risk; say so when reporting one. +`no-unsafe-enum-comparison` being off — whether locally or via the shared config — +is what matters for this skill: it is the lint counterpart of the parent's first +divergence shape, so a migration that widens an enum-valued field to `string` and +compares it to an enum member is green in **both** `tsc` and lint. Lint silence is +not a clearance for that shape. ## Probe note @@ -132,3 +203,6 @@ node_modules/.cache/typescript/tsconfig.tsbuildinfo` means a cache persists betw Arm A and Arm B. **Open question:** whether that cache can mask a probe diagnostic under `--noEmit`. I did not test it. If the two arms disagree in a way that does not track the probe, delete that file and re-run before reporting anything. + +A probe file must be inside `include` **and** not match `**/*.stories.ts(x)`, or it +is silently outside the program. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md index 74766d18..84a5bf44 100644 --- a/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md @@ -5,11 +5,12 @@ parent: tsc-blindspots # Blind spots — metamask-mobile -**Scope of this file.** Written from measured `tsconfig.json` and `.eslintrc.js` -values. The repo was not checked out on the machine this was written on, so there -are **no verified call sites below** — every statement about actual code is an open -question with the command that settles it. Add examples on the first real review; -the extension and core overlays show the form. +**Scope of this file.** `tsconfig.json`, `.eslintrc.js` and `package.json` were read +from `main` via `gh api repos/MetaMask/metamask-mobile/contents/`, so every +config claim below is verified. The repo is **not checked out**, so there are **no +verified call sites** — statements about actual code are open questions with the +command that settles them. Add examples on the first real review; the extension and +core overlays show the form. ## Strictness is local — the extension's audit trap does not apply @@ -17,42 +18,62 @@ the extension and core overlays show the form. that file sees it, which is the opposite of extension, where `strict` arrives through `@tsconfig/node22` and is invisible locally. -Run `--showConfig` anyway, for one reason: **`noUncheckedIndexedAccess` is not part -of `strict`** and is not set here. +Two flags that matter here are still not part of `strict`: -```bash -npx tsc -p tsconfig.json --showConfig -``` +- **`noUncheckedIndexedAccess` is not set.** Next section. +- **`skipLibCheck: true` is set**, so no `.d.ts` is checked — the same posture as + extension, and the same as every typecheck core actually runs. ## Unchecked indexing -`record[key]` and `arr[i]` yield `T`, never `T | undefined`, everywhere the root -`tsconfig.json` governs. Under `strict: true` this is the one nullability hole left -open, and it is the one that reaches runtime — as a property access on `undefined`, -not as a type error. - -Where to look, as commands rather than claims. The source root is not asserted here; -take `` from the tsconfig's own `include`: - -```bash -python3 -c "import json,re,sys; print(json.loads(re.sub(r'//.*','',open('tsconfig.json').read()))['include'])" -``` +`record[key]` and `arr[i]` yield `T`, never `T | undefined`. Under `strict: true` +this is the one nullability hole left open, and it is the one that reaches runtime — +as a property access on `undefined`, not as a type error. -Then find an index expression dereferenced immediately, and a hand-written runtime -guard on an index expression sitting beside one that has none: +The source roots are `app/`, `tests/` and `scripts/` (from `include`; `app/**/*` is +the bulk). Find an index expression dereferenced immediately, and a hand-written +runtime guard on an index expression sitting beside one that has none: ```bash -grep -rnE '\]\.[a-zA-Z]' --include=*.ts --include=*.tsx | grep -v '\.test\.' -grep -rnE '\?\.\[|\]\?\.' --include=*.ts --include=*.tsx | grep -v '\.test\.' +grep -rnE '\]\.[a-zA-Z]' app --include=*.ts --include=*.tsx | grep -v '\.test\.' +grep -rnE '\?\.\[|\]\?\.' app --include=*.ts --include=*.tsx | grep -v '\.test\.' ``` A guard the compiler did not ask for is evidence the author knew the lookup could miss; the sibling index without one is the finding. Both core and extension carry that exact pair on `networkConfigurationsByChainId[chainId]` followed by -`rpcEndpoints[defaultRpcEndpointIndex]`. **Open question:** whether mobile consumes -the same `NetworkController` state and whether that expression appears here — I did -not verify either. Settle both with `grep -rn "rpcEndpoints\[" --include=*.ts` -and a look at the `@metamask/network-controller` dependency in `package.json`. +`rpcEndpoints[defaultRpcEndpointIndex]`. **Open question:** whether it appears here. +Settle with `grep -rn "rpcEndpoints\[" app --include=*.ts`. + +## Hand-written `paths` into dependency `dist/` — a class the other two lack + +`tsconfig.json` maps roughly twenty subpath imports directly at declaration files +inside `node_modules`, with the reason stated: *"TODO: Remove these once we use +`Node16` module resolution."* `moduleResolution` is `node`, which cannot follow the +`exports` subpaths these packages publish, so the paths are written out by hand: + +```jsonc +"@metamask/json-rpc-engine/v2": ["node_modules/@metamask/json-rpc-engine/dist/v2/index.d.cts"], +"@metamask/keyring-api/v2": ["node_modules/@metamask/keyring-api/dist/v2/index.d.cts"], +"@metamask/perps-controller/types": ["node_modules/@metamask/perps-controller/dist/types/index.d.cts"], +``` + +Each is a hand-written claim about another package's build output — the same class +the parent skill is built around, one level up from a type to a file path. Two ways +it fails silently: + +1. **The path stops resolving.** A dependency reorganises `dist/`, and the mapping + points at nothing. Whether that surfaces as an error or as a fallback to normal + resolution is worth knowing before you trust a green run. +2. **The path resolves to a stale or wrong declaration.** These reach *into* core's + emitted `dist/`, which core's own checks do not exercise from a consumer's + position (see the core overlay). A type correct against core's `src` and stale in + its `dist` is invisible in core and lands here. + +**For review:** a PR touching one of these packages' subpath exports should be +checked against this list. **Open question:** whether any mapped path is currently +dead — settle with a loop over the `paths` values testing each file exists, then +`npx tsc -p tsconfig.json --traceResolution` for one of them. ## No DOM lib — this changes how probes are written, not just what compiles @@ -61,12 +82,11 @@ scope from `lib`, so a probe borrowing a DOM type fails for a reason that has nothing to do with the claim under test — the parent skill's Step 5 failure, and here it is the default outcome of copying a probe from an extension review. -`document`, `window`, `HTMLElement` and `Event` come from `lib.dom.d.ts` and are -therefore not supplied. `fetch`, `URL`, `AbortController` and `console` are also -`lib.dom.d.ts` members but are commonly re-declared by React Native's own type -packages or by `@types/node`. **Open question:** which of those resolve in this -repo. Settle each in one line — write it in a scratch file inside the `include` -paths and run `tsc`: +`typeRoots` is unset, so every `@types/*` package is auto-included, and +`@types/node@^24` is a devDependency. Node-declared globals therefore have a +source; DOM-only ones (`document`, `window`, `HTMLElement`, `Event`) do not. +**Open question:** which specific globals resolve. Settle each in one line — a +scratch file inside the `include` paths, then run `tsc`: ```ts type _Probe = HTMLElement; // TS2304 if absent @@ -74,67 +94,77 @@ type _Probe2 = typeof fetch; // TS2304 if absent ``` Do the same before using any DOM type in a real probe. Do not infer it from the -extension or core overlays; core has `DOM` in `lib` and extension has `DOM` plus -`es2023`, so both compile things that will not compile here. - -## The JS boundary — live, with one input unmeasured +other two overlays; core has `DOM` in `lib` and extension has `DOM` plus `es2023`. -`allowJs: true` puts `.js` files in the program. Whether they are **checked** -depends on `checkJs`, which was not among the measured values. The two are -different questions and only the second decides whether a type written for a JS -module is validated against anything. +## The JS boundary is live -**Open question, and it gates the whole class.** Settle it before writing the -review: +`allowJs: true` and **`checkJs` is not set**, so `.js` files are in the program and +checked against nothing — the same posture as extension. A hand-written type for a +function whose callers are all `.js` can drift indefinitely. -```bash -npx tsc -p tsconfig.json --showConfig | grep -i checkjs # absent => off -grep -rl '@ts-check' --include=*.js | wc -l # per-file opt-ins -``` +`app/core/InpageBridgeWeb3.js` and `scripts/inpage-bridge/dist` are explicitly +excluded, so they are not even in the program. -If `checkJs` is off — the default, and what extension does — then a hand-written -type for a function whose callers are all `.js` is checked against nothing and can -drift indefinitely. Size the boundary before weighting the finding, the way the -extension overlay does: +Size the boundary before weighting a finding, the way the extension overlay does: ```bash -find -name '*.js' | wc -l -find \( -name '*.ts' -o -name '*.tsx' \) | wc -l +find app -name '*.js' | wc -l +find app \( -name '*.ts' -o -name '*.tsx' \) | wc -l +grep -rl '@ts-check' app --include=*.js | wc -l ``` ## The lint layer -`.eslintrc.js:170` sets `@typescript-eslint/no-explicit-any` to `'error'`. As in -extension, that means **you cannot find an absorbed `any` by grepping for `any`** — -writing one is a lint error, so the pressure goes into `as` instead. Use `IsAny` at -the call sites (parent §7), and read escape-hatch clusters as the search index. +`lint:tsc` is `tsc --project ./tsconfig.json` with a 12 GB heap — one whole-program +typecheck, unlike core, where the equivalent script covers a fraction of the repo. +`lint` runs `eslint '**/*.{js,ts,tsx}'` **by glob**, not by tsconfig program, so +mobile does not have extension's excluded-files-lose-the-rule problem. + +`.eslintrc.js:170` sets `@typescript-eslint/no-explicit-any` to `'error'` in the +`*.{ts,tsx}` block at `:155`. **That is a re-enable, not a repo quirk** — the shared +`@metamask/eslint-config-typescript` sets it `'off'`, and all three repos turn it +back on. Consequence: you cannot find an absorbed `any` by grepping for `any`. Use +`IsAny` at the call sites (parent §7), and read escape-hatch clusters as the index. + +The same shared config sets all five `no-unsafe-*` rules (`-argument`, +`-assignment`, `-call`, `-member-access`, `-return`) to `'off'` at major 13.0.0, +which mobile resolves. Those are the rules that report an `any` *flowing into* a +typed position — so writing `any` is an error here and letting one arrive and spread +is unreported. + +The `*.{ts,tsx}` block disables three rules that matter to this skill: + +| Rule | Line | +|---|---| +| `no-floating-promises` | :199 | +| `no-unsafe-enum-comparison` | :206 | +| `restrict-template-expressions` | :221 | -`.eslintrc.js:206` turns off `@typescript-eslint/no-unsafe-enum-comparison`. That is -the lint counterpart of the parent's divergence shape 1 — a field widened from an -enum to `string` and then compared against an enum member is green in **both** `tsc` -and lint. Lint silence is not a clearance for that shape here. +`no-unsafe-enum-comparison` is the lint counterpart of the parent's divergence +shape 1 — a field widened from an enum to `string` and compared against an enum +member is green in **both** `tsc` and lint. Lint silence is not a clearance there. -**Open question:** whether `no-floating-promises` and `no-unsafe-function-type` are -on. Extension disables both; mobile was measured only for -`no-unsafe-enum-comparison`, so their state here is unknown rather than on. Settle -with `grep -n "no-floating-promises\|no-unsafe-function-type" .eslintrc.js` and, if -absent, by checking the shared config the file extends. +Two of the three are re-enabled in a narrow override: `.eslintrc.js:471` scopes +`files: ['app/**/*-method-action-types*.ts']`, and that block sets +`no-floating-promises` and `restrict-template-expressions` to `'error'` at +`:628-629`. That is generated-file territory, not a repo-wide restoration — do not +read it as either rule being on. ## Probe note -`module: commonjs` and `jsx: react-native`, against `Node16` in both extension and -core. A probe is **not** portable from those repos without rewriting its imports. -`isolatedModules: true` additionally requires `import type` / `export type` for -type-only positions — a probe that re-exports a type without `type` errors for the -wrong reason, which is the parent's Step 5 again. +`module: commonjs` and `moduleResolution: node`, against `Node16` in both extension +and core; `jsx: react-native` against `react`. A probe is **not** portable from +those repos without rewriting its imports. `isolatedModules: true` additionally +requires `import type` / `export type` for type-only positions — a probe that +re-exports a type without `type` errors for the wrong reason, the parent's Step 5 +again. ## Open questions, collected | Question | Settles it | |---|---| -| Is `checkJs` on? | `npx tsc -p tsconfig.json --showConfig \| grep -i checkjs` | -| How big is the `.js` surface? | `find -name '*.js' \| wc -l` | +| Does the `record[k]` then `arr[i]` pair appear? | `grep -rn "rpcEndpoints\[" app --include=*.ts` | +| How big is the `.js` surface? | `find app -name '*.js' \| wc -l` | | Which DOM-named globals resolve? | one-line `type _P = X;` probe per global | -| Does the `record[k]` then `arr[i]` pair appear? | `grep -rn "rpcEndpoints\[" --include=*.ts` | -| Are `no-floating-promises` / `no-unsafe-function-type` on? | `grep -n` in `.eslintrc.js`, then the shared config | -| Are there per-directory tsconfigs overriding these? | `find . -name 'tsconfig*.json' -not -path '*/node_modules/*'` | +| Is any `paths` mapping into `node_modules/**/dist` dead? | test each mapped file exists, then `--traceResolution` | +| Are there per-directory tsconfigs overriding the root? | `find . -name 'tsconfig*.json' -not -path '*/node_modules/*'` | From c1ea26aa37f735e3a3d0b8fdb3118b9f33bba33d Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 07:03:23 -0400 Subject: [PATCH 17/20] Name the owners of the `typescript` domain and its repo overlays Both platform teams co-own the domain, as they do `coding`, `general`, `performance` and `pr-workflow`. A client's overlay is owned by that client's platform team, placed directly under the domain line so it wins on last-match and so each domain PR inserts at its own anchor. --- .github/CODEOWNERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 20217c32..a57ba4bb 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -21,4 +21,7 @@ /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/swaps/ @MetaMask/swaps-engineers /domains/testing/ @MetaMask/qa +/domains/typescript/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/typescript/skills/*/repos/metamask-extension.md @MetaMask/extension-platform +/domains/typescript/skills/*/repos/metamask-mobile.md @MetaMask/mobile-platform /domains/ui/ @MetaMask/design-system-engineers From 6bfa2f576717512e52ff6b19c248d547b3857735 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 08:04:47 -0400 Subject: [PATCH 18/20] Match `BaseController`'s current `Messenger` constraint, and hedge two absolutes `RestrictedMessenger` no longer exists in core or the extension, and the example now follows core's `BaseController` declaration. A hand-written type can miss in either direction, which the PR's own divergence shapes already show, and line count is a minor factor rather than none. CI's lint job runs `lint:tsc` over the full project at its 6144 MB heap and exits 0, so the claim that it always runs out of memory is gone. --- domains/typescript/skills/avoid-any/skill.md | 11 +++++++++-- .../typescript/skills/decompose-large-files/skill.md | 2 +- domains/typescript/skills/derive-types/skill.md | 6 +++--- .../typescript/skills/migration-context-cost/skill.md | 2 +- .../tsc-blindspots/references/metamask-extension.md | 10 ++++++---- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index 0fb55dcf..ddc4fc0f 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -39,8 +39,15 @@ Identify which side of an assignment the `any` sits on: ```typescript class BaseController< - // eslint-disable-next-line @typescript-eslint/no-explicit-any - Messenger extends RestrictedMessenger, + ControllerName extends string, + ControllerState extends StateConstraint, + ControllerMessenger extends Messenger< + ControllerName, + ActionConstraint, + EventConstraint, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + any + >, > // ... ``` diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index dc1ccbf5..1c9d6e80 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -14,7 +14,7 @@ Reference application: `metamask-extension` #41735 (`MetamaskController` decompo Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. -And a single-pass conversion is impractical even for a capable AI — the binding constraint is the file's **context fan-in and fan-out** (upstream source types + downstream consumers), not its line count (see `migration-context-cost`). Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. +And a single-pass conversion is impractical even for a capable AI — the binding constraint is the file's **context fan-in and fan-out** (upstream source types + downstream consumers), with its line count a minor factor (see `migration-context-cost`). Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. ## The decision that matters: what is a coherent unit? diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md index 242238d3..aeb9a488 100644 --- a/domains/typescript/skills/derive-types/skill.md +++ b/domains/typescript/skills/derive-types/skill.md @@ -1,6 +1,6 @@ --- name: derive-types -description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer`) instead of hand-writing ad-hoc types that duplicate, run too wide, and drift. +description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer`) instead of hand-writing ad-hoc types that duplicate, run too wide or too narrow, and drift. maturity: experimental --- @@ -15,7 +15,7 @@ When a type already exists at an authoritative source — a controller's state t An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: - **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. -- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. +- **Incorrect in either direction.** A hand-written type is a _guess_ at the source's shape, and the guess can miss either way. Too wide, it admits values the authoritative type would reject, so invalid data still type-checks. Too narrow, it drops a case the source allows, such as a `| undefined`, and erases the compiler's record of why a runtime guard exists (see the five divergence shapes in `tsc-blindspots`). - **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. ## A grounded example (`metamask-extension` #42583) @@ -55,7 +55,7 @@ type TokenDetails = ReturnType< >; ``` -The messenger itself should extend the controller's `RestrictedMessenger` parameterized with those exported action types, so every `call` signature comes from the controller rather than a hand-rolled overload. The hand-rolled version is worse than a plain duplicate: one return is hand-copied (already looser than the controller's real type), the other (`Promise`) discards the type entirely. +The messenger itself should be a `Messenger` from `@metamask/messenger` parameterized with those exported action types, so every `call` signature comes from the controller rather than a hand-rolled overload. The hand-rolled version is worse than a plain duplicate: one return is hand-copied (already looser than the controller's real type), the other (`Promise`) discards the type entirely. The same PR also typed a dependency `getMetaMaskState: () => Record`, which forced every consumer to re-cast the shape by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. That downstream cast tax is what a too-wide type always imposes; deriving the dependency from the authoritative state type deletes it. Notably the same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand — the discipline is extending it to every referenced type. diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md index ea1ca3b4..f5d7bd19 100644 --- a/domains/typescript/skills/migration-context-cost/skill.md +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -21,6 +21,6 @@ Single-pass conversion of a high-fan-in/fan-out file is impractical even for a c ## How to use it -- **Scope tickets by context cost, not LOC.** Before sizing a migration ticket, estimate fan-in (how many upstream types it must derive from) and fan-out (`grep -rl` importer count). Size by the read-context plus the change surface, not the line count. +- **Scope tickets by context cost more than LOC.** Before sizing a migration ticket, estimate fan-in (how many upstream types it must derive from) and fan-out (`grep -rl` importer count). Size by the read-context plus the change surface, and treat line count as a minor factor. - **Sequence low-fan-out first.** Convert leaf / low-fan-out files early — few downstream edits per PR. Files whose upstream types are already TypeScript are cheaper on the fan-in side and give later conversions more typed ground to derive from. - **When the change surface won't fit one PR, reduce it first.** A hub with high fan-out is the case to decompose: split it into coherent units so each unit's fan-out — and thus each PR — is bounded (see `decompose-large-files`). Decomposition is one response to high context cost, not the only place the cost applies. diff --git a/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md index dd354cf5..a4e56593 100644 --- a/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md +++ b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md @@ -11,10 +11,12 @@ blind spots bite this repo, and where they live, go in the overlay; run mechanic NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit ``` -`package.json`'s `lint:tsc` uses `--max-old-space-size=6144`, which **OOMs** on a -full run on a 16 GB machine — and the OOM exits non-zero with no type diagnostics, -so a naive exit-code check reads it as "errors found." Raise the heap and read the -output. A full run takes roughly 3–5 minutes. +`package.json`'s `lint:tsc` uses `--max-old-space-size=6144`, and CI's `Test lint` +job ran it over the full project at that heap and exited 0 (`main` at +`0269fae48a`, 2026-09-14). A local full run can still **OOM**, and the OOM exits +non-zero with no type diagnostics, so a naive exit-code check reads it as "errors +found." If it does, raise the heap and read the output. A full run takes roughly +3–5 minutes. ## Where to put probes From 42625a08ac693ac925e932fab1b8944674649451 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 08:55:47 -0400 Subject: [PATCH 19/20] Prefer `@ts-expect-error` over `@ts-ignore`, and name the traps in derived types An unneeded `@ts-expect-error` fails `tsc` while an unneeded `@ts-ignore` stays silent. `Omit` over a union keeps only the shared keys unless it distributes, `string & Record` still accepts every string, and a circular import is answered with `import type`, not a placeholder type. A suppression is read for what it hides, and a decomposition greps the repo for consumers the replacement leaves dead. --- domains/typescript/skills/avoid-any/skill.md | 2 +- .../typescript/skills/decompose-large-files/skill.md | 2 +- domains/typescript/skills/derive-types/skill.md | 4 +++- .../tsc-blindspots/references/false-negatives.md | 2 +- domains/typescript/skills/tsc-blindspots/skill.md | 12 ++++++++++-- 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index ddc4fc0f..dc2b1a65 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -85,7 +85,7 @@ Like the generic-constraint case, this `any` is **not infectious** — it is sco Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. -When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a comment saying why (a TODO where a fix is planned), never `@ts-ignore`. An unneeded `@ts-expect-error` fails `tsc` with `Unused '@ts-expect-error' directive` while an unneeded `@ts-ignore` stays silent, and `@typescript-eslint/ban-ts-comment` bans `@ts-ignore` and requires a description on `@ts-expect-error`. To replace a member, assert the replacement instead of erasing the target: `target.method = stub as typeof target.method`, not `(target as any).method = stub`, so `tsc` still rejects a stub whose type does not overlap the original's. Never reach for `any` to unblock feature work "to fix later." ## Declared `any` beats absorbed `any` — and only one of them is countable diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 1c9d6e80..5c4addc2 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -38,7 +38,7 @@ When you *do* extract a unit, it is one self-contained change — no separate "f 2. **Port** the bodies in, unchanged in behavior. 3. **Define the seam** — inject the dependencies the module needs (or register its public methods as messenger actions) instead of reaching back into the file's globals. This is where the human judgment is. 4. **Rewire** the call sites to go through the seam. -5. **Delete the original** in the same change; leave no forwarding stub. +5. **Delete the original** in the same change; leave no forwarding stub. Then grep the whole repo, not only the file, for every consumer of what the new module replaces. An event that no longer has a listener outside the moved code is dead, and a deletion candidate. 6. **Add a structural unit test** against a stub/mock of the seam — especially valuable when the original file had no tests. Steps 1, 2, 5, 6 are largely mechanical (codemod territory — `jscodeshift` on the source, `ts-morph` on the module). Step 3/4 is the part that needs a person. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md index aeb9a488..b6cc4350 100644 --- a/domains/typescript/skills/derive-types/skill.md +++ b/domains/typescript/skills/derive-types/skill.md @@ -10,7 +10,9 @@ Deepens the TypeScript guidance in `mms-coding-guidelines`, and is the structura ## Derive, don't re-declare -When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters` / `Awaited`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inference covers an omitted return annotation, `as const` on a literal, and `satisfies` where a `:` annotation would widen the inferred type. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +Over a union, `Pick` and `Omit` work on `keyof` the whole union, which is only the keys every member shares, so `Omit` loses each member's own keys. Branch first with a distributive conditional type: `T extends unknown ? Omit : never` applies `Omit` to each member. An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md index ea093572..fdb5e906 100644 --- a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -258,7 +258,7 @@ at the time of writing. | `include` | Anything outside it is invisible to `tsc` | `app`, `development`, `shared`, `test`, `types`, `ui`, `*.ts` | | `checkJs` | Off ⇒ `.js` callers unchecked | unset | | `noEmit` + bundler | `tsc` never produces the shipped artifact; webpack/swc transpiles **without** typechecking, so a type error cannot break the build — only the separate `lint:tsc` job reports it | `noEmit: true` | -| `@ts-expect-error` / `@ts-ignore` | Point suppressions | grep before trusting a clean file | +| `@ts-expect-error` / `@ts-ignore` | Point suppressions | grep before trusting a clean file, then read each hit: which diagnostic it hides, and whether the path under review reaches that line | The last row is worth stating plainly: **type errors do not break the build.** They break a CI job. If that job is skipped, filtered, or its output is not read, the diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index a62730d9..75395b8a 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -201,13 +201,17 @@ the fifth was found on a later pass over the same PR: 1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, `number | undefined` for `number`. Admits values the real type rejects; worst - when a guard downstream depends on the narrower form. + when a guard downstream depends on the narrower form. A union ending in + `string & Record` reads as a narrowing and is not one: its last + member accepts every string, typos included. 2. **Dropped nullability** — the source says `| undefined`, the hand-written type doesn't. Erases the compiler's record of why a runtime guard exists. 3. **Duplication** — the same shape written out in two files, unshared. Both copies now need every future change. 4. **Placeholder** — `Record`, `any`, or `unknown` standing in for - a shape that is known. Pushes a cast to every use site. + a shape that is known. Pushes a cast to every use site. A circular import does + not justify one: `import type` the real type, which is erased from the emitted + JavaScript and adds no runtime edge to the module graph. 5. **False precision** — the inverse of a placeholder: the annotation is *narrower* than what actually arrives. `hexValueIsEmpty(value: string | null | undefined)` on a parameter fed `any` at every call site. `tsc` cannot report it, because @@ -222,6 +226,10 @@ the fifth was found on a later pass over the same PR: When a diff adds a hand-written type *and* an `as`, a `!`, a new `?.`, or an `eslint-disable` in the same region, check whether the escape hatch exists to service the type rather than the runtime. Count them — a cluster marks where to probe first. +Ask of each suppression what would fire on that exact line without it. `tsc` reports +an unused `@ts-expect-error` (TS2578) but not an unused `@ts-ignore`, and whether an +unused `eslint-disable` is reported depends on the repo's `reportUnusedDisableDirectives` +setting (`'error'` in extension and core, unset in mobile). ## A typing change should not change runtime behavior From f8a409abd881492cbf037b7c1d16457c518912d3 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 17 Sep 2026 13:17:10 -0400 Subject: [PATCH 20/20] Add `avoid-widening` for literal lookup tables and template literal keys --- .../typescript/skills/avoid-widening/skill.md | 95 +++++++++++++++++++ .../typescript/skills/derive-types/skill.md | 2 +- 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 domains/typescript/skills/avoid-widening/skill.md diff --git a/domains/typescript/skills/avoid-widening/skill.md b/domains/typescript/skills/avoid-widening/skill.md new file mode 100644 index 00000000..353f6a8d --- /dev/null +++ b/domains/typescript/skills/avoid-widening/skill.md @@ -0,0 +1,95 @@ +--- +name: avoid-widening +description: >- + Keep a type as narrow as the value it describes. An annotation wider than + what inference already knows throws information away, so annotate only to + add information. Keep immutable lookup tables literal with `as const + satisfies`, type built keys with template literal types and build them + `as const`, and widen on purpose only at a boundary where inference is + unsound. +maturity: experimental +--- + +# Avoid Widening + +Deepens `MetaMask/contributor-docs` [`docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L40-L42), *Avoid unintentionally widening an inferred type with a type annotation*: "Enforcing a wider type defeats the purpose of adding an explicit type declaration, as it _loses_ type information instead of adding it." `derive-types` covers where a type should come from. This skill covers how wide it should be, and `avoid-any` covers the annotation that turns checking off. + +## When To Use + +- Writing or reviewing a type annotation, a `satisfies` clause or an `as const`. +- A lookup table maps one set of names to another, such as trace names or action types. +- A collection is keyed by strings built from known parts, such as `` `${name}:${id}` ``. +- Proposing a type change in review, where a widening suggestion reads as a tightening. + +## Annotate to add information, never to repeat or lose it + +Compare the annotation with the type inference already produces. Equal adds nothing, and wider loses information. Only a narrower annotation, or a deliberate widening at a boundary (below), is worth writing. + +| declaration | resulting type | +|---|---| +| `const name = 'foo'` | `"foo"` | +| `const name: string = 'foo'` | `string`, widened | +| `const ids = [1, 2] as const` | `readonly [1, 2]` | +| `const ids: number[] = [1, 2]` | `number[]`, widened | + +To check a value against a type without widening it, use `satisfies` ([`docs/typescript.md` L134-L136](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L134-L136)). The exception is a mutable object, array or class that code adds to later. There the narrowest type excludes the additions, so annotate with `:` unless the value is meant to be immutable ([L201-L205](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L201-L205)). + +## Keep immutable lookup tables literal + +```typescript +// 🚫 The annotation widens every value to `TraceName`, so `NAMES[name]` no longer says which member it is. +const NAMES: Record = { + 'Feature Open': TraceName.FeatureOpen, + 'Feature Close': TraceName.FeatureClose, +}; + +// ✅ Checks every key and value, keeps each value's member type, and makes the table read-only. +const NAMES = { + 'Feature Open': TraceName.FeatureOpen, + 'Feature Close': TraceName.FeatureClose, +} as const satisfies Record; +``` + +- A plain object literal with neither widens its values to `TraceName`. +- `satisfies` alone already keeps each value's member type when the checked type is a union of literals, as an enum is. What `as const` adds is `readonly`, plus literal types for nested arrays and objects. It also marks the table as immutable, which is the condition under which the extensible-type exception above does not apply. + +## Type a built key by how it is built, and build it `as const` + +```typescript +// ✅ The map accepts only keys of this shape, so a key built from the wrong name does not compile. +const pending = new Map<`${FeatureTraceName}:${string}`, PendingSpan>(); + +const key = `${params.name}:${params.id}` as const; +pending.get(key); +``` + +- A template literal expression infers as `string` unless it is marked `as const` or contextually typed. So narrowing a collection's key type fails with TS2345 at each `get`, `set` and `delete` that passes an unmarked key. Change the key type and every expression that builds a key together. +- Measured with TypeScript 5.6.3 on [metamask-extension#46123 (preload Perps markets on unlock)](https://github.com/MetaMask/metamask-extension/pull/46123) at [`0118bcc256`](https://github.com/MetaMask/metamask-extension/blob/0118bcc25610de7238243c47d8ba37924251e017/app/scripts/controllers/perps/infrastructure.ts#L274-L306). Narrowing only the span map's key type produced five TS2345 errors, and adding `as const` to its two key expressions cleared them. + +## Widen on purpose only where inference is unsound, and only at a boundary + +When a library's types claim more than its runtime guarantees, the correct annotation is wider. luxon types `plus(duration)` as returning `this`, so a valid `DateTime` stays `DateTime` after `plus()`, although the result is invalid past year 275760. Declaring the return as `DateTimeMaybeValid` restores the distinction the library erased. + +That widening works only where the compiler has to use it: + +| form | forces the check? | +|---|---| +| `const x: DateTimeMaybeValid = start.plus(d)` | no, because control-flow analysis re-narrows `x` to its initializer | +| `start.plus(d) satisfies DateTimeMaybeValid` | no, because it checks assignability and keeps the inferred type | +| `start.plus(d) as DateTimeMaybeValid` | yes | +| a parameter or return typed `DateTimeMaybeValid` | yes | + +So the test is whether an annotation adds information inference lacks. Narrowing usually does. Widening does only where inference is wrong about the runtime. + +## Workflow + +1. Before writing `:`, hover or `tsc --noEmit` to see the inferred type, and write the annotation only if it is narrower, or a boundary widening of an unsound type. +2. For a table that code never adds to, use `as const satisfies `. For one that grows, annotate with `:`. +3. For a collection keyed by built strings, type the key as the template literal it is, and add `as const` to every expression that builds one. Typecheck before proposing the change, since the key type and the builders have to change together. +4. In review, run the proposed suggestion through `tsc` before posting it. A narrowing that leaves the builders unmarked fails the build. + +## Related + +- `derive-types` — take a type from its authoritative source instead of restating it. +- `avoid-any` — the annotation that disables checking. +- `tsc-blindspots` — divergence shapes the compiler cannot report, including a widened local. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md index b6cc4350..9c67238b 100644 --- a/domains/typescript/skills/derive-types/skill.md +++ b/domains/typescript/skills/derive-types/skill.md @@ -10,7 +10,7 @@ Deepens the TypeScript guidance in `mms-coding-guidelines`, and is the structura ## Derive, don't re-declare -When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters` / `Awaited`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inference covers an omitted return annotation, `as const` on a literal, and `satisfies` where a `:` annotation would widen the inferred type. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters` / `Awaited`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inference covers an omitted return annotation, `as const` on a literal, and `satisfies` where a `:` annotation would widen the inferred type. How narrow the result should be, including literal lookup tables and built keys, is `avoid-widening`. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." Over a union, `Pick` and `Omit` work on `keyof` the whole union, which is only the keys every member shares, so `Omit` loses each member's own keys. Branch first with a distributive conditional type: `T extends unknown ? Omit : never` applies `Omit` to each member.