Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions domains/typescript/skills/avoid-any/skill.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
---
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. 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
---

# 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.

## 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.

```typescript
class BaseController<
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Messenger extends RestrictedMessenger<N, any, any, string, string>,
> // ...
```

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<any, any>`) 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: <E>(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."

## 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<any>` 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<T>` 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.
56 changes: 56 additions & 0 deletions domains/typescript/skills/decompose-large-files/skill.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
---
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).

## 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 — 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?

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.

## Identifying the boundaries is the key — extraction is optional

**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.
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.
64 changes: 64 additions & 0 deletions domains/typescript/skills/derive-types/skill.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
---
name: derive-types
description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer<typeof struct>`) 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<typeof struct>`; 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-rolled a messenger type, re-declaring each controller action's signature and **hand-copying its return shape** inline.

🚫 Reinvents the controller's messenger and re-states its action returns:

```typescript
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<unknown>;
};
```

✅ Derive each return from the controller's exported action type; don't hand-copy it:

```typescript
import type { AssetsContractControllerGetTokenStandardAndDetailsAction } from '@metamask/assets-controllers';

// the action already types its own return — derive it
type TokenDetails = ReturnType<
AssetsContractControllerGetTokenStandardAndDetailsAction['handler']
>;
```

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<unknown>`) discards the type entirely.

The same PR also typed a dependency `getMetaMaskState: () => Record<string, unknown>`, 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

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.
Loading