react_pr_review.md

Things to look for when reviewing a React PR

Table of Contents

• PropTypes

PropTypes

Be as specific as possible

You should almost never use PropTypes.any. Whenever possible, use the exact type.

// Bad
static get propTypes() {
  return {
    name: PropTypes.any
  };
}

// Good
static get propTypes() {
  return {
    name: PropTypes.string.isRequired
  };
}

Why?

The more specific the PropType, the more likely it is to catch a bug and display a warning to the developer.

For stores, use instanceOf

// Bad
static get contextTypes() {
  return {
    authStore: PropTypes.object.isRequired
  };
}

// Good
static get contextTypes() {
  return {
    authStore: PropTypes.instanceOf(AuthStore).isRequired
  };
}

Why?

1. Any old object won't due, which makes this a better runtime check.
2. It's easier for new developers to know what to look for. It's confusing to see object as the type as it could be any object.

If something is required, use isRequired

// Bad
static get propTypes() {
  return {
    count: PropTypes.number
  };
}

// Good
static get propTypes() {
  return {
    count: PropTypes.number.isRequired
  };
}

Why?

Similar to the why of being as specific as possible, if your component relies on a prop existing, declaring the prop as required adds one more tool for debugging and might catch bugs early.