Topic 17.4
Reviewing TypeScript Code
In one line
How to read a pull request like a TypeScript expert: spot the any that leaks, the as that lies, the switch that forgot a case and the type that is too clever, using a short checklist and code that really fails at runtime.
Think of it like this
A building inspector. The house looks fine from the street and every door opens. The inspector does not admire it; they check the places where it can fail: the wiring, the gas pipe, the load-bearing wall. A TypeScript review is the same. Code that compiles can still be dangerous. You look for the few places where the compiler was switched off or lied to.
Words you'll meet
New words in this topic, in plain English. Come back here whenever one feels fuzzy.
- Code review
- Reading a teammate's change before it is merged, looking for bugs and unclear code.
- any leak
- An
anyvalue flowing into typed code, which turns off checking for everything that touches it. - Type assertion
value as T: telling the compiler to treat a value asTwithout any check.- Boundary
- The place where outside data enters your program: network, storage, user input, environment.
- Type guard
- A function or check that proves a type at runtime and tells the compiler, like
value is Order. - Exhaustive check
- A pattern that makes the compiler fail when a union case is not handled.
- assertNever
- A function that takes
never. Calling it in the default branch is an error if any case is unhandled. - Lint rule
- An automatic check, run by ESLint, that flags a risky pattern in code review.
Step by step
01Step 1: read the signature, then the body
Signatures are promises. parseOrder(json: string): Order promises a real Order. The body must earn it. Read down: return JSON.parse(json). JSON.parse returns any, and any is assignable to Order, so the compiler accepted a promise nobody kept.
Run the 'before' example. The first call prints threw parseOrder(...).total.toFixed is not a function: the server sent "42.5" (text), and the type said number.
02Step 2: find the casts
Search the diff for as , !, <T>x, @ts-ignore. Each is a place where a person overruled the compiler. In the example (data as Order).total reads a property of null and throws Cannot read properties of null (reading 'total').
Ask 'what proves this?'. If the answer is 'nothing, we know it is fine', replace the cast with a guard that checks it.
03Step 3: ask what happens when a case is added
The area function checks for a circle, then treats everything else as a square by casting. A triangle has no side, so undefined ** 2 is NaN, and the output shows 3: NaN. No error, no crash, just a wrong number.
A switch with default: return assertNever(shape) fixes it: now adding a hexagon to the union makes this exact call an error until you handle it.
function assertNever(value: never): never {
throw new Error("Unhandled case: " + JSON.stringify(value));
}04Step 4: weigh the clever type
The third example's Path<Settings> builds every dotted path ('theme', 'theme.dark', 'profile.address.city'). A typo is rejected with an error that lists all the valid paths, and the code is only a few lines, so I would approve it. If the error were a wall of generics or the type took seconds to check, I would ask for a plain union.
Notice the review comment is about a decision: 'is the cost worth the bug it prevents?', not about taste.
05Step 5: write the review and apply the fix
The 'after' example fixes all three spots: unknown plus isOrder at the boundary, a guard in place of a cast, an exhaustive switch with assertNever. Same inputs, honest outputs: Bad order is thrown, the guard returns 0, and the triangle area is 10.
06The review checklist
Keep this short list next to you. Each line is one question to ask of a change.
1. any: where does an any enter? is it unknown + validated at the boundary?
2. as / ! : what proves this? could a guard replace it?
3. unions: is every case handled? is there an assertNever default?
4. boundary: fetch / storage / env / input checked at runtime?
5. optional: is every ? or | undefined handled before use?
6. async: is every promise awaited or returned? are errors typed as unknown?
7. types: is any generic type readable? is there a simpler type that rejects the same bugs?
8. escapes: @ts-ignore (use @ts-expect-error with a reason), double casts, eslint-disableTry it yourself
- 1
Find the lie
Before running the first example, read it and write down the three failures you expect. Then run it. Which one would a lint rule (
no-unsafe-return,no-unsafe-member-access) have caught? - 2
Add a hexagon
In the second example, add
| { kind: "hexagon"; side: number }toShape. Read the error before you scroll: which line is it on, and what is the type of the leftover value? - 3
Review your own code
Take a function you wrote recently and apply the checklist. Count how many
as,!andanyyou find, and replace one with a guard.
Code & diagrams
Three bugs the compiler accepts: an any leak from JSON.parse, an as that trusts null, and a hidden default that treats a triangle as a square. The output shows each failure.
Console
(nothing was printed)Expected console output
1: threw parseOrder(...).total.toFixed is not a function
2: threw Cannot read properties of null (reading 'total')
3: NaNunknown plus a type guard at the boundary, a guard instead of a cast, and an exhaustive switch with assertNever. The same inputs now give controlled results.
Console
(nothing was printed)Expected console output
1: threw Bad order
2: 0
3: 10Path of Settings lists every dotted key. The typo on the second getSetting call is a compile error that names the valid paths, yet the code still runs, and the typo walks into undefined and throws, which is the crash TypeScript predicted.
Console
(nothing was printed)Expected console output
Pune
Uncaught TypeError: Cannot read properties of undefined (reading 'city')Break it on purpose
Errors are the best teachers. Make each change, read the error, guess what went wrong, then reveal the answer.
Break #1
Add a case nobody handles
In the second example, add | { kind: "hexagon"; side: number } after the triangle member of Shape.
Break #2
Remove the guard
In the second example, delete the line if (!isOrder(data)) throw new Error("Bad order"); from parseOrder.
Myth vs fact
Myth
If the compiler is happy, the review can focus on style.
Fact
The compiler cannot see lies it was told to accept (any, as, !, @ts-ignore). Those are exactly what a reviewer must find.
Myth
A cast is fine as long as the type is right today.
Fact
A cast is checked by nobody. When the server, the storage format or a union changes, the cast keeps compiling and the failure moves to runtime.
Myth
Banning all clever types makes code better.
Fact
Some earn their place by turning a runtime typo into a compile error with a readable message. Judge each by the bug it prevents and the cost it adds.
Pro corner
Extra depth for experienced readers. New to this? Skip it for now and come back later.
- ▸
typescript-eslint's
strict-type-checkedrules automate most of this checklist:no-explicit-any,no-unsafe-assignment,no-unsafe-return,no-non-null-assertion,no-unnecessary-type-assertion,no-floating-promisesandswitch-exhaustiveness-check. Put them in CI so reviewers can focus on design. - ▸
@ts-expect-errorfails when the line below it stops having an error, so stale suppressions are removed automatically.@ts-ignorenever complains. Prefer the first, always with a reason. - ▸
useUnknownInCatchVariables(part ofstrictsince 4.4) typescatch (e)asunknown. Reviewers should flagcatch (e: any)and(e as Error).messageon unvalidated errors: a thrown value can be a string or anything. - ▸
assertNeveralso gives a runtime safety net: if untyped data produces a value the types said was impossible, you get a clear error instead of a silently wrong result.
Remember this
- 1
The problem this solves: code can compile and still be wrong, because the compiler only checks what it was not told to ignore. A reviewer's job is to find the few places where checking was switched off or the compiler was told something untrue. These five rules tell you where to look. **Rule 1: follow every
any** (Topic 1.6).anyis contagious.JSON.parse,response.json()and untyped libraries return it, and anything you build from it is unchecked. In the first exampleparseOrder(json): Orderhas a correct-looking signature but returnsJSON.parse(json), soanyslips intoOrdersilently, andorder.total.toFixed(2)crashes when the server sends a string. Ask: where does this value really come from, and who checked it? The fix isunknownat the boundary plus one validation (Topic 13.4). - 2
**Rule 2: be suspicious of
as** (Topic 5.6).value as Userdoes not convert or check anything. It tells the compiler to stop looking, and it is only refused when the types are completely unrelated. A correct use exists (narrowing something you really know, such asdocument.getElementById('x') as HTMLInputElementafter you checked the page). A wrong use hides a missing field, as intotalOf(data: unknown)that casts toOrderand then readstotalfromnull. Replace it with a type guard or a validated parse. Also look foras unknown as T(a double cast) and the non-null!: both are 'trust me' markers. - 3
Rule 3: check that every case is handled (Topic 4.6). A
switchorifchain over a union without an exhaustivedefaultquietly mishandles the next member someone adds. The review question is 'what happens when we add a case?'. Withdefault: return assertNever(shape)the answer is 'the compiler points at every function to update'. Without it, as in the first example, atrianglefalls into the final branch and the area isNaN. - 4
Rule 4: question clever types (Topic 10.6). A type that needs a paragraph to explain, takes the editor a second to hover, or produces an error longer than the code is a cost for every reader. Ask three things: Does it reject a real bug? Is the error message readable? Is there a plain alternative (a union of literals, an overload, a simpler generic)? Some clever types earn their keep: the
Path<Settings>type below turns a typo in'profile.adress.city'into a compile error that lists every valid path. - 5
Rule 5: look at the edges. Public function signatures,
exported types, and places where data enters (fetch,localStorage,process.env, form values, query strings). Types in the middle of a program matter less; the boundary is where lies get in. Also look at optional properties used without a check,Promises that are never awaited,Object.keysresults used askeyof, and@ts-ignorecomments (a@ts-expect-errorwith a reason is better, because it fails when the problem is fixed). - 6
How to write the comment. Name the failure, not the style: 'This compiles, but
parseOrderreturnsanyhere, so a stringtotalcrashes at line 12. SuggestunknownplusisOrder.' Show the input that breaks it. Keep a short checklist (below) so reviews are consistent. Strict flags and typescript-eslint (Topic 15.2) catch many of these automatically, so mention a lint rule when one exists (no-explicit-any, theno-unsafe-*family,no-unnecessary-type-assertion,switch-exhaustiveness-check,ban-ts-comment).
Explain it without notes
What do you look for first when reviewing a TypeScript pull request?
Why is JSON.parse(text) as User a review blocker, and what do you ask for instead?
A teammate adds a member to a union. How do you make sure every switch is updated?
When is a type assertion acceptable?
How do you decide whether a clever type should be approved?
What is the difference between @ts-ignore and @ts-expect-error, and what is your team rule?
Practice
Review this code. Name each problem and rewrite it.
``ts
async function loadUsers(): Promise<User[]> {
const res = await fetch("/api/users");
return (await res.json()) as User[];
}
This function compiles. What crashes, and how do you stop it at compile time?
``ts
type UiEvent = { type: "click"; x: number } | { type: "key"; code: string };
function describe(e: UiEvent): string {
if (e.type === "click") return click at ${e.x};
return key ${(e as { code: string }).code};
}
A teammate wrote catch (e: any) { alert(e.message); }. What is wrong, and what is the safe version?
Trade-offs
- ↔
Strict review rules slow a change down and annoy people if applied by hand. Automate what you can (strict flags, lint rules in CI) and keep human review for design judgement.
- ↔
A guard function costs a few lines and a little runtime; in exchange the type is true. A schema library reduces the lines but adds a dependency. Choose by how many boundaries you have.
Done when you can
I can find an
anyleak and say where the value really came from.I can replace an unsafe
aswith a guard or a validated parse.I can make a
switchexhaustive withassertNever.I can judge a clever type by the bug it prevents, its error message and its cost.
I can write a review comment that names the failure and the fix.
I can name lint rules that automate parts of the checklist.