Skip to Content
Course content

250: Code Review Checklist for TypeScript Pull Requests

Click on the "Edit" button in the top corner of the screen to edit your slide content.

How do I handle "any" without being the annoying reviewer who blocks every PR?

We've all been there. You see any and your instinct is to immediately request a change. But if you do that for every single instance, your teammates will start ignoring your feedback. I've found the best approach is to ask why it's there. Is it a genuine edge case, or is the dev just tired of fighting the compiler?

Instead of just saying "no any," suggest unknown. It forces the consumer of the variable to perform a type check before using it, which is exactly what we want for safety. Look at this common pattern in API handlers:

// ❌ The "I'll fix it later" approach
function processWebhook(payload: any) {
  console.log(payload.userId); // Dangerous!
}

// ✅ The "Safety First" approach
function processWebhook(payload: unknown) {
  if (payload && typeof payload === 'object' && 'userId' in payload) {
    console.log((payload as { userId: string }).userId); 
  }
}

If they're using any because the type is too complex to write, that's your cue to help them build a proper interface or a generic. I usually frame it as: "I think unknown fits better here because we can't guarantee the shape of the webhook."

What are the red flags I should look for regarding type assertions?

Whenever I see the as keyword, my internal alarm goes off. Type assertions are essentially telling TypeScript, "Shut up, I know more than you do." The problem is that we're often wrong. The most dangerous spot is when someone casts an API response directly to a type.

If you see const user = data as UserProfile, that's a huge red flag. If the API changes or returns a 404, that code will crash at runtime, and TypeScript won't have warned you. I always push for "Type Guards" instead. It's a bit more boilerplate, but it's the only way to be sure.

// ❌ Dangerous assertion
const user = response.data as UserProfile;

// ✅ Safe type guard
function isUserProfile(obj: any): obj is UserProfile {
  return obj && typeof obj.username === 'string' && typeof obj.id === 'number';
}

if (isUserProfile(response.data)) {
  console.log(user.username); // Now this is actually safe
} else {
  throw new Error("Invalid user data received");
}


How do I tell if a complex type is actually useful or just "over-engineered"?

TypeScript allows for some incredibly powerful type gymnastics—conditional types, mapped types, template literal types—but there's a fine line between "elegant" and "impossible to maintain." I've seen PRs where a developer spent three hours writing a recursive type to map a nested object, but now nobody else on the team knows how to add a new field to it.

When you're reviewing, ask yourself: "If I had to change this logic six months from now, would I understand what's happening in ten seconds?" If the answer is no, suggest simplifying. It's often better to have a slightly more verbose, explicit interface than a "clever" one-liner that requires a PhD in Type Theory to decode.

For example, if you see someone using a complex infer chain just to extract a string from a union, suggest a simple helper type or just being explicit. Remember, code is read far more often than it is written. I'd rather see a few extra lines of clear code than a "magic" type that makes the IDE lag and the developers sweat.




📋 Practical Task

Reviewing the "Customer Order Pipeline" PR

You are reviewing a Pull Request for a new order processing system. The following code has been submitted. Your task is to identify three specific TypeScript smells based on the lesson (specifically regarding any, type assertions, and over-engineering) and write a brief mentor-style comment for each, suggesting a specific fix.

interface Order {
  id: string;
  amount: number;
  status: 'pending' | 'shipped' | 'delivered';
}

async function handleOrderUpdate(update: any) {
  // We know the API returns the Order object here
  const order = update.data as Order;
  
  if (order.status === 'delivered') {
    console.log(`Order ${order.id} is complete.`);
  }
}

// This type is meant to ensure the key is valid for the Order object
type OrderKeyValidator<T> = T extends keyof Order ? T : never;
function getOrderValue<K extends OrderKeyValidator<any>>(order: Order, key: K) {
  return order[key];
}
Rating
0 0

There are no comments for now.

to be the first to leave a comment.