coryhouse / reactjsconsulting

React Related Resources
http://www.reactjsconsulting.com
378 stars 37 forks source link

Code review / audit checklist #165

Open coryhouse opened 1 year ago

coryhouse commented 1 year ago

Here's a handy checklist: https://github.com/mgreiler/code-review-checklist

When reviewing a code base, here’s what I search for:

(See my review docs, PRs, package.json’s)

JavaScript

CI

See CI issue Preview deployments

Source control

What files are the longest? What files are changed the most? (See Gary Burhardt's https://github.com/garybernhardt/dotfiles) Which file do you like best? Which is worst?

Devops

https://dora.dev/research/

Clean up

Clean up first Signs a team is out of control

Package manager

Consider pnpm or Bun. Consider Corepack

Code reuse

https://github.com/kucherenko/jscpd - Find copy/pasted code See #5 for more on reusable components

Dependencies

depcheck or Knip (listed under TypeScript too) Check for updates Suggest alternative packages

Deploys

Notifying stale SPA users

Formatting

Prettier, running via editor and enforced via CI

Hooks

Using pre-push instead of pre-commit Hook is fast Hook should be redundant - CI = real safety Avoiding useEffect

APIs

Return camelCased JSON Handle saving stale records and protect from overrwrites

HTTP

Centralized URLs (for reuse in APIs and mocks)

Env

Wrapped, strongly typed, Zod validated env vars

Folders

Mirrors URL structure Colocates related files No barrels. my tweet thread

File structure

Mirror URL structure Consistently cased Colocating related items Using Dependency Cruiser to validate and enforce intra-project file dependencies.

TypeScript

Use knip or Ts-prune to find unused code and run via CI to protect (something similar for other types of code like CSS? Eliminate any - Use type-coverage - to report type coverage Avoid optional fields Avoid type assertions via “as” (again, can find via type-coverage) Use string literals instead of string Replace string | number with number Don't use 0 or "" for missing numbers Force TS Version Prefer Record over switch See TypeScript issue for more

URL

Does the URL support deep links for sharing when relevant? Is data from the URL copied into state? more here

HTML

disabled main

General

Cpell

Accessibility

See a11y repo

State

Caching fetch calls

CI

Runs build, test, lint At least one approval is required PRs aren’t being rubber stamped No Lint warnings are allowed Use Danger to enforce ad-hoc checks list a Changelog entry

ESLint

eslint-disable Unused imports and vars No var Prefer const Running associated package for each tech

React

index Audit useEffect No needless state

Forms

Validation onBlur Does it reflect latest data if user comes back? (easy mistake to make with react-query as mentioned here) What if you try saving stale data that someone else already changed? Dirty notifcation when leaving