[장바구니 협업 Step 2] 센트(김영우) 미션 제출합니다. #125
Conversation
There was a problem hiding this comment.
센트 안녕하세요~
뭔가 리코일로 끝장을 한번 봐본거 같아서 좋았습니다. 😄😄
어떠셨나요?
어디까지 리코일을 쓰고 어디까지 커스텀훅을 쓰고 이러한 센트 나름의 기준이 생기셨나요?
컴포넌트와 훅의 관계는 어떻게 하는게 좋더라 이런것도 좀 감이 생기셨을까요?
아리까리한 부분이 있었다면 코드에 코멘트를 남겨주시면 더 좋은 대화를 이어갈 수 있을거 같아요.
확인 후 궁금한 부분은 편하게 코멘트 남겨주세요~~
에러를 처리하는 보편적인 방식이 있는지, 또 그런 보편적인 방식은 어떻게 학습하는 것이 좋을지 궁금합니다!
이번 미션에서 에러를 처리하는 과정에서 Error Boundary를 처음 사용해보았는데, 어떤 에러를 어떻게 처리할까 고민이 많았습니다. 혹시 동동은 에러를 처리할 때 참고하는 에러 처리 방식이 있으신가요?? 이미 사용중인 서비스들에서 방식을 알아보고 싶었지만, 필요한 에러를 발생시키는 방법을 알 수 없어 관련 정보를 알아보기 힘들었습니다 😭😭
일단 전에 제가 했던 대답을 먼저 드립니다.
woowacourse/react-shopping-cart#170 (review)
다시 한번 말씀드려보면 에러 핸들링은 정책의 문제이기 때문에 어떻게 정책을 세우는지가 제일 중요합니다.
에러가 발생하면 유저에게 어떻게 보여주고 싶은가? 가 먼저고,
그거를 기술적으로 풀어나가는거죠~~
요즘 저희 팀 코드를 보면, 그냥 api에서 하나라도 에러라도 발생하면 바로 ErrorPage 를 보여주는 경우가 일반적인거 같아요. 어차피 하나라도 api가 제대로 동작을 안하면 제대로 된 결과를 보여주기가 힘드니까, ErrorPage를 빨리 보여주고 빨리 저희 팀에 노티가 오게끔 한답니다.
(물론 매우 사소한 UI 에러는 그 나름의 정책을 세우기도 한답니다 😄)
| @@ -70,6 +70,7 @@ | |||
| "gh-pages": "^5.0.0", | |||
| "msw": "^1.2.1", | |||
| "prop-types": "^15.8.1", | |||
| "react-error-boundary": "^4.0.8", | |||
There was a problem hiding this comment.
react-error-boundary 라이브러리를 사용하면,
직접 ErrorBoundary 컴포넌트를 구현하는 것에 비해
무엇이 편하다고 느끼셨어요? (궁금)
There was a problem hiding this comment.
사실 에러 바운더리를 직접 구현하기 위해선 클래스 컴포넌트를 사용해야 하는데 이 부분에서 시간이 모자라다고 느껴 라이브러리를 설치해 구현했었습니다! 한번 직접 구현해서 적용해보고자 했는데 그래도 시간이 조금 모자라네요 ㅠㅠ
여기에서 한번 구현해보고자 했는데 아직 반영을 하지 못했습니다
There was a problem hiding this comment.
에러바운더리 개념자체는 어려운게 아니니까요 한번 시도해보세요~ 화이팅~
public/favicon.png
Outdated
There was a problem hiding this comment.
파일명만 보면 이 파일은 favicon 용도인거 같은데, 실제로 favicon은 적용되어 있지 않네요?
There was a problem hiding this comment.
아하 저 부분을 적용해보려 하다가 마무리를 못하고 제출했나보네요! 마저 마무리 진행했습니다!
수정한 커밋: 40483fe
src/App.tsx
Outdated
| <Route | ||
| path="/" | ||
| element={ | ||
| <Suspense fallback={<LoadingPage />}> | ||
| <Main /> | ||
| </Suspense> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/cart" | ||
| element={ | ||
| <Suspense fallback={<LoadingPage />}> | ||
| <Cart /> | ||
| </Suspense> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/order" | ||
| element={ | ||
| <Suspense fallback={<LoadingPage />}> | ||
| <OrderList /> | ||
| </Suspense> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/orderDetail" | ||
| element={ | ||
| <Suspense fallback={<LoadingPage />}> | ||
| <OrderDetail /> | ||
| </Suspense> | ||
| } |
There was a problem hiding this comment.
각 페이지를 나타내는 컴포넌트에 모두 Suspense가 있어야 한다고 생각해서 위와 같이 작성을 했었는데 말씀을 듣고 보니 어차피 각 Suspense가 모두 같은 fallback을 가지고 있으니 Routes를 하나의 Suspense로 감싸도 괜찮겠다는 생각이 들어 그렇게 고쳐보았습니다!!
수정한 커밋: 6bea4bc
| isDetailPage, | ||
| totalProductPrice, | ||
| totalOrderLength, | ||
| }: OrderGroupProps) => { | ||
| const navigate = useNavigate(); | ||
|
|
||
| return ( | ||
| <Style.Container> | ||
| <Style.Header> | ||
| <Style.OrderNumber $isDetailPage={isDetailPage ?? false}> |
There was a problem hiding this comment.
| isDetailPage, | |
| totalProductPrice, | |
| totalOrderLength, | |
| }: OrderGroupProps) => { | |
| const navigate = useNavigate(); | |
| return ( | |
| <Style.Container> | |
| <Style.Header> | |
| <Style.OrderNumber $isDetailPage={isDetailPage ?? false}> | |
| isDetailPage = false, | |
| totalProductPrice, | |
| totalOrderLength, | |
| }: OrderGroupProps) => { | |
| const navigate = useNavigate(); | |
| return ( | |
| <Style.Container> | |
| <Style.Header> | |
| <Style.OrderNumber $isDetailPage={isDetailPage}> |
이렇게 하는건 어떤가요?
There was a problem hiding this comment.
오 제가 props에 기본값을 주는 방식을 많이 사용해보지 않아서 이렇게도 할 수 있는 것을 떠올리지 못했던 것 같습니다!! 해당 방식 활용해서 수정해보았습니다.
수정한 커밋: 023fe6c
There was a problem hiding this comment.
props에 기본값을 주는 것이랑 객체에 default parameter를 적용하는 것이나 완전히 동일한 것이죵
리액트라고 특별한게 아니니, JS 관련 아는 지식을 다 적용해보세요!
| </Style.ViewDetailButton> | ||
| </Style.Header> | ||
| {totalProductPrice ? ( | ||
| <Fragment> |
There was a problem hiding this comment.
| <Fragment> | |
| <> |
key prop 을 안쓸거면 굳이 Fragment라고 안써도 될거 같아요
(이렇게 쓰면 매우 old한 코드로 보인답니다. 이유는 .. 안알랴줌)
There was a problem hiding this comment.
이유는,
<></> 가 <Fragment></Fragment> 보다 나중에 나왔기 때문에
<Fragment></Fragment> 를 쓰면 예전 리액트 지식으로 개발하는 사람처럼 보이기 때문입니다 😄
src/mocks/orderHandlers.ts
Outdated
There was a problem hiding this comment.
msw + localStorage 로 서버 + DB를 모킹하려고 고생하셨네요.. 👏🏻 👏🏻 👏🏻
There was a problem hiding this comment.
감사합니다 ㅠㅠㅠ msw를 처음 써봐서 그냥 무지성으로 백엔드 로직을 다 구현해버리자는 생각을 했다가 몸이 고생해버렸네요😭😭
src/hooks/recoil/useCartRecoil.ts
Outdated
There was a problem hiding this comment.
-
개인적인 생각인데요,
서버에 POST나 PATCH 요청을 보내고 난 다음에
그냥 다시 GET을 요청해서 서버랑 싱크를 맞추는게 더 편한거 같아요. -
api 요청 횟수를 줄이려는 노력 자체가 의미가 없는건 아니지만,
api 요청 횟수에 너무 겁먹을 필요는 없습니다.
서버가 진정한 Single Source of Truth 아닐까요? -
POST 뒤에 또 GET 하는게 좀 부담스러우면 POST 의 응답으로 body에 값을 넣어달라고 하는 것도 방법이겠네용~
(저희 팀에서는 POST 와 PATCH의 응답에 항상 body로 값을 넣어서 주고 있습니다 ㅎㅎ..Location은 거의 안쓰는거 같네요.
파싱하기 귀찮기도 하고용)
근데 응답에 넣어줘도 또 GET 요청합니다 ㅎㅎㅎ
There was a problem hiding this comment.
앞서 이야기 했던 것처럼 클라이언트의 상태를 따로 두어 관리하고 있었기 때문에 PATCH나 POST 요청 후 GET을 하거나 반환되는 값을 이용해 렌더링을 진행하는 것은 처음 구현하고자 했던 코드의 방향성과 조금 맞지 않는다고 생각합니다!
하지만 혹시 동동이 이야기 해주고자 하셨던 것이 낙관적 업데이트를 진행한 이후에 반환되는 실제 서버 데이터로 클라이언트 상태를 업데이트 하라는 이야기였다면 이건 따로 고민을 한번 해봐야 할 것 같습니다! 지금 제 코드로는 서버의 상태와 클라이언트의 상태가 동일하다고 보장할 수 없을 것 같아보이네요 ㅠㅠ
아 그리고 Location을 사용했던 것은 이전 미션을 진행할 때 API 예상 명세를 그대로 이번 미션에서도 가져와 새로운 명세를 만들 때 반영했기 때문이었습니다!! (실제로는 거의 안쓴다고 하니 조금 슬프네요 ㅠㅠ)
There was a problem hiding this comment.
저는 낙관적 업데이트를 거의 안 쓰고 있어서요 ㅎㅎ.. 제가 말한건 낙관적 업데이트 얘기가 아니긴 했습니다~
대부분 서버의 응답이 굉장히 빠르기 때문에 대부분의 유저들은 그냥 서버 응답 올때 렌더링 해줘도 크게 불편함을 안느끼는거 같더라고요(뇌피셜)
그리고 시간이 오래걸리면 백엔드개발자분들한테 api latency 줄여덜라고 하는게 보통은 더 좋더라고요 ㅎㅎ
낙관적 업데이트를 하더라도 나중에 서버와 동기화시키는건 중요할거 같은데요? refetch 로 인한 타이밍이 꼬이는 이슈가 좀 있을 수 있는데,
서버가 SSoT 이므로, 백그라운드에서 열심히 싱크 맞춰주는 것도 좋을거 같습니다~ (리코일로는 어케할지 감이 잘 안오네요 ㅎㅎ)
src/hooks/fetch/useOrderFetch.ts
Outdated
| const order = (usingPoint: number) => { | ||
| return fetch(`${apiEndPoint}/orders`, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Basic ${base64}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| cartItemIds: selectedCartItems.map((cartItem) => cartItem.id), | ||
| originalPrice: selectedCartItems.reduce((acc, curr) => { | ||
| return (acc += curr.product.price * curr.quantity); | ||
| }, 0), | ||
| usedPoint: usingPoint ?? 0, | ||
| pointToAdd: selectedCartItems.reduce((acc, curr) => { | ||
| const earnedPoint = | ||
| (curr.product.price * curr.quantity * curr.product.pointRatio) / | ||
| 100; | ||
| return (acc += earnedPoint); | ||
| }, 0), | ||
| }), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
body를 만드는 부분과, 이를 fetch하는 부분을 나누는게 더 유지보수에 좋습니다.
계산과 액션 나누기~!!
There was a problem hiding this comment.
말씀해주신 부분 수정해서 반영 완료했습니다!!
수정한 커밋: ac66584
| import { useState } from 'react'; | ||
|
|
||
| export const usePointInputHandler = (canUsingUserPoint: number) => { | ||
| const [usingPoint, setUsingPoint] = useState<number>(0); |
There was a problem hiding this comment.
| const [usingPoint, setUsingPoint] = useState<number>(0); | |
| const [usingPoint, setUsingPoint] = useState(0); |
There was a problem hiding this comment.
수정 완료했습니다!
수정한 커밋: bda2fad
| if (e.target.checked) | ||
| return setSelectedCartIdList(() => getAllCartIdList()); |
There was a problem hiding this comment.
| if (e.target.checked) | |
| return setSelectedCartIdList(() => getAllCartIdList()); | |
| if (e.target.checked) { | |
| setSelectedCartIdList(() => getAllCartIdList()); | |
| return; | |
| } |
센트가 작성한 코드와 제가 위에 작성한 코드나 하는 일은 똑같습니다.
하지만 위와 같이 코드에서 개발자의 의도를 명확하게 드러내주는 것이 대부분의 경우에 더 좋다고 생각해요.
return은 함수의 종료를 의미하기도 하지만 어떤 값을 반환하는지도 나타내니까요~!
There was a problem hiding this comment.
아하!! 감사합니다! 그동안 위 코드처럼 작성해도 문제 없이 동작했어서 코드 길이도 줄일 겸 계속해서 저런 방식으로 작성했었는데 return문 때문에 코드를 오해할 수도 있으니 동동이 알려주신 대로 코드를 작성해봐야겠어요! 말씀해주신 내용 반영 완료했습니다!
수정한 커밋: 1c07910
|
동동!! 정성스러운 리뷰 감사드립니다! 중간중간 제 역량 부족으로 완벽히 리뷰 내용을 반영하지 못한 부분이 있는데 이 부분 빠른 시일 내로 반영해보도록 하겠습니다 ㅠㅠ |
안녕하세요 동동!! 센트입니다😀😀 이번 장바구니 협업 미션 리뷰 잘 부탁드립니다👍
지난번에 리뷰요청이 늦어져 너무 죄송했습니다 ㅠㅠ 이번에는 말씀해주신 대로 draft를 먼저 올려봤습니다!
🔗 배포 페이지
🎯 학습 목표
이번 미션에서는 백엔드와 처음으로 협업을 해본 만큼 기능이 모두 잘 동작할 수 있도록 하는 것을 목표로 미션을 진행했습니다.
🧑💻 코드 설명
사용자의 상태가 변경됨에 따라 요청을 함께 진행하도록 코드를 작성해보았고, 그렇기 때문에 사용자는 서버의 응답을 기다리지 않고 바로바로 변경되는 화면을 볼 수 있도록 하고자 했습니다! 그래서 요청, 리코일 변경을 따로 진행하는 훅을 만들어보았습니다!
서버 변경을 구현하기 위해서 서버를 선택하면 리코일 상태로 지정되어 있는 apiEndPoint를 변경하도록 하고, 해당 값이 변경됨에 따라 atomFamily, selectorFamily로 필요한 값들을 가져올 수 있도록 코드를 작성해보았습니다!
hooks/hooks 폴더 하위에는
fetch,recoil로 폴더를 나누어 각각 서버에 요청을 넣는 훅, 리코일 상태를 변경하는 훅을 나누어 보았습니다.recoil/장바구니, 주문 목록, 메인 페이지, 유저 적립금 정보 등을 recoil 상태로 관리하기 위해 각각 해당하는 atom, selector를 만들어주었습니다.
에러를 처리하는 보편적인 방식이 있는지, 또 그런 보편적인 방식은 어떻게 학습하는 것이 좋을지 궁금합니다!
이번 미션에서 에러를 처리하는 과정에서 Error Boundary를 처음 사용해보았는데, 어떤 에러를 어떻게 처리할까 고민이 많았습니다. 혹시 동동은 에러를 처리할 때 참고하는 에러 처리 방식이 있으신가요?? 이미 사용중인 서비스들에서 방식을 알아보고 싶었지만, 필요한 에러를 발생시키는 방법을 알 수 없어 관련 정보를 알아보기 힘들었습니다 😭😭