Purpose of this document
Onboarding Guide: Introduces engineers new to JavaScript and TypeScript to DOs & DONTs of these languages.
Improve Type usage: Aim at simplifying how we use types, as types can get very complex and hard to deal with quickly if not used correctly.
Unified Style Guide: Reduces debates on code style and guidelines by making the decision once.
Efficient Reviews: Simplifies PR reviews and discussion by allowing referencing guidelines that already contain examples.
Everyone is welcome to contribute to this document
Most rules originate from https://nathanbrachotte.notion.site/Code-Style-and-Architecture-f774439d3c6b4ae5981b82ca3c7437ca
General
Tech debt
We follow the boy scout rule. Refactor as you go when you see potential improvements that can be made to the code youβre touching.
Comments
Provide context and explain complexity:
Include product context (why we're implementing this feature)
Explain unusual or non-obvious solutions (why we're using this specific approach)
Clarify any complex or potentially confusing parts of the code
Aim for clarity: Help other developers (or your future self) easily understand the reason this code exists.
Colocate your comments
Co-locating comments means putting comments right next to the code they explain, making it easier to understand and keep everything up-to-date.
See https://kentcdodds.com/blog/colocation#lets-talk-about-code-comments
Mark deprecated files
When deprecating something (Components, functions):
/\*\*
- @deprecated - Give a reason and an alternative
\*\*/
This will make it very obvious in your IDE:
When deprecating a whole file
Change the file name to DEPRECATED_xx
Naming
Booleans
Prefix with has or is if possible
Functions
Prefix with verbs get, has, extract etc
Constants
Use SCREAMING_SNAKE_CASE
Use positive names (to avoid double negatives)
// β
Easy to read
if (newPushNotificationsEnabled) {
...
}
// β Double negative, harder to read
if (!newPushNotificationsDisabled) {
...
}
There can be times where we need to name constants as negatives and thatβs okay, but we should default to positives.
JavaScript
Strict equality
In JavaScript, == is the equality operator, while === is the strict equality operator.
// β
Safe comparison
const isTheSame = 0 === '0' // false
// -> compares both value and type without converting them.
// β Unsafe comparisons
const isTheSame = 0 == '0' // true
// -> compares two values for equality after converting both values to a common type (type coercion)
// '0' is converted to 0 before comparison. Error-prone.
Exception: You can use the equality operator when checking for non-nullish values:
// β
Allows you to easily check if something is not nullish (undefined | null)
// While also allowing other falsy values ('', 0, false, NaN)
const hasParams = myParam != null
Guard Pattern (or Early return) pattern
// β Nested ifs, hard to read, hard to tell what's returned
getRoute() {
if (isEpisode) {
if (item.kind === "original") {
return routes.guidePath(item.show.slug)
} else {
return routes.browseEpisodePath(
item.show.slug,
item.contentItemId
)
}
} else if (isBlocked) {
return routes.basicBookPath(item.slug)
} else {
return routes.searchBookPath(item.slug)
}
}
// β
Return early, reads well, no nesting
getRoute() {
if (!isEpisode && isBlocked) {
return routes.basicBookPath(item.slug)
}
if (!isEpisode) {
return routes.searchBookPath(item.slug)
}
if (item.kind === "original") {
return routes.guidePath(item.show.slug)
}
return routes.browseEpisodePath(
item.show.slug,
item.contentItemId
)
}
Avoid function definitions
Function definitions in JS have global scope, they also bind this and they hoist themselves. These oddities can cause unforeseen issues if you are not prepared for them. In order to avoid potential βgotchasβ, we advise you use arrow functions whenever possible.
// β avoid function definitions
function myFunc() {
// do something
}
// β
use const containing arrow function definitions
const myFunc = () => {
// do something
}
Avoid creating partial Arrays
You might think of creating an array of length 5 like this. Array(5)
Avoid doing this as it can cause some very weird behaviour. Consider the following:
// β
const t = Array(5)
// [ <5 empty items> ]
t.length
// 5
t.forEach((\_,i) => console.log(i))
// undefined and nothing is printed to console. Array is not iterated over.
// β
const t = Array.from({length: 5})
// [ undefined, undefined, undefined, undefined, undefined ]
t.length
// 5
t.forEach((\_,i) => console.log(i))
// undefined and 0...4 is printed to console. Array _is_ iterated over.
If you want to initialise your array with the Array.from({length: 5}) method, you can use a second argument:
const t = Array.from({length: 5}, (\_,i). => i+1)
// [1,2,3,4,5]
Donβt use default exports
const myCoupon = {
coupon_code: ["blackfriday"],
slug: "blackfriday",
discount: 50,
starting_at: "2024-04-01",
expires_at: "2027-04-01"
}
// β When importing `myCoupon` in another file, we lose its reference and can make tree-shaking harder
export default myCoupon
// β
Use "find reference" feature in your IDE and you can see everywhere it's used
const myCoupon = { ... }
Don't use "should" in test strings
Add βshouldβ to the start of a test is superfluous as the very act of running a test is to assert the specified behaviour.
// β
it("should add 1", () => {
expect(adder(1)).toBe(2)
})
it("should work for large numbers", () => {
expect(adder(1_000_000)).toBe(1_000_001)
})
// β
it("adds 1", () => {
expect(adder(1)).toBe(2)
})
it("works for large numbers", () => {
expect(adder(1_000_000)).toBe(1_000_001)
})
TypeScript
Consuming APIs
If youβre in a monorepo and using REST: trpc
If you are using GraphQL: use Codegen and generate types (and hooks) automatically
If you are consuming a 3rd party REST API: End-to-end type safety for the REST of us.
Interface VS Type
Use β
type over βinterface when declaring a type
interfaces are changeable by the rest of the codebase (declaration merging).
Example
type Person = {
name: string
}
// This gives Duplicate identifier 'Person' error
type Person = {
age: number
}
interface Person {
name: string
}
// Declaring Person by mistake somewhere else in the codebase
interface Person {
age: number
}
const john: Person = {
name: "John",
age: 26,
}
Read this article for a deeper dive into the topic https://blog.bitsrc.io/type-vs-interface-in-typescript-cf3c00bc04ae
When extending a big existing type, prefer interface : https://www.totaltypescript.com/react-apps-ts-performance
Immutability
Using mutable variables is error-prone. It can have nasty side effects, and TypeScript will give you a rough time.
Let VS Const
Default to const
// β If we keep mutating myArray it can become hard to understand what value it holds at a specific time
let myArray = [1, 2]
myArray = myArray.map(v => v \* 2)
// β
Easier to keep track of myArray's value
const myArray = [1, 2]
const myDoubledArray = myArray.map(v => v \* 2)
// ππ» Exception: try/catch
// Easy to set a starting value (or set to undefined)
let apiResponse = [1]
try {
if(shouldFetch) {
apiResponse = await fetch("...")
}
} catch (error) {
// Gives us easy way to handle errors
console.error(error)
// And we can even set fallback values
apiResponse = [1, 2]
}
apiResponse.map(...)
Immutability π€π» Typescript
Ideally, always default to working with immutable values:
// β Adding properties to an object after its declaration can lead to type errors and unexpected behavior
async function findUserFromPostIds(postIds: string[]) {
const query = {
include: {
post: true,
},
};
if (postIds.length > 0) {
query.where.postId = { in: postIds };
// ^? β TS Error: Property 'where' does not exist on type '{ include: { post: boolean; }; }'
// π `query` type doesn't contain a `where` key
}
return await prisma.user.findMany(query);
}
// β Avoid using overly broad types as they defeat the purpose of TypeScript's type safety.
// And prevent TypeScript from calling you out on wrong types
async function findUserFromPostIds(postIds: string[]) {
const query: Record<string, any> = { // β This too wide of a type
include: {
post: true,
},
};
if (postIds.length > 0) {
query.where.postId = { in: postIds }; // β
You are able to add entries to it
}
return await prisma.user.findMany(query);
// ^? β But the returned type will not include `post` because the param type wasn't narrow enough
}
// ~β
Use lib-generated types (in that case, by Prisma)
async function findUserFromPostIds(postIds: string[]) {
const where: Prisma.UserWhereInput = {
include: {
post: true,
},
};
if (postIds.length > 0) {
query.where.postId = { in: postIds };
}
return await this.prisma.user.findMany({
where: where,
});
}
// β
Use implicit type by creating immutable objects
async function findUserFromPostIds(postIds: string[]) {
const query = {
include: {
post: true,
},
// π Alternatively, declare another const before `query` is declared if that gets too complex
where: postIds.length > 0 ? { postId: { in: postIds } } : undefined
};
// β
Query instantly has the correct type
return await prisma.user.findMany(query);
// π Note: You can always create new objects to add new fields, as long as it's immutable:
return await prisma.user.findMany({ ...query, include: { ...query.include, username: true }});
}
The as mistake keyword
Why itβs a mistake
Maintainability: Using as often will lead to issues when refactoring our code. If the actual structure of the data changes, the TypeScript compiler won't be able to point out mismatches where the as keyword is used.
Type Safety: When you use as , you're effectively telling the compiler to trust you that you know the type of an object better than it does. This can easily lead to errors that the compiler would usually catch because you're bypassing its type-checking. If your assertions are incorrect, you'll experience runtime errors that could have been prevented.
βBut TS doesnβt correctly handle this and that JS method.β
2 solutions:
Use TS-reset lib. It overrides some of TS types for JS methods to work better
https://github.com/total-typescript/ts-reset
If TS-reset doesnβt handle your use case, build a helper to βcorrectlyβ type the poorly handled method. For example, to correctly type Object.entries:
type Entries<T> = {
[K in keyof T]: [K, T[K]]
}[keyof T][]
export const getEntries = <T extends object>(obj: T) =>
Object.entries(obj) as Entries<T>
What if I still use as?
We add a comment explaining why that is and hints at how we could avoid using it in the future.
The only valid reasons are:
When types from another lib are broken
When types from another lib are missing/lacking
When fixing the types would take days, and we are 100% sure there wonβt be any runtime error because of it
Use implicit types
Constants
// β The return type of getUser is already `User`
// Declaring the type here complicates refactoring if the return type is changed.
const user: User = await getUser(userId);
// β
const user = await getUser(userId);
// ^? User
Functions
// β οΈ Sometimes, it's okay to specify the return type of a function
// When you want to check the function's complex internal logic returns the right type.
async getUser(userId: string): Promise<User> {
const user = await sql`SELECT ...`
// ^? any
const superComplexCalculationWithUser = ...
// We should validate at runtime here too
return superComplexCalculationWithUser
}
// β
Most of the time, we don't need to specify anything,
// as we should mostly rely on implicit types
// Example: Prisma returns a type that changes implicitly based on arguments.
async getUser(userId: string) {
const user = await this.prisma.user.findUnique({
where: { id: userId },
select: { name: true, }
});
return user;
}
const name = getUser("123").name
// ?^ string - Type is inferred by Prisma
// If we were to remove `name: true` from the select,
// TS would start throwing on every place that relies on `.name` anyways.
Non-nullish assertions
The problem
In some instances, it is possible we know a value is here, but TS doesnβt. For example we could get from Prisma:
// In the args, we make sure only users with posts are returned
const usersWithPost = this.prisma.users.findMany(...)
// ^ User & { post?: Post}
It will become annoying to work with post because although we know from the filtering in args that we only got the users with posts, post type is still optional.
// β We have to check for nullish value every time
const allPostTitles = usersWithPost.map(user => user.post?.title).map(Boolean)
// ?^ Post | undefined
So we might wanna go with:
// β Forcefully tell TS the value is non-nullish
const allPosts = usersWithPost.map(user => user.post! && ...)
// ?^ Post
But this might bring its own set of problems in the future.
If we start using ! every time we think weβre more right than TS, we might start using it at times weβre wrong, and create hard-to-catch bugs.
If we change the arguments in findMany function to also include users that donβt have a post, we might effectively lie to TypeScript the same way we would do with as and run into runtime issues
Alternative
https://github.com/alexreardon/tiny-invariant
tiny-invariant is a library that check if a value is falsy, if it is, it throws. If it isnβt, it narrows the type. All in one line β€οΈ
const allPostTitles = usersWithPost.map(user => {
// ?^ user.post: Post | undefined
// β
Validate at runtime and narrow the type
invariant(user.post, "Found user without a post")
return user.post.title
// ?^ Post
})
Avoid Types.As.Objects
In general try to import the most specific thing you can. Avoid import entireLib from "some-lib" if possible. This is especially true for import \* statements. This will help bundlers figure out what they can tree-shake.
// β Long and verbose.
import react from "react"
const Cloner = ({children}: {children: react.JSX.Element}) => react.cloneElement(
<Row title="Cabbage">
Hello
</Row>,
{ isHighlighted: true },
'Goodbye'
)
// β
import { JSX, cloneElement } from "react"
const Cloner = ({children}: {children: JSX.Element}) => cloneElement(
<Row title="Cabbage">
Hello
</Row>,
{ isHighlighted: true },
'Goodbye'
)
React / JSX
Avoid high mem ops in render functions
In React functional components, the body of your function is processed on EVERY render cycle. It is important to consider this and try to reduce processing power and memory usage whenever possible.
Consider these examples:
// β emojis does not use props or state, it does not need to live inside the render
// function. This creates an array ON EVERY RENDER
const MyStaticComponent = () => {
const emojis = Array.from({length: 5}, () => <div className="w-2 h-2">π</div>)
return (
<>{emojis}</>) }
// β
emojis is created once and then reused. This is the most ideal form as
// the calculation is done once ever, outside of the render loop (non-blocking)
const emojis = Array.from({length: 5}, () => <div className="w-2 h-2">π</div>)
const MyStaticComponent = () => (
{' '}
<>{emojis}</>)
// β This array gets freshly created on every render. This might be not be a problem
// if the component is rendered once on a page, but consider a situation where you
// render a 100 list of user reviews, each with their own rating. Now every page render
// has to recreate 100 arrays!
const MyRatings = ({ ratings }: { ratings: number[] }) => (
{' '}
<div className="some-css">
{ratings.map((todo) => (
<div key={`${rating}`}>{rating}</div>
))}
</div>
)
// β
Now each ratings component is only rendered once as long as the ratings
// do no change. Now the calculations are only done on the first page render.
const MyRatings = ({ ratings }: { ratings: number[] }) => {
const ratingsComponent = useMemo(
() => ratings.map(
todo => <div key={`${rating}`}>{ rating }</div>
)
, [ratings])
return (
<div className="some-css">{ratingsComponent}</div>) }
// β This button component creates a new onClick event function on EVERY RENDER
// #DeathByOneThousandCuts
const MyButton = ({ parentOnClick }: { parentOnClick: (e: MouseEvent) => void }) => (
<button onClick={(e) => {
// do something
// handle parent event
parentOnClick(e)
}
}>
My button
</div>
)
// β
Now the handler is only created once (unless the props change).
// This Component can now be rendered hundreds/thousands of times on a page and still be performant
const MyButton = ({ parentOnClick }: { parentOnClick: (e: MouseEvent) => void }) => {
const buttonHandler = useCallback((e: MouseEvent) => {
// do something
// handle parent event
parentOnClick(e)
},[parentOnClick])
return (
<button onClick={buttonHandler}>
My button
</div>
)
}
Prefer children over components as props
Use children only if it's a 1:1 child being rendered (add Container example)
Add official
Use named props
Prefer children over components as props
If a child is just being directly rendered in the βbodyβ of your component. Use children.
// β Needlessly verbose and can have weird edge cases
const WrapWithEmoji = ({ ComponentToWrap, emoji }: { ComponentToWrap: JSX.Element, emoji: string }) => (
{' '}
<>
{emoji}
{ComponentToWrap}
{emoji}
</>
)
// β
Uses native react types. Handles children in native react way.
const WrapWithEmoji = ({ children, emoji }: PropsWithChildren<{ emoji: string }>) => (
{' '}
<>
{emoji}
{children}
{emoji}
</>
)
The exception to this rule
When you require component props to be used in unconventional layouts where the child isnβt obviously the βbodyβ of the component. Or when you have multiple components being used at once in specific ways.
// β children is being used in a non-universal way. Very error prone. Not obvious
// from the parent where the children are going
const MyLayout = ({ children, emoji }: PropsWithChildren<{ emoji: string }>) => (
{' '}
<header>{children[0]}</header>
My Cool Layout! {emoji}
<div>{children[1]}</div>
<footer>{children[2]}</footer>)
// β
Props clearly define what the conponent is using the child for. Doesnt have
// all the ReactChildren edge cases
type MyLayoutProps = {
Header: JSX.Element,
Body: JSX.Element,
Footer: JSX.Element,
emoji: string
}
const MyLayout = ({ Header, Body, Footer, emoji }: MyLayoutProps) => (
{' '}
<header>{Header}</header>
My Cool Layout! {emoji}
<div>{Body}</div>
<footer>{Footer}</footer>)
Donβt use array index as key
Rendered arrays in React use keys in order to save time on render. When a prop array that is rendered changes, React uses the key to figure out where to add/remove rows so that it doesnβt have to rerender the entire array. If you use the index as the key, the entire array needs to rerender whenever there is a change. See official docs for more info.
// β List completely rerenders on every change to listItems
const MyList = ({ listItems } : { listItems: string[]) => (
{' '}
<>
listItems.map((item, index) => <div key={index}>{item}</div>)
</>
)
// β
Assuming listItem strings are unique this works.
const MyList = ({ listItems } : { listItems: string[]) => (
{' '}
<>
listItems.map((item) => <div key={item}>{item}</div>)
</>
)
// Or defer ordering to parent
// β
Let the parent decide what unique key to use
type ListItem = {
item: string
key: string
}
const MyList = ({ listItems } : { listItems: ListItem[]) => (
{' '}
<>
listItems.map(({(item, key)}) => <div key={key}>{item}</div>)
</>
)
Avoid complex/nested ternaries in render loop
Complex conditionals or nested ternary statements make it hard for people to reason about what is rendering. Instead, use the early return pattern (AKA guard statements). In cases where early returns do not work. Consider encapsulated ternaries in useMemoand then treating them like components.
// β Logic is hard to follow. Lots of things are kinda repeated
const MyPage = () => {
const { loading, data, error} = useSomeQuery()
const { loading: loading2, data: data2, error: error2} = useSomeOtherQuery()
return (
<>
{ loading && !error2 ? <Loading/> : loading2 ? <Loading/> : <MyHappyPathComponent data={{...data, ...data2}}/> : null }
{ error || error2 ? error ? <DisplayError error={error}/> : <DisplayError error={error2}/> : null}
</>
)
}
// β
Way easier to read.
const MyPage = () => {
const { loading, data, error} = useSomeQuery()
if(error) return <DisplayError error={error}/>
if(loading) return <Loading/>
// Please dont actually rename variables like this.Doing it for the demo...
const { loading: loading2, data: data2, error: error2} = useSomeOtherQuery()
if(error2) return <DisplayError error={error2}/>
if(loading2) return <Loading/>
return (<MyHappyPathComponent data={{...data, ...data2}}/>)
}
// β
Another way to do it when logic is coupled between lots of states.
// Ideally you want to extract SomeQueryComponent and SomeOtherQueryComponent
// into their own component function if possible. If you can't, this is a
// clean and performant way of rendering it.
const MyPage = () => {
const { loading, data, error} = useSomeQuery()
const SomeQueryComponent = useMemo(() => {
if(error) return <DisplayError error={error}/>
if(loading) return <Loading/>
return <SomeComponent data={data}/>
}, [error,loading, data])
const SomeOtherQueryComponent = useMemo(() => {
if(error2) return <DisplayError error={error}/>
if(loading2) return <Loading/>
return <SomeComponent data={data2}/>
}, [error2,loading2, data2])
return (
<article>
<p>Some text</p>
<h2>Something dependant on data</h2>
{SomeQueryComponent}
<h2>Something dependant on data2</h2>
{SomeOtherQueryComponent}
</article>
)
}
Keep HTML as flat as possible
As a general rule, the less HTML on the page, the faster it is to render and the easier it is to debug. React provides fragments in order to facilitate grouping HTML without needing to use an HTML element.
// β The div likely does nothing here. It carries no styling, and makes using this
// component in the parent harder to style
const MyList = ({ listItems } : { listItems: string[]) => (
<div>
listItems.map((item) => <div key={item}>{item}</div>)
<div/>
)
// β
the "`<>`" dissapears on render leaving you with clean HTML
const MyList = ({ listItems } : { listItems: string[]) => (
{' '}
<>
listItems.map((item) => <div key={item}>{item}</div>)
</>
)
Use ternaries instead of &&
Usually when something is missing we want to render null or undefined. But by not expliciting returning null using a ternary, we could have to deal with some unexpected behaviour because, for example, in JS 0 && "hey" returns 0
// β This will render: <ul>0</ul>
function ContactList() {
const contacts = []
return (
<ul>
{contacts.length &&
contacts.map((contact) => (
<li key={contact.id}>
{contact.name}
</li>
))}
</ul>
)
}
// β
This will render <ul></ul>
function ContactList() {
const contacts = []
return (
<ul>
{contacts.length ?
contacts.map((contact) => (
<li key={contact.id}>
{contact.name}
</li>
): null}
</ul>
)
}
This also happens with "" which is a very common scenario.
See https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx for a more in-depth explanation