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)
.
.
.
๐
Top comments (0)