[1단계 - 장바구니] 리바이(성은우) 미션 제출합니다. #351
Conversation
Co-authored-by: 이상희 <[email protected]>
Co-authored-by: 이상희 <[email protected]>
Co-authored-by: 이상희 <[email protected]>
Co-authored-by: 이상희 <[email protected]>
Co-authored-by: 이상희 <[email protected]>
Co-authored-by: 이상희 <[email protected]>
- 백엔드 API Patch 로직 - context 로직 Co-authored-by: 이상희 <[email protected]>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
안녕하세요 리바이! 레벨1때도 다양한 시도를 해주신 것으로 기억하는데요! 이번 미션도 역시 많은 시도를 해주신 것 같아 재미있게 리뷰할 수 있었어요!!
테스트 코드에 대해서
현업에서도 MSW + Vitest/RTL 조합 많이 써요! 다만 현재 테스트는 한 번에 너무 많은 걸 검증하려고 해서 나중에 유지보수가 힘들 수 있어요. 비즈니스 로직 테스트와 UI 인터랙션 테스트를 분리하면 훨씬 안정적이고 디버깅하기 쉬운 테스트를 만들 수 있을 거예요. 특히 그 early return 부분은 꼭 수정하시는 걸 추천드려요.
성능 최적화 관련
lazy loading은 좋은 시도인데, 현재 컴포넌트 크기로는 효과가 미미할 것 같아요. 그보다는 Context value를 useMemo로 감싸서 불필요한 리렌더링을 방지하는 게 더 체감 효과가 클 거예요. React DevTools Profiler로 한번 측정해보시면 어디서 불필요한 렌더링이 발생하는지 금방 보일 거예요.
전체적으로 정말 탄탄하게 구현하셨고, 특히 고민한 흔적들이 많이 보여서 좋았습니다. 이런 식으로 계속 "왜?"를 고민하면서 개발하시면 좋을 것 같아요! 이번미션도 너무 수고많으셨고 코멘트 확인해주시고 재요청 주세요~~
| const { selectedCartItems } = useSelectedCartContext(); | ||
|
|
||
| const totalPrice = selectedCartItems.reduce((acc, item) => acc + item.product.price * item.quantity, 0); | ||
| const deliveryFee = totalPrice >= 100000 ? 0 : 3000; |
There was a problem hiding this comment.
요거는 도메인에 중요한 숫자라 판단되어 상수로 분리해주셔도 좋을 것 같아요
__test__/cart.test.tsx
Outdated
| }); | ||
| it('장바구니에 상품이 하나도 없을 때 EmptyCartItemUI가 잘 보이는지', async () => { | ||
| const cartItemCards = await screen.findAllByTestId('cart-item-card'); | ||
| if (cartItemCards.length !== 0) return; |
There was a problem hiding this comment.
@JUDONGHYEOK
장바구니에 상품이 들어있을 경우에는 해당 테스트가 빈 장바구니가 아니므로 계속 테스트를 실패해서 cardItemCards.length가 0보다 클 때만 실행하도록 하였습니다!
사실 해당 로직을 넣었을 경우
const emptyMessage = await screen.findByText('장바구니에 담은 상품이 없습니다.');
expect(emptyMessage).toBeInTheDocument();해당 테스트가 실행이 안됨으로 아무 의미가 없어지는데, 고민을 하였을 땐 selectedItems에 대해서 빈 배열만 return해주는 api를 따로 만들어야 할까? 라는 아이디어와, App컴포넌트 대신 빈 배열을 가져오는 따른 TestApp 같은것을 만들어야 하나라는 생각해 해봤습니다!
이러한 경우에는 동키콩은 어떻게 해결할 수 있는지 궁금합니다..!
There was a problem hiding this comment.
결론적으로 빈 배열을 반환하는 새로운 App 컴포넌트을 구현하여 빈 장바구니 테스트 케이스에 적용하였습니다!
commit: 5e04047
There was a problem hiding this comment.
그렇게 하는 것도 방법일 수 있겠지만 유지보수하는데 추가적인 비용이 발생할 것 같아요. 만약에 저라면 msw를 활용하고 있으니 API모킹을 테스트에도 활용해볼 수 있을 것 같아요 명시적으로 장바구니에 상품이 들어있는 경우와 들어있지 않는 경우를 나눠서 관리하면 테스트 안정성도 올라가지 않을까요?
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export const SelectedCartProvider = ({ children }: SelectedCartProviderProps) => { |
There was a problem hiding this comment.
선택 상태만 Context로 관리하고 cartItems는 컴포넌트 상태로 관리해주신 이유가 있으실까요??
There was a problem hiding this comment.
PR내용에서 설명한거와 같이 해당 부분에 대해서 많은 고민을 했었는데 현재 서비스(미션)에서는 장바구니 페이지라는 특성 상 selected된 상품들만 전역적으로 쓰일 것 같아서 해당 방법 택하였습니다!
하지만 서비스가 확장되고 cartItems또한 여러 페이지와 컴포넌트에서 쓰인다면 cartItems 전체를 context로 전역적으로 state들을 관리하는게 더 낫다고 생각합니다!
There was a problem hiding this comment.
저라면 사실 관리의 이점때문에 selected필드를 cart에 같이 저장하거나 ids만 관리하여 파생상태로 관리할 수 있도록 구현했을 것 같은데요. 이 부분은 기능의 확장이 어떤 방향으로 가느냐에 따라 달라질 수 있는 부분이 존재할 것 같아서 유지해주셔도 괜찮을 것 같습니다!
There was a problem hiding this comment.
해당 피드백 감사합니다! 현재는 cartItems도 전역으로 다루도록 리팩토링 하였습니다!
| expect(emptyMessage).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('장바구니에서 체크박스를 누르면 배송비를 고려하여 해당 금액이 반영된다.', async () => { |
There was a problem hiding this comment.
현재 해당테스트를 보니, 한 번의 테스트에서 너무 많은 것들을 검증하려고 하는 것 같아요. DOM 요소 존재 확인부터 시작해서 텍스트 파싱, 숫자 변환, 배송비 계산 로직, UI 상호작용, 그리고 최종 결과 검증까지 모든 게 하나의 테스트에 들어가 있더라고요.
이렇게 되면 테스트가 실패했을 때 정확히 어느 부분에서 문제가 발생했는지 파악하기 어려워질 수 있고, 나중에 비즈니스 로직이 바뀌거나 UI가 변경될 때 테스트 유지보수도 힘들어질 것 같습니다. 예를 들어 배송비 정책만 바뀌었는데 DOM 구조 파싱하는 부분까지 영향받을 수 있잖아요.
개인적으로는 이런 복합적인 기능은 단위별로 쪼개서 테스트하는 게 어떨까 싶어요. 체크박스 클릭 동작 자체는 따로 테스트하고, 배송비 계산 로직도 별도로 테스트하고, 그다음에 통합 플로우에서는 "사용자가 이런 행동을 했을 때 최종 결과가 이렇게 나온다"는 정도만 검증하는 식으로요. 그러면 각 테스트의 목적도 명확해지고, 실패했을 때 디버깅도 훨씬 수월할 것 같습니다.
There was a problem hiding this comment.
테스트에 대한 피드백 감사합니다! 고려해서 전반적으로 테스트코드를 더 명료하게 리팩토링해보겠습니다!
There was a problem hiding this comment.
오 넵 expect문이 여러개가 되는 것은 문제가 되지 않습니다만 지금 상황에서는 너무많은 expect가 반복되어서 분리되면 좋을 것 같았습니다!
| }; | ||
|
|
||
| return ( | ||
| <SelectedCartContext.Provider |
There was a problem hiding this comment.
context에서는 value가 바뀌면 하위컴포넌트 모두가 리렌더링 되기 때문에 메모이제이션에 특히 더 신경쓰면 좋을 것 같습니다!
또 하위 컴포넌트 중 복잡도가 있는 컴포넌트라면 memo를 활용하시는 것도 좋은 방법입니다!
There was a problem hiding this comment.
오.. 상당히 도움되는 피드백이네요! 좋은 피드백 감사합니다 😮
| const existingItemIndex = prevItems.findIndex((item) => item.id === cartItem.id); | ||
|
|
||
| if (existingItemIndex > -1) { | ||
| const updatedItems = [...prevItems]; | ||
| updatedItems[existingItemIndex].quantity = quantity ?? 1; | ||
| return updatedItems; | ||
| } |
There was a problem hiding this comment.
| const existingItemIndex = prevItems.findIndex((item) => item.id === cartItem.id); | |
| if (existingItemIndex > -1) { | |
| const updatedItems = [...prevItems]; | |
| updatedItems[existingItemIndex].quantity = quantity ?? 1; | |
| return updatedItems; | |
| } | |
| const exists = prevItems.some(item => item.id === cartItem.id) | |
| if (exists) { | |
| return prevItems.map(item => | |
| item.id === cartItem.id | |
| ? { ...item, quantity: updatedQuantity ?? 1 } | |
| : item | |
| ) | |
| } |
요기도 간단하게 some으로 요렇게 구현하는 건 어떠세요?
There was a problem hiding this comment.
@JUDONGHYEOK
해당 피드백 감사합니다!
코드 효율성에 대해서 같이 얘기하고 싶습니다!
제가 구현한 코드에서는
const updateSelectedCartItem = (cartItem: CartItem, quantity?: number) => {
setSelectedCartItems((prevItems) => {
const existingItemIndex = prevItems.findIndex((item) => item.id === cartItem.id);
if (existingItemIndex > -1) {
const updatedItems = [...prevItems];
updatedItems[existingItemIndex].quantity = quantity ?? 1;
return updatedItems;
}
return [...prevItems, cartItem];
});
};findIndex 메소드 통해서 한 번만 순회하면 해당 idnex를 이용해서 해당 로직을 처리할 수 있지만
some 메소드를 사용할 경우
const updateSelectedCartItem = (cartItem: CartItem, quantity?: number) => {
setSelectedCartItems((prevItems) => {
const existing = prevItems.some((item) => item.id === cartItem.id);
if (existing) {
return prevItems.map((item) =>
item.id === cartItem.id ? { ...item, quantity: quantity !== undefined ? quantity : item.quantity } : item
);
}
return [...prevItems, cartItem];
});
};some메소드와 map 메소드를 사용하여 두 번을 순회해야할 것 같아서 해당 부분에서는 findIndex가 성능 측면에서 현재 상황에서는 미미할 수도 있겠지만 더 효율적이라고 생각하는데 혹시 동키콩은 어떻게 생각하시나요!
There was a problem hiding this comment.
오 좋은 관점이네요! 하지만 말씀하신대로 성능상의 이점이 그렇게 큰가를 고민해볼 필요는 있을 것 같아요. 현재 보여주신 코드는 어떻게 할지 설명하는 명령적인 방식이라 코드의 가독성이 떨어진다고 생각했었거든요. 언제나 트레이드오프는 존재하니 적절하게 고려하면 좋을 것 같습니다! 이부분은 그렇구나 하고 넘어가셔도 될것 같아요!😄
There was a problem hiding this comment.
피드백 주셔서 감사합니다! 동키콩의 의견을 들어보니 해당 코드를 다양한 시각으로 생각할 수 있게 되서 흥미롭네요 😮
|
안녕하세요 동키콩! 궁금한 점에 대해서 피드백 주신 부분에 코멘트로 달았습니다.
-> 현재는 carItems도 context를 통하여 전역 상태관리로 리팩토링하여 props를 줄이고 코드 효율성을 높였습니다! 이에 대한 동키콩의 생각이 궁금합니다! 1. 개별 input box가 모두 선택 시 전체 input box 가 checked 되도록 하여 UX 개선commit: b10222f 2. CartItemCard 컴포너트에서 메소드 가독성 개선commit: 9cca2c8 이전 코드 const isSelected = selectedCartItems.findIndex((item) => item.id === cartItem.id) === -1 ? false : true;리팩토링 코드 - some 메소드를 사용함으로써 boolean값을 return으로 받아 코드 가독성 측면에서 개선 const isSelected = selectedCartItems.some((item) => item.id === cartItem.id);3. SelectInput 컴포넌트에 Type 명시commit: 7230cca type SelectInputProps = React.ComponentPropsWithoutRef<'input'>;4. OrderPriceSummary 매직넘버 상수화commit: 4805823 5. 빈 배열을 반환하는 새로운 App 컴포넌트을 구현하여 빈 장바구니 테스트 케이스에 적용commit: 5e04047 6. 카드 페이지 처음 진입할 때 전체 선택 되도록 로직 추가commit: 4c18d5a |
package.json
Outdated
| "react": "^18.2.0", | ||
| "react-dom": "^18.2.0", | ||
| "react-error-boundary": "^4.0.13", | ||
| "react-router": "^7.6.1", | ||
| "recoil": "^0.7.7", |
There was a problem hiding this comment.
오잉 recoil은 왜 설치되어있었던것이죠? 사용하지 않고 있으니 지워주어도 좋을 것 같아요!
There was a problem hiding this comment.
앗..이번 미션이 작년 기수 미션에서 따온 것 같아서 처음부터 깔려있었던 것 같네요. 제거하겠습니다!
| quantity, | ||
| setCartItems, | ||
| }: CartItemQuantitySelectorProps) { | ||
| const [cartQuantity, setCartQuantity] = useState<number>(quantity); |
There was a problem hiding this comment.
trigger역할을 하는 것이 왜 필요한지 잘 이해를 못했어요. quantity를 굳이 두지 않고 CartItem이나 selectedCartItem을 그대로 사용해도 무방할 것 같다는 생각입니다. 오히려 현재 상황에서는 API에서 에러가 난다면 UI와 서버상태의 불일치가 발생할 수도 있어보여요!
| quantity, | ||
| setCartItems, | ||
| }: CartItemQuantitySelectorProps) { | ||
| const [cartQuantity, setCartQuantity] = useState<number>(quantity); |
There was a problem hiding this comment.
아니면 오히려 낙관적 업데이트를 고려했다고 한다면 이해가 될 것 같아요! 그런데 그것도 quantity없이 구현할 수 있을 것 같기도 하구요.
| await updateCartItem(cartItem.id, cartQuantity); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| console.error('장바구니 아이템 수량 업데이트 실패:', error.message); |
There was a problem hiding this comment.
일반 사용자인 경우는 alert를 통해서 직관적으로 실패함을 알 수 있지만 개발자의 경우 브라우저에서 어떤 error가 발생했는지 개발자 도구 탭에서 확인하기 위함으로 남겼습니다!
동키콩은 이에 대해서 어떻게 생각하시나요! 불필요한 리소스일까요?
| expect(emptyMessage).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('장바구니에서 체크박스를 누르면 배송비를 고려하여 해당 금액이 반영된다.', async () => { |
__test__/cart.test.tsx
Outdated
| }); | ||
| it('장바구니에 상품이 하나도 없을 때 EmptyCartItemUI가 잘 보이는지', async () => { | ||
| const cartItemCards = await screen.findAllByTestId('cart-item-card'); | ||
| if (cartItemCards.length !== 0) return; |
There was a problem hiding this comment.
그렇게 하는 것도 방법일 수 있겠지만 유지보수하는데 추가적인 비용이 발생할 것 같아요. 만약에 저라면 msw를 활용하고 있으니 API모킹을 테스트에도 활용해볼 수 있을 것 같아요 명시적으로 장바구니에 상품이 들어있는 경우와 들어있지 않는 경우를 나눠서 관리하면 테스트 안정성도 올라가지 않을까요?
src/pages/CartPage/App.tsx
Outdated
| const CartList = React.lazy(() => import('../../features/cart/ui/CartList')); | ||
| const OrderPriceSummary = React.lazy(() => import('../../features/cart/ui/OrderPriceSummary')); |
There was a problem hiding this comment.
사실 번들이 kB단위로 보여서 번들크기를 줄이는 효과는 미비할 것 같구요. 때문에 스켈레톤을 보여주는 것에서 loading분기에서 사실상 전부 처리되고 있는 것 같아서 이 역시도 효과가 있다고 보기는 어려울 것 같아요
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export const SelectedCartProvider = ({ children }: SelectedCartProviderProps) => { |
There was a problem hiding this comment.
저라면 사실 관리의 이점때문에 selected필드를 cart에 같이 저장하거나 ids만 관리하여 파생상태로 관리할 수 있도록 구현했을 것 같은데요. 이 부분은 기능의 확장이 어떤 방향으로 가느냐에 따라 달라질 수 있는 부분이 존재할 것 같아서 유지해주셔도 괜찮을 것 같습니다!
| const existingItemIndex = prevItems.findIndex((item) => item.id === cartItem.id); | ||
|
|
||
| if (existingItemIndex > -1) { | ||
| const updatedItems = [...prevItems]; | ||
| updatedItems[existingItemIndex].quantity = quantity ?? 1; | ||
| return updatedItems; | ||
| } |
There was a problem hiding this comment.
오 좋은 관점이네요! 하지만 말씀하신대로 성능상의 이점이 그렇게 큰가를 고민해볼 필요는 있을 것 같아요. 현재 보여주신 코드는 어떻게 할지 설명하는 명령적인 방식이라 코드의 가독성이 떨어진다고 생각했었거든요. 언제나 트레이드오프는 존재하니 적절하게 고려하면 좋을 것 같습니다! 이부분은 그렇구나 하고 넘어가셔도 될것 같아요!😄
|
@JUDONGHYEOK 1. CartItemQuantitySelector 컴포넌트에서 불필요한 state 제거이번에 현재는 이렇게 하는게 최선인데 이 방향에 대한 동키콩의 피드백이나 더 효율적으로 개선시킬 부분들이 있을까요? 2. test코드에서 UI로직과 비즈니스 로직 분리하여 테스트 - Comment
3. test: MSW 핸들러 override로 빈 장바구니 상태 테스트 구현commit: 174dabf 궁금한 점!
이 피드백에서 궁금한 점이 생겼는데 혹시 번들 크기가 어느정도 되야지 |
번들크기에 대한 기준이 명확하게 있는 것은 아니지만 Code Splitting은 보통 100KB 이상일 때 효과가 있다고 생각해요! 10-30KB 정도로는 네트워크 오버헤드가 더 커서 오히려 느려지기도 하구요! |
| expect(emptyMessage).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('장바구니에서 체크박스를 누르면 배송비를 고려하여 해당 금액이 반영된다.', async () => { |
There was a problem hiding this comment.
오 넵 expect문이 여러개가 되는 것은 문제가 되지 않습니다만 지금 상황에서는 너무많은 expect가 반복되어서 분리되면 좋을 것 같았습니다!
안녕하세요 동키콩!
1레벨 미션에서 봽었는데 또 봬서 너무 반갑네요 ㅎㅎ 🍀
그때 많은 새로운 것들을 동키콩 덕분에 알게되어 감사한 마음이 있습니다:)
이번 미션에서도 잘 부탁드립니다!
📦 장바구니 미션
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
RTL 테스트 케이스를 모두 작성했나요?
배포한 데모 페이지에 정상적으로 접근할 수 있나요?
리뷰 요청 & 논의하고 싶은 내용
1) 상태 설계 의도
전체의
cartItems에서isSelected라는 property를 추가하여cartItems전체를 이후에 context에 추가하여 관리 해야할지,selectedCartItems즉,checkbox가 selected된 (결제할 cartItems) cartItems만 전역으로 관리해야할지에 대해서 많은 고민이 있었습니다.그래서 각각의 장단점을 아래와 같이 정리해보았습니다.
1.
cartItems에isSelected타입을 CartItems의 Property에 추가해서 전체 CartItems를 Context로 관리할 경우cartItems뿐만 아니라 selected가 되지 않은cartItems가 필요한 경우에도 모든cartItems를 전역적으로 사용할 수 있다.isSelected를 property에 넣어주기 위해서 매번 업데이트 해야한다. (하지만 useCallback 등으로 해당 문제를 해결할 수 있을 것 같긴 하다.) , Context가 무거워지고 로직이 더 복잡해질 수 있다.2. selected 된
selectedCartItems만 Context로 관리할 경우isSelectedproperty를 넣을 필요가 없어서 전체 순회를 할 필요가 없다. 2번 방법을 사용할 경우 계산을selectedCartItem만 담긴 context 사용해서 비교적 가볍게 관리 할 수 있다.selected가 안된cartItems가 전역적으로 관리할 수가 없가 없어서 해당 data가 필요할 경우 사용할 수가 없다.결론적으로 위 2가지 방법 중 2번 방법을 택하였습니다.
현재 미션은 장바구니 라는 서비스에 한정되어있고 select가 안된 cartItems만 따로 필요 하지 않은 것 같아서
selectedCartItems만 context를 사용하여 전역적으로 관리하기로 결심하였습니다./shared/context에 위치한SelectedCartProvider.tsx에서는selectedCartItems상태를 통해서 로직들을 관리하고 있습니다.return 값으로는 4가지가 있습니다.
selectedCartItemsstateupdateSelectedCartItemaddAllCartItemsInSelectedremoveSelectedCartItem즉
selectedCartItems상태와 이 상태를 업데이트 시키는 3가지의 handler 함수를 통해서 모든 상태를 변화시키고 있습니다.이번 리뷰를 통해 논의하고 싶은 부분
현업에 계신 동키콩을 통해서 DX 측면에서 효율적인 상태관리 (+Context API), 테스트코드, 렌더링 최적화 등을 중심으로 코드리뷰를 받고싶습니다.
현재 lazy import와 Suspense를 사용하여 Skeleton UI를 보여주는 방식으로도 하였는데 해당 로직에서 개선할 부분이 있으면 언제든지 피드백 부탁드립니다!
추가적으로 현업에서는 테스트코드를 어떻게 짜는지 궁금합니다. 미션을 통해서 스스로 배우면서 테스트코드를 짜는 입장에서 이렇게 짜는게 맞나? 라는 생각을 자주 하고 현업에 가서는 어떻게 짜야하지 라는 호기심이 가끔 생기는 것 같습니다.
(현재 코드에서는 MSW를 통해서 Vitest+RTL 테스트를 mock 데이터를 기반으로 하였습니다. 현업에서도 Vitest+RTL를 사용하여 테스트 할때 msw를 쓰는지도 궁금합니다!)
최근에 효율적인 상태관리와 최적화에 대해서 많은 관심이 생겨서 이에 대해서 현재 코드에서 개선시킬 수 있는 부분들이 있으면 같이 상호작용 하면서 많이 배우고 성장하고 싶습니다!
✅ 리뷰어 체크 포인트
1. 클라이언트 상태관리
2. MSW/Test