17 Frontend code reviews in real project (react, typescript)

mainly javascript/typescript and react, let's get start it

💎 1. Use date-fns instead of moment

moment weights 241kb which is so heavy, and before checking this PR, I've known that date-fns exists in this project so proposed this one.
Besides in our slack, I found engineers had already talked about it even long time ago, like We wanna remove moment someday

you can use no-restricted-imports option of ESLint in your project just like this

💎 2. Don't use magic number

there was like this code

const handleHogeChange = () => {
  setIsOpen(false)
  setTimeout(() => {
    // do something
  }, 170)
}

magic number in this code is 170, if other engineer see this code at first time, they couldn't get what it is asap.
↓

const HOGE_TRANSITION = 170

const handleHogeChange = () => {
  setIsOpen(false)
  setTimeout(() => {
    // do something
  }, HOGE_TRANSITION)
}

(actually this HOGE_TRANSITION is assigned in other const define file, but now I just wrote like that to get easily)

💎 3. DRY

Don't Repeat Yourself
If you don't know this one, you would find easily on the internet what it is

💎 4. Don't pass useState to child component

when you want to change state in child component, you shouldn't pass useState to child component directory. Because it makes so hard to get in which component this state is controlled.

↓ so just use callback function like this
https://www.webtips.dev/solutions/pass-data-from-child-to-parent-in-react

💎 5. Don't use type any in event argument in React

I found like this code

onClick={(e: any) => handleActionClick(e, action.id)}

and I thought engineer have tried code like that once

but as you can see error appeared and he gave up.
(just my imagination though lol)

React prepares own event for support cross browser, so you should use it
https://reactjs.org/docs/events.html#mouse-events

onClick={(e: React.MouseEvent) => handleActionClick(e, action.id)}

by the way, library preact which is similar to react doesn't implement like this code. It's one of the reason why it's lighter than react
https://preactjs.com/guide/v8/differences-to-react/

Synthetic Events: Preact's browser support target does not require this extra overhead

💎 6. Avoid hard coding

What is hard coding?
it means practice of embedding values directly into the source code of a program instead of using variables or other abstractions

useState('admin_user')

↓

const ADMIN_USER = 'admin_user'
useState(ADMIN_USER)

it is too simple to get for what I defined variable, but if you write code in big project, it would cause bug.

When I see hard code in project, my brain reacts instinctively like "🤨🤨🤨🤨🤨🤨🤨 hmmmmmmmm", even in train, bathroom, toilet, bed. So avoid it please lol

💎 7. UseState() of toggle button

// write in this way, it's simpler than write "true" and "false" each time
useState(prev => !prev)

💎 8. Use object not case in like this condition

I've modified like this way ↓
when conditionally rendering component based on some string enum (like a role), use an object to map enum values to components.

💎 9. Use meaningful Names

// (before)
const m = moment(value, valueFormat[type])

↓

// (after)
const inputDate = moment(value, valueFormat[type])

💎 10. Set default argument value, if it is possible. Specially in utility function

I found like this code about debounce

const DELAY = 200
.
.
.
const debouncedQuery = useDebounce(name, DELAY)

I was like "What is 200? Where is it from??", this related to Magic number which I've already written before.
After searching using places debouncedQuery, I realised the reason why he wrote 200 is that DELAY = 200 is written in 5 other files as well.
So I proposed adding default argument in useDebounce function like this

const useDebounce = (value, delay = 200) => {
....(some codes)
}

💎 11. Don't set JSX in useState value

I found like this code, but JSX shouldn't be managed in useState.
React decides to re-render or not by observing state(=data), but if JSX which is result of rendering sets in state, it would be like destroying react concept itself.
It works without any problem though

const [hogeComponent, setHogeComponent] = React.useState<JSX.Element>(hogeComponentRenderer)

💎 12. Add meaningful name in if condition

Actually it's not only if condition, but people somehow add meaningless name in if condition easily

// 🤔 What's that ...?
if(template.id > 0) {
...
}

↓

const existsTemplate = template.id > 0
if(existsTemplate) {
...
}

💎 13. Don't use negative condition, if you don't have reason

!isAdmin ? first() : second() 

↓

isAdmin ? second() : first()

💎 14. Eliminate unnecessary rendering

I've often seen people forgot adding useCallback in proper places.
Or one one big object of useState manages a lot of form.
If I found unnecessary rendering components which are caused by onChange event, I would warn it. It could be heavy action for browser.

How I find it?

I useReact developper tools in chrome.
↓ Check Highlight updates when components render, and you can see which component render

💎 15. use debounce

For example, when you implement input which request to server each onChange event, you have to add debounce. Otherwise, you know server cries.

In my case

In my project, I've found like this input(↑), and debounce time was 200ms. But Even if I wrote so quickly, requests were sent on each onChange event. So I proposed changing this time to 500ms

There are some examples, which you need to add debounce like animation, changing window size ....

💎 16. Use controlled and uncontrolled component depending on situation

When the form appears in code, I start to think about it.
Simply put ↓

- controlled
  you need to watch and render on every onChange event
- uncontrolled
  1. you don't need to watch and render on every onChange event, in case of simple form
  2. If user can add number of form, also you should use uncontrolled

💎 17. Don't manipulate DOM directly without special reason

I found like this code

const currentVal = form.querySelector('input[type="text"]').value

If you need to manipulate DOM, you should use useRef in react.js.
Specially when you need to change/get something that has no related to react life cycle, for example getting dom width, height or scroll height

You shouldn't use useRef only because you want to change some data

💎 (I will keep writing each time when I find interesting code review)

.
.
.
😀