[클린코드 리액트 2기 양세영] 장바구니 미션 Step 1 #3
Conversation
There was a problem hiding this comment.
안녕하세요 세영님! 장바구니 미션을 함께하게 된 리뷰어 황준일입니다 🙏
리뷰가 무척 늦어졌네요 ㅠㅠ 죄송합니다... 요즘 회사 일에 치이다보니 여유가 너무 없네요 ㅎㅎ;
1. 성능 최적화가 필요합니다.
일단 전체적으로 성능 최적화를 신경쓰시면 좋을 것 같아요.
useMemo, useCallback, memo 등을 적극적으로 사용해보시면 어떨까요?
컴포넌트 제대로 만들기 아티클을 참고해보시면 좋을 것 같아요!
이 외에도 리뷰어 슬랙 채널에서 이야기 나눴던 메세지들을 참고해보세요!
2. 사이드 이펙트 관리가 필요합니다.
지금 api를 컴포넌트 내에서 직접 하고 있어요. 그런데 데이터를 관리하는 방식은 굉장히 다양합니다. 가령, localStorage, sessionStorage 같은 것들을 이용할 수도 있고, cookie를 사용할 수도 있고, 그냥 로컬 변수로 선언해서 관리할 수도 있고, 지금처럼 rest api가 아니라 graphql을 이용할 수도 있습니다.
그런데 컴포넌트는 이런 것들에 관심있을까요?
컴포넌트는 단순하게 데이터를 토대로 UI를 그려주기만 하면 되지 않을까요?
컴포넌트가 컴포넌트답게 사용되기 위해선 지금의 코드를 어떤식으로 개선하면 좋을지 고민해보시면 좋답니다!
컴포넌트도 그렇고, 함수도 그렇고, "사이드 이펙트"를 기준으로 나눠서 관리해보면 좋아요.
쏙쏙 들어오는 함수형 코딩 이라는 책에 보면, 사이드 이펙트가 있는 함수를 "액션"이라고 이야기하고, 사이드 이펙트가 없는 함수를 "계산" 이라고 이야기합니다.
그래서 "액션"들은 묶어서 관리하면 좋고, "계산"들도 묶어서 관리하면 좋아요 ㅎㅎ
컴포넌트도 마찬가지입니다.
사이드 이펙트가 있는 컴포넌트들끼리 묶어서 관리하고,
사이드 이펙트가 없이 독립적으로 쓰일 수 있는 컴포넌트들끼리 묶어서 관리하는거죠!
어떤 내용인지 감이 오시나요?
3. 상태 유지하기
단순하게 msw를 사용한다고 해서 상태가 유지되는건 아니랍니다. msw의 역할은 api 호출을 모킹하는 역할을 수행하는거라서요! 그래서 msw로 내려주는 데이터를 아예 localStorage 같은 곳에 담아두고 이용하면 어떨까요? post 요청을 보내면 msw에서 localStorage에 데이터를 쓰고, get을 하면 msw에서 localStorage에 있는 데이터를 가져오는거죠 ㅎㅎ
api -> msw -> localStorage
이런 흐름이랍니다!
이 외에 자세한 내용은 전체적인 리뷰를 참고해주시면 좋을 것 같습니다!
궁금한점은 언제든 코멘트 혹은 슬랙으로 질문 주세요 😁
There was a problem hiding this comment.
리팩토링한 부분
- 컴포넌트를 순수함수로 모두 수정.
- react query 의 형태를 참고해서 hook을 만들었고 error 핸들링을 추가.
- 로컬스토리지를 이용해 카트 추가 기능 수정.
- axios instance 추가.
- 전체적으로 데이터 및 로직을 다루는 부분들을 custom hook으로 분리.
궁금한 부분
- custom hook으로 잘 분리한게 맞는지 궁금합니다!
- 그리고 api 코드들이 들어가니깐 폴더 구조에 대해서 고민이 많아졌는데 관리하기 편한 폴더구조에 대해서 어떻게 생각하시는지 궁금합니다!
- 에러 핸들링을 이렇게 처리만 하면 되는게 맞을까요? axios의 interceptor로 에러처리를 해줄수도 있던데 이러한 작업들까지 들어가야 완벽한지 궁금합니다!(Home.tsx, Cart.tsx)
그리고 성능최적화는 아직 적용못해봤는데 step2에서 개선시켜보겠습니다...ㅠ_ㅠ
There was a problem hiding this comment.
안녕하세요 세영님! 전체적으로 리팩토링 잘 해주셨네요 👏👏
특히 대부분의 사이드이펙트가 분리되어 관리되고 있어서 무척 보기 좋습니다 👍
Step1은 여기서 마무리하도록 하겠습니다!
질문에 대한 답변
custom hook으로 잘 분리한게 맞는지 궁금합니다!
굉장히 잘 분리해주셨습니다!
다만 모든 hook을 하나의 폴더에서 관리하고 있는데요, hook이 쓰이는 범위를 생각해서 위치해주시면 좋을 것 같아요 ㅎㅎ
스코프를 지정한다는 느낌으로여!
그리고 api 코드들이 들어가니깐 폴더 구조에 대해서 고민이 많아졌는데 관리하기 편한 폴더구조에 대해서 어떻게 생각하시는지 궁금합니다!
저같은 경우, apiClient -> service -> hook 이런 방식으로 만들어서 사용하고, 이에 따라 폴더도 따로따로 만들어서 관리하는 편입니다. 중요한건 의존성이 한 방향으로 흘러야 한다는 점이고, 똑같은 레이어에 있는 것들끼리 묶어주는거죠!
에러 핸들링을 이렇게 처리만 하면 되는게 맞을까요? axios의 interceptor로 에러처리를 해줄수도 있던데 이러한 작업들까지 들어가야 완벽한지 궁금합니다!(Home.tsx, Cart.tsx)
axios interceptor는 error의 형태를 정제하는 정도로 생각해주시면 좋을 것 같아요. 아마 세영님의 의도랑 interceptor의 역할은 맡지 않을 것 같습니다 ㅎㅎ 그래서 그냥 지금처럼 관리해도 무방해보입니다!
조금 더 나아가서, error에 대한 ui를 만들어서 관리해주면 좋아요.
(예시toast나 alert 컴포넌트를 만들어서 띄운다)
네이밍 관련
지금 전체적으로 타입에 대한 네이밍이 중구난방으로 되어있어요.
타입 컨벤션을 정해서 일관적인 방식으로 네이밍을 해주면 좋을 것 같습니다.
나머지는 피드백 내용을 확인해주세요!
|
|
||
| export const updateCartList = (payload: CartItemType | undefined) => | ||
| axiosRequest({ | ||
| url: `${process.env.REACT_APP_API_PATH}/carts`, |
There was a problem hiding this comment.
axiosRequest({
url: `/carts`,axiosRequest에서 baseURL을 지정해놨기 때문에, path만 작성해도 될 것 같아요!
| const axiosRequest = axios.create({ | ||
| baseURL: process.env.REACT_APP_API_PATH, | ||
| }); | ||
|
|
||
| export default axiosRequest; |
There was a problem hiding this comment.
조금 더 구체적으로.. httpRequest 는 어떤가요!?
import axios from 'axios';
const httpRequest = axios.create({
baseURL: process.env.REACT_APP_API_PATH,
});
export default httpRequest;
| const SvgCart = ({ color }: SvgType) => { | ||
| return ( | ||
| <svg | ||
| width="100%" | ||
| height="100%" | ||
| viewBox="0 0 31 27" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <path | ||
| d="M27.6829 21.1472L30.4512 6.06599C30.4578 5.9992 30.4578 5.93169 30.4512 5.86491C30.4512 5.6427 30.3783 5.42959 30.2485 5.27246C30.1187 5.11533 29.9426 5.02706 29.7591 5.02706H7.57127L6.58852 0.594872C6.54536 0.422401 6.4574 0.271377 6.33772 0.164216C6.21803 0.0570544 6.073 -0.000531902 5.92414 3.70261e-06H0.692072C0.508523 3.70261e-06 0.332492 0.0882762 0.202703 0.245402C0.0729145 0.402528 0 0.615637 0 0.837847C0 1.06006 0.0729145 1.27317 0.202703 1.43029C0.332492 1.58742 0.508523 1.67569 0.692072 1.67569H5.39816L9.68901 21.1891C9.71797 21.3059 9.76766 21.4137 9.8346 21.5048C9.90154 21.596 9.98412 21.6684 10.0766 21.7169C9.81818 22.244 9.68413 22.8467 9.68901 23.4596C9.68901 24.3484 9.98066 25.2009 10.4998 25.8294C11.019 26.4579 11.7231 26.811 12.4573 26.811C13.1915 26.811 13.8956 26.4579 14.4148 25.8294C14.9339 25.2009 15.2256 24.3484 15.2256 23.4596C15.2226 22.8702 15.0913 22.2922 14.8449 21.7839H22.5269C22.2806 22.2922 22.1493 22.8702 22.1463 23.4596C22.1463 24.3484 22.438 25.2009 22.9571 25.8294C23.4763 26.4579 24.1804 26.811 24.9146 26.811C25.6488 26.811 26.3529 26.4579 26.8721 25.8294C27.3912 25.2009 27.6829 24.3484 27.6829 23.4596C27.6807 22.8407 27.537 22.2346 27.2676 21.7085C27.3683 21.6607 27.4582 21.585 27.5303 21.4875C27.6024 21.3901 27.6547 21.2735 27.6829 21.1472ZM24.8385 6.70275H28.894L28.2781 10.0541H24.534L24.8385 6.70275ZM7.93807 6.70275H12.5265L12.831 10.0541H8.67858L7.93807 6.70275ZM9.79282 15.0812L9.0523 11.7298H12.9902L13.3016 15.0812H9.79282ZM10.1665 16.7569H13.4539L13.7584 20.1082H10.914L10.1665 16.7569ZM12.4573 25.1353C12.1835 25.1353 11.9159 25.037 11.6883 24.8529C11.4607 24.6688 11.2833 24.4071 11.1785 24.1009C11.0737 23.7947 11.0463 23.4578 11.0997 23.1327C11.1532 22.8076 11.285 22.5091 11.4786 22.2747C11.6721 22.0404 11.9188 21.8808 12.1873 21.8161C12.4558 21.7515 12.7341 21.7846 12.987 21.9115C13.2399 22.0383 13.4561 22.2531 13.6082 22.5286C13.7603 22.8042 13.8414 23.1282 13.8414 23.4596C13.8414 23.904 13.6956 24.3302 13.436 24.6445C13.1765 24.9588 12.8244 25.1353 12.4573 25.1353ZM17.9939 20.1082H15.1495L14.8449 16.7569H17.9939V20.1082ZM17.9939 15.0812H14.6927L14.3813 11.7298H17.9939V15.0812ZM17.9939 10.0541H14.229L13.9245 6.70275H17.9939V10.0541ZM22.2224 20.1082H19.378V16.7569H22.5269L22.2224 20.1082ZM22.6861 15.0812H19.378V11.7298H22.9906L22.6861 15.0812ZM23.1498 10.0541H19.378V6.70275H23.4474L23.1498 10.0541ZM24.9146 25.1353C24.6408 25.1353 24.3732 25.037 24.1456 24.8529C23.918 24.6688 23.7406 24.4071 23.6358 24.1009C23.531 23.7947 23.5036 23.4578 23.557 23.1327C23.6104 22.8076 23.7423 22.5091 23.9359 22.2747C24.1294 22.0404 24.3761 21.8808 24.6446 21.8161C24.9131 21.7515 25.1914 21.7846 25.4443 21.9115C25.6972 22.0383 25.9134 22.2531 26.0655 22.5286C26.2176 22.8042 26.2987 23.1282 26.2987 23.4596C26.2987 23.904 26.1529 24.3302 25.8933 24.6445C25.6338 24.9588 25.2817 25.1353 24.9146 25.1353ZM26.4371 20.1082H23.6135L23.918 16.7569H27.0531L26.4371 20.1082ZM24.0772 15.0812L24.3886 11.7298H27.9805L27.3645 15.0812H24.0772Z" | ||
| fill={color ?? COLORS.BLACK} | ||
| /> | ||
| </svg> | ||
| ); | ||
| }; | ||
|
|
||
| export default SvgCart; |
There was a problem hiding this comment.
| const mq = (cssObject: CSSObject) => { | ||
| const facePaintFn = facepaint( | ||
| breakpoints.map((bp) => `@media (min-width: ${bp}px)`) | ||
| ); | ||
| return facePaintFn(cssObject); | ||
| }; | ||
|
|
||
| export default mq; |
| // media query | ||
| const mq = (cssObject: CSSObject) => { | ||
| const facePaintFn = facepaint( | ||
| breakpoints.map((bp) => `@media (min-width: ${bp}px)`) |
There was a problem hiding this comment.
bp처럼 줄여쓰는 것도 좋은 방식은 아닌 것 같네요 ㅠㅠ breakpoint 처럼 풀네임으로 적어주면 어떨까요?
| <Section> | ||
| <header className="flex-col-center mt-50 mb-50"> | ||
| <h2 className="cart-section__title">{text}</h2> | ||
| <hr className="divide-line mt-20" /> | ||
| </header> | ||
| </Section> |
There was a problem hiding this comment.
SectionTitle 이라서 Section내부에 들어갈줄 알았는데, 반대로 SectionTitle 안에 Section이 있는 구조네요..! 사용하는 입장에서 헷갈리지 않을까요?
| imageUrl, | ||
| onClickProductImage, | ||
| onClickAddCart, | ||
| }: ProductType) => { |
| products, | ||
| navigateToDetailedPage, | ||
| addCart, | ||
| }: SectionProductListProps) => { |
|
|
||
| interface SectionProductListProps { | ||
| products: ProductType[]; | ||
| navigateToDetailedPage: (num: number | undefined) => void; |
There was a problem hiding this comment.
navigateToDetailedPage: (num?: number) => void;이렇게 optional을 활용할 수도 있답니다~
| <ProductItem | ||
| key={item.id} | ||
| imageUrl={item.imageUrl} | ||
| name={item.name} | ||
| price={item.price} | ||
| onClickProductImage={() => navigateToDetailedPage(item.id)} | ||
| onClickAddCart={() => addCart(item)} | ||
| /> |
There was a problem hiding this comment.
<ProductItem
key={item.id}
{...item}
onClickProductImage={() => navigateToDetailedPage(item.id)}
onClickAddCart={() => addCart(item)}이렇게 표현할 수 있답니다!
| const axiosRequest = axios.create({ | ||
| baseURL: process.env.REACT_APP_API_PATH, | ||
| }); | ||
|
|
||
| export default axiosRequest; |
There was a problem hiding this comment.
조금 더 구체적으로.. httpRequest 는 어떤가요!?
import axios from 'axios';
const httpRequest = axios.create({
baseURL: process.env.REACT_APP_API_PATH,
});
export default httpRequest;
| import { setupWorker } from 'msw'; | ||
| import { handlers } from './handlers'; | ||
|
|
||
| export const worker = setupWorker(...handlers); |
There was a problem hiding this comment.
setupBrowerWorker.ts 이렇게 수정하면 좀 덜 어색할까요!?
| import SectionTitle from '../components/common/SectionTitle'; | ||
| import SectionCartList from '../components/cart/SectionCartList'; | ||
| import Layout from '../layout/Layout'; | ||
| import useCart from '../hooks/useCart'; | ||
| import { CartItemType } from '../types'; |
There was a problem hiding this comment.
넵 수정했습니다! 그런데 이부분 관련해서 궁금한 점이 있는데요!
- import로 묶어주게 되었을 때 코드가 깔끔해지는 것 외에 또 장점이 있는지 궁금합니다!
- 그리고 확장성을 생각한다면 모든 폴더에 index.tx를 추가해서 묶는것이 좋을까요!?
| interface SectionCartListProps { | ||
| carts: CartItemType[]; | ||
| } |
There was a problem hiding this comment.
전체적으로 네이밍을 모두 수정했습니다
props type은 "Props"를 suffix로,
단순 data type은 "Type"을 suffix로 적용했습니다
그리고 추가로 궁금한점이 있는데요!
준일님은 현업에서 type 네이밍을 어떤식으로 하시나요!?
Pull Request Template
안녕하세요 준일님! PR 메세지 부족한 부분이 있어 조금 수정했습니다. 장바구니 미션 잘부탁드립니다!
1. 가이드
1-1. 배포
1-2. 로컬 환경
2. 리뷰 요청 ✍️
2-1. 어렵거나 아쉬웠던 점
로컬 상태에서 msw handler를 이용해서 상태 유지가 가능한 mocking api를 만들고 싶은데 어떻게 해야할지 잘 모르겠습니다.. 카트에 아이템을 추가하려면 전역 상태를 만들어야하는데 context api로 시도했을 때는 hook는 jsx function 내부에서만 사용가능해서 에러가 뜨고, lowdb로도 한번 시도해봤는데 handler 로직 내부에 넣는 순간 에러가 나더라구요. 이 부분은 어떻게 해결 가능할까요!?
비동기 함수를 useEffect에서 호출하다보니 "Promise returned from fetchProduct is ignored" 이라는 주의창때문에 반환값으로
Promise<void>로 타입을 지정해줬는데도 해결이 안되더라구요. 혹시 이 부분은 타입을 어떻게 설정해주는 것이 좋을까요?(SectionProductList.tsx : 43)2-2. 나누고 싶은 고민
2-3. 중점적으로 리뷰받고 싶은 부분
api 관련 코드들이 익숙하지 않아 어떻게 api 파일을 나누고 사용해야 관리가 편한지? 에러처리는 이렇게 하는 것이 맞는지? 몰라서 어려웠습니다. 그래서 api 폴더에 있는 코드들과 해당 함수들이 사용되는 흐름에서 유지보수와 에러처리 관점에서 리뷰 부탁드립니다!
코드 가독성을 위해 폴더구조와 네이밍에 신경을 썼는데 가독성 떨어지는 부분에 대해 의견 부탁드립니다!
2-4. 새로운 시도 혹은 미션을 통해 도전한 점
3. 질문있어요 🙋
4. 요구 사항 ✅
4-1. 필수 요구사항
MSW를 활용한
API mockingEndpoint만 변경하면 언제든Real API를 바라볼 수 있다고 가정하고 상상합니다.Real API없이 로컬에서만 동작하는 상태로 리뷰 받는 것이 기본 원칙입니다.로고를 누르면 상품목록 페이지로 이동한다.장바구니버튼을 누르면 장바구니 페이지로 이동한다.주문목록버튼을 누르면 주문목록 페이지로 이동한다.상품들은 n x 4 레이아웃으로 보여진다.
상품들에는 사진, 이름, 금액이 보여진다.
장바구니 버튼을 클릭하면 (**)
4-2. 선택 요구사항 (심화)
상품상세