Skip to content
Merged
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
88 changes: 64 additions & 24 deletions .claude/skills/typescript-code-and-test-standards/SKILL.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,5 @@ Where you find nothing, say so in one line. Do not invent findings to fill the r

- **Quote the actual text.** A finding you cannot quote is dropped. Where the text holds a credential value, such as a token, a password, an API key, a private key, or a session identifier, quote it with that value replaced by `[REDACTED]`: a redacted quote is a quote, so the finding still ships, and the substitution is made when the finding is written rather than when the file is searched.
- **Read the body before describing a symbol.** Never write a description inferred from a name; that is the drift this audit exists to stop.
- **Do not report what the linter reports.** Formatting, spacing, and line width are not yours.
- **Do not report what the linter reports**, meaning a rule the project has actually enabled. Indentation, line width, and quote style are not yours. Neither are the blank lines separating groups inside a body: no tool owns those, but this pass is about comments and documentation blocks, so it does not claim them either.
- One finding per defect.
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ These do **not** qualify: a module that is merely slow, a module that is awkward
- A test deleted or an assertion weakened in the diff, where the change is not a deliberate behaviour replacement stated as such.
- A fallback added in production code that exists only to make a test pass.
- An orphan test file with no same-named source beside it, or a second test file for one source.
- A test that would still pass if the behaviour it names were broken. Break the behaviour in your head and ask which assertion fails; if none does, this is the finding.
- Cases that cannot fail independently of one another, which lock in one behaviour under several names.
- Setup or a fixture disproportionate to what the assertion reads, where most of the value built is never checked.

## Flakiness sources to scan for

Expand Down Expand Up @@ -98,5 +101,5 @@ Where you find nothing, say so in one line. Do not invent findings to fill the r
- **Quote the actual line.** A finding you cannot quote is dropped, not softened. Where the line holds a credential value, such as a token, a password, an API key, a private key, or a session identifier, quote it with that value replaced by `[REDACTED]`: a redacted quote is a quote, so the finding still ships, and the substitution is made when the finding is written rather than when the file is searched. A test fixture is the usual place a credential value turns up.
- **Read the mocked module before judging the mock.** A guess about whether it holds logic is worthless here, and it is the one thing this pass exists to establish.
- **Respect a deliberate decision.** A mock with a clear boundary comment, a grandfathered test title, or a convention the project's own rules file mandates is not a finding.
- **Do not report what the linter reports.** Formatting, unused variables, and import order are not yours.
- **Do not report what the linter reports**, meaning a rule the project has actually enabled. Unused variables and import order are not yours.
- One finding per defect. Do not restate the same mock under three headings.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ Copy this file to `.github/instructions/typescript-standards.instructions.md` in

## Precedence

This project's formatter owns indentation, quotes, semicolons, width, trailing commas, and import order. Its linter owns unused variables, equality, and brace enforcement. Its compiler owns types. Never report a violation of a rule the project's configuration has turned off, and never change a configuration file to match these rules.
This project's formatter owns indentation, quotes, semicolons, width, trailing commas, and import order. Its linter owns unused variables, equality, and brace enforcement, for every rule it has turned on. Its compiler owns types. Never report a violation of a rule the project's configuration has turned off, and never change a configuration file to match these rules.

**A rule configured off is a decision. A rule never configured is silence, and silence cedes nothing.** A linter running a handful of rules has taken no position on the rest, so its quiet is not a reason to drop a finding below.

Everything below is what those tools cannot check.

Expand All @@ -33,18 +35,21 @@ Everything below is what those tools cannot check.
- **No Markdown link syntax.** `[text](url)` is Markdown's, and `[name](#anchor)` renders as dead text in a hover tooltip. Use `{@link SymbolName}`, `@see https://example.com`, or `{@link https://example.com Display text}`.
- The block precedes a decorator and never sits between the decorator and the declaration.

## Readability
## Readability and naming

- Braced blocks except for a single-line early exit (`if (!data) return;`, `break`, `continue`, `throw`).
- Braced blocks except for a **short** single-line early exit (`if (!data) return;`, `break`, `continue`, `throw`). Length is what makes that exit readable, not the fact that it exits early: a long condition returning a long expression takes braces, and the condition is extracted to a named predicate. A formatter prints an unbraced single-statement `if` as written however far past the print width it runs.
- A blank line before `return`, `break`, `continue`, and `throw` when not first in the block.
- No blank lines between `switch` cases.
- Separate groups doing different work with a blank line: setup, action, assertion.
- Separate groups doing different work with a blank line: setup, action, assertion. **A body with no blank line anywhere is the finding.** A formatter collapses and strips blank lines and never inserts one, so a file written as a single block stays one block, correctly indented and unreadable.
- **A name is clear to a reader meeting it for the first time.** Do not abbreviate by deleting letters. A short name belongs to an established idiom in a tight scope, a loop index or a caught error; a run of unrelated single letters (`a`, `b`, `c`) is an absent name, not a short one.
- A function body whose length, nesting depth, or widest expression runs past what a reader holds at once is a finding, backstopped at 50 lines, three levels, and a condition of more than about three logical operators or wider than the project's own print width. A long condition is usually type narrowing, so extract it to a named predicate, or to a type guard (`function isFoo(x: unknown): x is Foo`) where the caller needs the narrowing. A deeply nested body flattens by early return, and a nested chain of `typeof`/`in`/truthiness checks wants a discriminated union.

## Tests

- **Logic changes, bug fixes, and new features land with their tests in the same change.** Pure refactors need no new tests, but no existing test may be skipped, deleted, or weakened. A diff that weakens a test is a behaviour change, not a refactor.
- **One test file per source file**, colocated and same-named. No orphan test file, no test file named after a function that lives elsewhere, no second test file for one source.
- **Never**: skip, gut, or delete a failing test; use `.skip`; write a no-op assertion or one that restates the implementation; type-assert an already-typed value; build a one-row table-driven test; or add a fallback in production code to make a test pass.
- **Never** leave a test that would still pass if the behaviour it names were broken, write cases that cannot fail independently of one another, or build setup disproportionate to what the assertion reads.
- Every test answers one question: what behaviour does this lock in that a real future change could break? The review form is sharper: **would this test fail if the behaviour it names were broken?**
- Name the subject in the suite and the behaviour in the case, as a third-person verb phrase. New titles do not start with "should".
- Table-driven tests name every field; no positional rows. Rows that differ in the assertion body belong in separate cases.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Record your strictness posture and anything a contributor gets wrong repeatedly,

## Style guide carve-outs

Record every rule from the skill's Google TypeScript Style Guide digest that your project deliberately does not follow, and why. The four most commonly overridden are default exports, file naming, underscore prefixes on intentionally unused bindings, and mandatory return-type annotations.
Record every naming, type-system, or style rule the skill carries that your project deliberately does not follow, and why. Its style digest is where most of them are written down, and the naming and readability rules sit in the skill body beside them. The four most commonly overridden are default exports, file naming, underscore prefixes on intentionally unused bindings, and mandatory return-type annotations.

## Mock boundaries in this project

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
# Google TypeScript Style Guide digest

Digest of the [Google TypeScript Style Guide](https://google.github.io/styleguide/tsguide.html), limited to rules a formatter and linter do not already enforce. Consult it when the host project's configuration, its rules files, and the surrounding code all leave a question open.
Digest of the [Google TypeScript Style Guide](https://google.github.io/styleguide/tsguide.html), limited to rules a formatter and linter do not already enforce. It is read two ways.

**Type system**, **Assertions and suppressions**, **Errors**, and the **Defects** half of **Language features** are read on every review. Each decides whether code is wrong, provable from the language or the runtime without knowing what the program is for, so none of them waits for a question to be open.

The rest settles a question the host project's configuration, its rules files, and the surrounding code all leave open.

Anything the project's formatter settles (quotes, semicolons, line width, blank lines at block edges, import order, trailing commas) is out of scope: run the formatter, do not hand-adjust.

**What a formatter settles is narrower than it looks.** It collapses runs of blank lines and strips them at a block's edges, and it never inserts a separating one, so a body written without blank lines stays without them. It does not narrow an over-wide expression either: an unbraced single-statement `if` is printed as written however far past the print width it runs. Both of those judgements are the reader's, not the formatter's.

**A project that consistently applies a different variant of a rule below has a preference, not a defect.** Follow the project. Several of these rules are commonly and deliberately overridden, and each such rule says so where it applies.

## Contents
Expand All @@ -12,7 +18,7 @@ Anything the project's formatter settles (quotes, semicolons, line width, blank
- Type system
- Assertions and suppressions
- Imports and exports
- Language features
- Language features (defects, then preferences)
- Errors
- Comments and documentation
- Rules frameworks commonly override
Expand All @@ -23,7 +29,6 @@ Anything the project's formatter settles (quotes, semicolons, line width, blank
- `lowerCamelCase` for variables, parameters, functions, methods, properties, and module aliases.
- `CONSTANT_CASE` for module-level constants and enum values that are genuinely immutable, not for every `const`.
- Treat acronyms as words: `loadHttpUrl`, not `loadHTTPURL`.
- Names must be clear to a new reader. Do not abbreviate by deleting letters. Variables in scope for ten lines or fewer may use short names.
- A local alias of an existing symbol keeps the original's naming format.

## Type system
Expand Down Expand Up @@ -60,18 +65,30 @@ Anything the project's formatter settles (quotes, semicolons, line width, blank

## Language features

- `const` by default, `let` when reassignment is needed, never `var`. One variable per declaration.
Split by what a violation is, because the two halves are read at different times. A **defect** is provable from the language or the runtime without knowing what the program is for, and is read on every review. A **preference** is read only where the project has left the question open.

### Defects

- Never `var`. Its function scoping makes a binding captured inside a loop hold the loop's final value in every closure.
- `===` and `!==` always, except `== null` when both `null` and `undefined` should match.
- Braced blocks for control flow.
- Every `switch` has a `default`, placed last, and non-empty groups do not fall through.
- Prefer `for...of`. Never unfiltered `for...in`; use `Object.keys()` or an own-property check.
- Spread objects into objects and arrays into arrays only; never spread a primitive, `null`, or `undefined`.
- Prefer `for...of`. Never unfiltered `for...in`, which walks inherited enumerable keys and hands back an array's indices as strings; use `Object.keys()` or an own-property check.
- Never array-spread a non-iterable. `[...null]`, `[...undefined]`, and `[...42]` throw at runtime. Object spread is total, so `{...null}` evaluates to `{}` rather than throwing, which is why only the array form sits here.
- Convert types with `String()`, `Boolean()`, `Number()`, template literals, or `!!`, never with `new`. Do not use unary `+` for string to number. Check for `NaN` explicitly, since it compares unequal to everything including itself. Use `Number()` for a complete conversion and `parseInt` only to read a non-decimal base or a leading numeric substring, always with a radix.
- No `eval`, `with`, `debugger` in production, builtin prototype modification, or the `Array()` and `Object()` constructors. `Array(3)` builds three empty slots rather than an element.
- Do not set non-numeric properties on an array; use a `Map` or an object.
- `sort()` compares by string by default, so sorting numbers without a comparator puts 10 before 9.
- **A mishandled promise fails silently, which is what puts these here rather than among the preferences.** Flag a promise whose rejection is neither handled nor propagated, because the rejection surfaces as an unhandled rejection far from its cause. Flag an `async` callback handed to a non-awaiting iterator, `forEach` being the common one, when the caller needs to await the work it starts. Flag `map` producing promises without awaiting or returning an aggregate such as `Promise.all`.

### Preferences

- `const` by default, `let` when reassignment is needed. One variable per declaration.
- Braced blocks for control flow.
- Prefer function declarations for named functions; use arrow functions rather than function expressions. Use a concise arrow body only when the return value is used.
- Classes should not hold properties initialized to arrow functions, which obscures `this`.
- Convert types with `String()`, `Boolean()`, `Number()`, template literals, or `!!`, never with `new`. Do not use unary `+` for string to number. Check for `NaN` explicitly. Reserve `parseInt` for non-decimal bases.
- Do not write an explicit boolean coercion where the context already coerces, such as an `if` or `while` condition. Enum values are the exception: compare them explicitly.
- No `const enum`. No `eval`, `with`, `debugger` in production, builtin prototype modification, `Array()` or `Object()` constructors, `require()` imports in a module file, or `namespace Foo {}`.
- Do not set non-numeric properties on an array; use a `Map` or an object.
- Spread objects into objects and arrays into arrays only.
- Classes should not hold properties initialized to arrow functions, which obscures `this`.
- No `const enum`, no `require()` imports in a module file, and no `namespace Foo {}`.

## Errors

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ Confirm the host project's own exemption list before applying this one. A projec
- **Never type-assert an already-typed value.** Checking that a `string` is a string tests the compiler, and the compiler already ran.
- **Never build a one-row table.** A table with one row is a loop that runs once, which is a plain test case written indirectly.
- **Never add a fallback in production code to make a test pass.** A `?? defaultValue` inserted to satisfy an assertion moves a defect from the test into production.
- **Never leave a test that would still pass if the behaviour it names were broken.** Break the behaviour in your head and ask which assertion fails. If none does, the test names something it does not check, and it will keep passing through the regression it was written to catch.
- **Never write cases that cannot fail independently of one another.** Several cases breaking together lock in one behaviour, not several. The extra names cost a reader time and buy no coverage, and they make a single regression look like a suite-wide collapse.
- **Never build setup disproportionate to what the assertion reads.** A forty-line fixture feeding a test that asserts one field states thirty-nine facts the test does not check, and every one of them is a way the test breaks for a reason unrelated to its subject.

## The question every test answers

Expand Down Expand Up @@ -100,6 +103,8 @@ Put shared setup in the runner's before-each hook, and clear mock state there to

Prefer building a fresh subject per case over sharing one across the file.

**Size the setup to the assertion.** A fixture is read as a claim about what the case needs, so a field the assertion never reaches is noise that later breaks the case for an unrelated reason. Build the smallest value the behaviour requires, and where several cases genuinely need different parts of one large object, that is a sign they belong in separate suites rather than behind one shared hook.

## Time, timers, and other flakiness sources

- **Debounced or delayed behaviour** uses the runner's fake timers, installed in the before-each hook and torn down in the after-each hook after running pending timers. Real waiting in a test is slow and non-deterministic.
Expand Down
Loading