[1단계 - 장바구니] 로건(신종욱) 미션 제출합니다. #356
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]>
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]>
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]>
Co-authored-by: 신종욱 <[email protected]>
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 (
|
-> 일단 두개다 트레이드오프가 존재해서 정답은 없는거 같아요. 정리를 해보면, selectedIds로 관리한 현재 방식의 장점을 보면, 원본 서버 응답을 그대로 유지해 SSR 캐싱이나 stale-while-revalidate 시에도 안정적이고, 선택 로직이 독립 상태로 관리되어 로컬 상태 관리가 단순해질거 같아요 이런 이유로 결론은 지금 상황에서는 선택 상태는 UI 상태이며, 서버 데이터와는 분리되어야 한다는 점에서 selectedIds를 별도 상태로 관리한 현재 방식이 SRP 관점에서 더 깔끔하고, 확장성에도 유리하다고 저는 생각됩니다. |
-> 이부분도 위와 비슷하게 트레이드 오프를 판단해봐야할거 같아요. 정리하면 현재처럼 refetchCartItems를 인자로 전달하는 방식은 useHandleCartItemQuantity가 독립적이면서도 useShoppingCart와 느슨하게 연결될 수 있어요 그렇기에 테스트 시점에서도 refetch를 mocking해줄 수 있어 유연합니다. 하지만 반대로 useShoppingCart에서 래핑하는 방식은 handleCartItemQuantity의 역할이 명확하게 나뉘어집니다. 예를 들면 하나는 API 요청 핸들러, 하나는 요청 후 side-effect 처리 핸들러 이런식으로요.. 다만 이 경우 loading 상태 분리, 에러 처리 중복 등 코드 중복이 많이 발생 할 수 있다고 생각이 들어요. 그래서 저의 결론은 핸들러는 독립된 로직 단위로 분리하고 필요한 side effect (refetch 등등..)는 상위 훅에서 조합하는 방식이 결합도를 낮추면서 재사용성과 명확한 책임 분리가 가능한 구조라고 생각이 듭니다. |
-> 이부분은 2차 리뷰때 현재 코멘트 남겨진 코드를 수정하고 다시 확인해볼게요 (코드를 수정하면서 화면이 바뀔수도 있어서요) |
|
[리뷰어 리뷰 포인트]
|
src/apis/deleteCartItem.ts
Outdated
| ); | ||
| } | ||
| } catch (error) { | ||
| throw error; |
There was a problem hiding this comment.
catch해서 다시 throw할 거면 try-catch가 의미 없지 않을까요?
There was a problem hiding this comment.
의미 없는 try-catch 였네요!!
수정하여 반영했습니다 ㅎㅎ
eslint의 도움을 제대로 받지 못했었네요 😅
fix: deleteCartItem 및 updateCartItemQuantity 함수에서 불필요한 try-catch 블록 제거
src/apis/deleteCartItem.ts
Outdated
| const BASE_URL = import.meta.env.VITE_BASE_URL; | ||
| const TOKEN = import.meta.env.VITE_TOKEN; |
There was a problem hiding this comment.
이런 코드들은 다른곳에서도 사용할 수 있으니 별도 apiClient 또는 config.ts에서 관리해도 좋을거 같네요
There was a problem hiding this comment.
추상화를 하려고 했는데 시간 관계상 하지 못했었습니다 ㅎㅎ
별도의 apiClient로 공통 되는 부분(BASE_URL, TOKEN, option, url)을 추상화 해보겠습니다!
src/apis/deleteCartItem.ts
Outdated
| const BASE_URL = import.meta.env.VITE_BASE_URL; | ||
| const TOKEN = import.meta.env.VITE_TOKEN; | ||
|
|
||
| const deleteCartItem = async ({ params }: DeleteCartItemParams) => { |
There was a problem hiding this comment.
궁금증) DeleteCartItemParams 는 id 밖에 없는데 params로 래핑한 이유는 코드 공통화를 위함일까요? 뭔가 코드 복잡도만 올라간거 같아서요
There was a problem hiding this comment.
코드 공통화와 확장성을 고려해서 params로 래핑했었습니다!
GET, POST, PATCH, DELETE 전부 api와 연관되어 있기 때문에 인자로 넘기는 경우 params로 통일해둔다면 일관성있게 이런 객체 형식으로 값을 넘기고 있다는 것과 추가로 넘기는 인자가 생기지 않을까 하는 확장성을 고려해 이렇게 했었습니다.
다만 케빈의 말처럼 인자가 하나인 경우, 오히려 명시적으로 id를 넘기는 게 복잡하지 않을 수 있다는 생각도 드네요 ㅎㅎ
현재는 delete에서 다양한 인자를 받을 일이 없을 것 같아 id만 넘길 수 있도록 수정했습니다! 😊
refactor: deleteCartItem 함수의 매개변수 구조 변경
혹시 케빈은 코드 복잡도를 낮추기 위해 인자에 대한 규칙이나 기준이 있을까요? 🤔
페어와 빠르게 진행 하면서 제대로 지키지 못한 부분도 있을 것 같지만,
일단 제가 생각하는 기준은 "인자의 수가 2개 이상부터는 객체, 하나의 경우 객체가 아닌 값으로 전달"이라고 생각하고 있습니다 😊
| ); | ||
| } | ||
|
|
||
| export default CartCard; |
There was a problem hiding this comment.
궁금증) id.toString 하는곳이 많은데, 타입이 string이어야 한다면, 처음부터 타입을 string으로 통일하거나, 변환을 최소화해야 유지보수가 편할거 같아요
There was a problem hiding this comment.
코드를 전체적으로 시맨틱하게 짜도록 노력해보면 좋을거 같아요
There was a problem hiding this comment.
궁금증) handleCartItemQuantity 를 다루는 로직에서 버튼을 빠르게 여러 번 누르면 이슈가 생길수도 있지 않을까요? 요런점을 고민해보셨을까요?
There was a problem hiding this comment.
초기에 타입을 잘못 설정해서 계속 toString을 통해 변환해줘야 했네요 😅
서버에서 오는 타입 그대로 사용할 수 있도록 number 타입으로 설정했습니다.
refactor: 장바구니 관련 함수 및 컴포넌트에서 id 및 quantity 매개변수를 string에서 number로 타입 변경
"코드를 전체적으로 시맨틱하게 짜도록" 이 말이 **"무엇을 하는 코드인지 그 의도와 목적이 명확하게 드러나도록 작성"**이 맞을까요?? 🤔
만약 맞다면 변수 이름, 함수 이름, 컴포넌트 이름, 파일 구조 등이 그 역할과 의미를 좀 더 직관적으로 나타내도록 해보겠습니다!!
전체적으로 수정 후 보셨을 때 어떤 부분에서 좀 더 수정이 되면 좋겠다 라는 게 있으시다면 구체적으로 짚어주시면 감사하겠습니다 ㅎㅎ
궁금증) handleCartItemQuantity 를 다루는 로직에서 버튼을 빠르게 여러 번 누르면 이슈가 생길수도 있지 않을까요? 요런점을 고민해보셨을까요?
isLoading을 선택한 이유는 중복 호출을 제한하자는 맥락에서 선택했었습니다! 구체적으로 한 번의 수량 변경(PATCH) 및 장바구니 정보 갱신(GET) 프로세스가 완전히 완료되기 전까지는 추가적인 요청이 나가지 않도록 방지하려 했습니다.
리뷰 주신 '버튼을 빠르게 여러 번 누르면 이슈가 생길 수 있다'는 말씀에 대해, 혹시 다음과 같은 측면들을 우려하신 걸까요? ㅎㅎ
-
isLoading에 의해 여러 번의 버튼 클릭을 시도조차 못한 이슈 (사용자 경험/의도 불일치):
- 현재는 isLoading을 통해 버튼을 비활성화 하고 있습니다. 만약 '수량 +1' 버튼을 빠르게 세 번 눌러서 수량을 3 늘리려 했는데, 버튼이 비활성화되어 한 번만 눌러졌다면, 사용자는 자신의 의도가 완전히 반영되지 않았음에 당황할 수 있을 것 같습니다. 즉, 비활성화된 버튼 때문에 애초에 여러 번 누르지 못했으니, 3번의 증가를 시도조차 못했다고 생각할 수 있을 것 같습니다. 🤔
-
PATCH와 GET api 요청이 버튼 클릭마다 이뤄지므로 api 요청을 너무 많이하는 이슈 (중복 요청 또는 너무 잦은 요청)
-
PATCH가 실패해도 early return이 없으므로 GET 요청이 이뤄지는 이슈 (의미 없는 요청)
There was a problem hiding this comment.
"코드를 전체적으로 시맨틱하게 짜도록" 이 말이 **"무엇을 하는 코드인지 그 의도와 목적이 명확하게 드러나도록 작성"**이 맞을까요?? 🤔
만약 맞다면 변수 이름, 함수 이름, 컴포넌트 이름, 파일 구조 등이 그 역할과 의미를 좀 더 직관적으로 나타내도록 해보겠습니다!!
전체적으로 수정 후 보셨을 때 어떤 부분에서 좀 더 수정이 되면 좋겠다 라는 게 있으시다면 구체적으로 짚어주시면 감사하겠습니다 ㅎㅎ
-> 일단 기본적으로 태그를 기준으로 말씀드리려고했어요! 지금 li 안에 div 요소가 쭉 있는데 하나의 상품 정보 덩어리는 의미상
로 감싸는 게 더 명확할거 같구, ProductName 도 강조를 위해서 p 태그 보단 strong 이 좋을거 같아요! CartCard.tsx 파일 기준으로만 보고 말씀드렸는데, 지금 시기에 접근성과 시맨틱 관련해서 한번 정리를 하면 좋을거 같아서 코멘트 달아봤어요!There was a problem hiding this comment.
isLoading을 선택한 이유는 중복 호출을 제한하자는 맥락에서 선택했었습니다! 구체적으로 한 번의 수량 변경(PATCH) 및 장바구니 정보 갱신(GET) 프로세스가 완전히 완료되기 전까지는 추가적인 요청이 나가지 않도록 방지하려 했습니다.
리뷰 주신 '버튼을 빠르게 여러 번 누르면 이슈가 생길 수 있다'는 말씀에 대해, 혹시 다음과 같은 측면들을 우려하신 걸까요? ㅎㅎ
-> 1번 이유가 코멘트 의도가 맞을거 같아요! 그래서 의도는
<ProductQuantityControl
...
disabled={isQuantityUpdateLoading}
/>
이런식으로 하는 방향이 있으면 좋겠다 라는 생각이었는데, 말씀주신 것처럼 isLoading 값이 있었네요! 제가 disabled 값만 생각해서isLoading 을 놓친거 같아요! 코멘트 달아주셔서 감사해요~
| useEffect(() => { | ||
| if (!initialized.current && cartItemsData.length) { | ||
| const allIds = cartItemsData.map((item) => item.id.toString()) | ||
| setSelectedCartIds(allIds) | ||
| initialized.current = true | ||
| } | ||
| }, [cartItemsData]) |
There was a problem hiding this comment.
저는 useEffect 에서 setState 함수나 ref 가 있을때 매우 주의깊게 봅니다. 이런 로직에서 에러를 많이 봐서인데요. 지금처럼 태 초기화 로직이 useEffect + ref로 다소 불안정하다는 느낌을 받습니다.
이부분은 꼭 수정은 아니지만, 저라면 ref 대신 useMemo 또는 derived state로 처리하는 편이 더 안전할거 같고, 단방향 흐름을 유지하려면 cartItemsData → selectedCartIds 파생 추론하는 방식이 적합해서 요렇게 수정할거 같아요
There was a problem hiding this comment.
selectedCartIds는 장바구니에서 선택이 될 때마다 바뀌어야 해서 useMemo 또는 derived state로 할 수 없다고 생각했습니다. 🤔
function CartContent () {
...
const selectedCartIds = useMemo(() => {
return cartItemsData.map((item) => item.id);
}, [cartItemsData]);
// handleSelectCartItem 같은 핸들러는 이젠 selectedCartIds를 변경할 수 없습니다.
const handleSelectCartItem = (id: number) => {
// 이 부분에서 selectedCartIds를 직접 변경할 수 없음
};
...
}시도했던 다른 방법
상위에서 cartItemsData가 빈 배열이 아닐 때만 cartItemsData를 받도록 구조를 변경했습니다.
이를 통해 useEffect와 useRef를 사용하지 않도록 변경할 수 있었습니다.
function Cart(){
...
return (
...
cartItemsData.length && (<CartContent ... />)
)
function CartContent({
cartItemsData,
handleCartItemQuantity,
handleDeleteCartItem,
isQuantityUpdateLoading,
isDeleteItemLoading,
}: CartContentProps) {
const [selectedCartIds, setSelectedCartIds] = useState<number[]>(
cartItemsData.map((item) => item.id)
)
...문제점
- 이렇게 props로 넘기는 값을 state의 초기값으로 설정하는 것은 안티패턴이라는 것을 공식문서에서 봤었습니다.
- CartContent 내부에서 cartItemsData.length에 따라 EmptyCartMessage 컴포넌트를 보여주고 있어서 이렇게 상위에서 하면 CartContent 내부의 조건부 렌더링이 되지 않습니다.
현재는 마땅한 방법이 없는 것 같아서 훅으로 분리해봤는데 괜찮은 건지 잘 모르겠습니다. 😅
refactor: 장바구니 선택 기능을 useSelectedCartIds 훅으로 분리
케빈의 개선안이 이런 의도는 아니었을 것 같은데 더 좋은 구조가 있을까요? 🤔
There was a problem hiding this comment.
일단 제가 생각했던 리팩토링은 이런식이였어요!
function CartContent() {
const {
cartItemsData,
handleCartItemQuantity,
handleDeleteCartItem,
isQuantityUpdateLoading,
isDeleteItemLoading,
} = useShoppingCart();
const [selectedCartIds, setSelectedCartIds] = useState<number[]>([]);
useEffect(() => {
// 초기화 조건: cartItemsData가 채워지고, selectedCartIds가 아직 비어있을 때만
if (cartItemsData.length > 0 && selectedCartIds.length === 0) {
setSelectedCartIds(cartItemsData.map((item) => item.id));
}
}, [cartItemsData, selectedCartIds.length]);
const handleSelectCartItem = (id: number) => {
setSelectedCartIds((prev) =>
prev.includes(id) ? prev.filter((i) => i !== id) : [...prev, id]
);
};
...
}
ref 없이 selectedCartIds를 초기화하는 방식입니다!
1번 질문에서 안티패턴이라고 하셨는데, 제 생각은 조금 다른 거같아요
이 패턴이 문제가 되는 것은 주로 props가 변경되었을 때 state는 그대로 남아 예기치 않게 동기화가 어긋나는 경우인데, 현재처럼 "초기 mount 시 한 번만 prop(cartItemsData)을 기반으로 상태(selectedCartIds)를 구성하고, 이후는 내부 상태로 관리하는 경우"는 허용 가능한 예외인거 같아요 요 상황은 해당 문서 를 봐주시면 좋을거같아요!
2번은 말씀하신대로 정확하게 "Empty 상태 처리도 CartContent의 책임"이라면, 상위에서는 무조건 렌더하고, 내부에서 상태 분기 처리하는 것이 구조적으로 더 좋습니다.
| const handleOrderConfirm = () => { | ||
| const selectedCartItems = cartItemsData.filter((cartItem) => | ||
| selectedCartIds.includes(cartItem.id.toString()), | ||
| ) | ||
| const totalPrice = selectedCartItems.reduce( | ||
| (total, item) => total + item.product.price * item.quantity, | ||
| 0, | ||
| ) | ||
|
|
||
| const shippingFee = totalPrice >= FREE_SHIPPING_OVER ? 0 : SHIPPING_FEE | ||
| const totalPriceWithShipping = totalPrice + shippingFee | ||
|
|
||
| const state: OrderConfirmationLocationState = { | ||
| selectedCartItemsLength: selectedCartIds.length, | ||
| selectedCartItemsCount: selectedCartItems.reduce( | ||
| (totalCount, item) => totalCount + item.quantity, | ||
| 0, | ||
| ), | ||
| totalPrice: totalPriceWithShipping, | ||
| } | ||
| navigate(PAGE_URL.ORDER_CONFIRMATION, { state }) | ||
| } |
There was a problem hiding this comment.
이 함수를 보면, 장바구니 항목 필터링, 가격 계산, 배송비 계산 모두 한 함수에 몰려 있는거 같은데 분리해볼수 있을까요?
There was a problem hiding this comment.
동일한 함수가 중복되어서 사용되고 있어서 각각의 계산을 함수로 분리하고 이 함수들을 getOrderSummary 함수에서 호출하는 방식으로 분리했습니다!
| const shippingFee = totalPrice >= FREE_SHIPPING_OVER ? 0 : SHIPPING_FEE; | ||
| const totalPriceWithShipping = totalPrice + shippingFee; |
There was a problem hiding this comment.
이 로직이 어디서 본거같은 생각이 들었는데, handleOrderConfirm 에도 비슷한 로직이 있네요
SRP 관점에서도 UI + 계산이 섞여 있어 테스트나 확장성이 떨어질거 같은데 수정해볼수 있을까요?
There was a problem hiding this comment.
동일한 함수가 중복되어서 사용되고 있어서 각각의 계산을 함수로 분리하고 이 함수들을 getOrderSummary 함수에서 호출하는 방식으로 분리했습니다!
src/contexts/ApiContext.tsx
Outdated
| type Action = | ||
| | { type: "startFetch"; name: string } | ||
| | { type: "fetchSuccess"; name: string; payload: unknown } | ||
| | { type: "fetchError"; name: string; error: unknown } | ||
| | { type: "endFetch"; name: string }; |
There was a problem hiding this comment.
안전하게 타입 사용할 수 있도록 정의했습니다! 👍
refactor: FetchActionType 및 FetchActionName 열거형 추가 및 관련 코드 수정
| dispatch: React.Dispatch<Action>; | ||
| }>({ | ||
| state: initialState, | ||
| dispatch: () => {}, |
There was a problem hiding this comment.
이렇게 뒀을때, 실수로 context를 provider 없이 사용하면 런타임 오류 발생할거 같아요
There was a problem hiding this comment.
어떤 의도로 피드백을 주신 건지 이해가 잘 가지 않습니다.
혹시 힌트나 자세한 설명을 좀 더 주실 수 있을까요??
export function useData<T>({
fetcher,
name,
}: {
fetcher: () => Promise<T>;
name: FetchActionName;
}) {
const context = useContext(ApiContext);
if (!context) {
throw new Error("apiContext는 apiContextProvider 내부에 위치해야 합니다.");
}
...여기서 throw new Error를 하고 있어서 문제라는 걸까요?? 👀
There was a problem hiding this comment.
아하 요 코드에서 제가 생각한 부분은,
useContext(ApiContext)를 사용할 때, ApiProvider 없이 사용하면 이 기본값이 그대로 사용됩니다. 이때 dispatch()를 호출해도 아무 일도 안 일어납니다. 심지어 오류도 발생하지 않아요. 즉, silent fail이 됩니다.
코드로 보면 좀더 쉽게 이해가 될텐데,
function App() {
return <CartPage />; // ApiProvider 없이 사용
}
이런코드에서 dispatch()가 실행되지만 실제 리듀서가 동작하지 않고, 콘솔 에러도 없습니다.
즉, 버그가 발생해도 디버깅이 매우 어렵습니다.
라는 생각으로 달았었습니다!
근데 사실 요 로직은 로건이 보여준 코드로 해결됩니다!
const context = useContext(ApiContext);
if (!context) {
throw new Error("apiContext는 apiContextProvider 내부에 위치해야 합니다.");
}
이렇게 잘 처리해주고 있는데, 이 코드 덕분에 useData에서 context가 undefined일 때 명시적으로 에러를 던지기 때문에, 위에서 말한 silent fail 문제가 발생하지 않습니다.
여기에 대해서 잘 이해하고 있는지 궁금하기도 하고 서로 싱크가 잘 맞혀지는지 확인차 코멘트를 달아봤어용
| case "startFetch": | ||
| return { | ||
| ...state, | ||
| loadingStates: { ...state.loadingStates, [action.name]: true }, |
There was a problem hiding this comment.
이러면 매 액션마다 상태가 깊은 복사되어서 메모리 효율이나 리렌더링 불필요하게 증가할거 같은데 어떻게 수정해볼수 있을까요?
There was a problem hiding this comment.
후보 1. 이전 미션에서 여러 API를 하나에서 관리하는 거에서 인상 깊어서 그대로 쓰려고 하니 지금 사용하고 있는 api가 하나인데 상태가 너무 깊게 되었습니다. 플랫하게 만들기 위해선 구조를 간단하게 하나의 api에 맞춰서 만들면 될 것 같습니다.
아마 다음과 같은 구조로 플랫하게 만들어질 것 같아요! 😅
const initialState: State = {
data: [],
loading: false,
error: false,
};
function apiReducer(state: State, action: Action): State {
switch (action.type) {
case FetchActionType.StartFetch:
return {
...state,
loading: true ,
error: undefined ,
};후보 2. 리렌더링 최적화에 맞춘다면 메모이제이션을 잘 하면 되는 걸까요?
케빈의 의도가 어떤 측면인지 궁금합니다! 🤔
There was a problem hiding this comment.
일단 코멘트의 의도는 리렌더링나 메모리 효율 관점에서의 불변성 복사 최소화해보면 어떨까에서 시작됐어요
loadingStates: { ...state.loadingStates, [action.name]: true }
이코드는 매번 얕은 복사를 하게 되며, 상태 객체가 변경되었는지 비교하는 구조(useContext, useSelector 등)에 따라 불필요한 리렌더링이 발생할 수 있지 않을까? 라고 생각됐어요 그래서
if (state.loadingStates[action.name] === true) return state;
이런 조건문이 있다면 동일한 값으로 불변성을 깨지 않고 setState 하지 않아서 React의 얕은 비교 최적화를 활용해 불필요한 리렌더를 막을 수 있으면 좋겠다 라는 생각을 했어요!
그치만 로건이 제안해준 후보 1은 구조 설계 최적화 관점에서 매우 좋은 방법인거 같고, 2번도 메모이제이션 사용도 좋은데, 하지만 이 경우는 상태 업데이트 자체가 불필요하게 일어나는 문제였기 때문에, 최대한 setState 자체를 하지 않도록 하는 게 1차적인 해결방법인거 같아요
|
안녕하세요 케빈! #356 (comment)
아마 주요 기능에 대해 에러 테스트가 예외에 대한 테스트일 것 같습니다! |
|
[ux 리뷰] 상품 목록이 많아질때, 전체가 스크롤 되는게 아니라 여기만 스크롤되면 더 좋을거 같아요 |
|
[ux 리뷰] 각상품의 재고가 표시되면 좋을거 같아요 |
|
[ux 리뷰] 배송비가 0원이라도 없어지지 말고, 0원이라고 표기해주면 좋을거 같아요~ |
|
[버그] 2025-06-05.1.53.06.mov |
안녕하세요. 케빈!! 저는 로건입니다!
좋은 점, 아쉬운 점 가감없이 편하게 피드백 부탁드립니다!! 👍
2주간 잘 부탁드립니다! 🙇♀️
📦 장바구니 미션
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
RTL 테스트 케이스를 모두 작성했나요?
배포한 데모 페이지에 정상적으로 접근할 수 있나요?
리뷰어가 장바구니 추가를 쉽게 할 수 있도록 Curl 명령어를 전달해주세요. (토큰을 채워주세요)
리뷰 요청 & 논의하고 싶은 내용
시연 영상
default.mov
1) 상태 설계 의도
원본 상태
현재 장바구니에서 선택한 장바구니 아이템들을 식별할 수 있어야 했습니다.
서버에서 주는 데이터를 그대로 받아오고, 그 중에서 필요한 id들만 고를 수 있도록 또다른 원본 상태를 만들었습니다.
모든 정보를 다루지 않고 필요한 값만 꺼내서 다루는 게 상태를 복잡하게 다루지 않는다고 생각하여 이렇게 만들었습니다.
2) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
사용자의 경험
프론트엔드는 사용자 경험이 가장 많이 영향을 주는 부분이며, UI를 하나하나 설계를 할 때, 사용자에게 좀 더 편리한 경험을 제공하는 것이 중요하다고 생각합니다.
갑자기 사용자가 수량을 변경하다가 감소 버튼을 한 번 더 누르면 요소 자체가 사라져서 당황스러울 수 있다고 생각했습니다. 그래서 수량이 1이 되면 - 버튼을 disabled 처리하여, 사용자가 의도치 않게 상품이 장바구니에서 사라지는 혼란을 방지하고 안정적인 사용 흐름을 제공하고자 했습니다. 장바구니에서 상품을 없애고 싶다면 무조건 옆의 삭제 버튼을 이용하도록 유도하는 것이 더 명확한 사용자 경험이라고 판단했습니다. 쿠팡과 같은 곳을 많이 참고해봤습니다!
또한 각각의 버튼에서 사용자가 2번 연속 잘못 누르는 것을 방지하기 위해, 이 버튼이 사용되고 있는지 loading 상태를 두어서 요청이 실행 중일 때는 disabled 처리했습니다. 현재는 MSW 환경이라 매우 짧게 1ms 정도 disabled 되는 것을 확인할 수 있었는데, 실제 API 연결이 된다면 사용자의 의도치 않은 중복 클릭을 막아 더욱 안정적인 서비스를 제공하는 데 유용하게 사용될 것이라고 생각합니다.
children을 사용해서 props drilling 피하기
children을 사용해서 props drilling을 피하려고 노력했습니다. 이전에는CartList컴포넌트가cartItemsData와selectedCartIds를 props로 직접 받아 하위의CartCard에게 다시 props로 전달하는 구조였습니다.CartList는childrenprop을 통해CartCard들을 받아 렌더링하도록 변경했습니다. 이렇게 함으로써CartList는 더 이상 중간에서 데이터를 전달하는 역할만 하지 않고, UI 구조를 담당하게 되었습니다.useShoppingCart커스텀 훅 내부에서useData라는 커스텀 훅을 통해 장바구니 아이템에 대한 데이터를 받고,useData는 Context API를 통해 전역 상태를 전달합니다. 이를 통해 하위 컴포넌트들이 필요한 데이터를 props로 일일이 받지 않고 훅을 통해 직접 사용할 수 있게 되었습니다.children을 사용하여CartList에CartCard를 명시적으로 배치하고, 필요한 데이터를 각CartCard에서 사용하도록 하여 props drilling을 개선할 수 있었습니다.3) 이번 리뷰를 통해 논의하고 싶은 부분
클라이언트 상태 관리
처음에는 서버에서 주는 데이터(response)를 가공해서 cartItems에서 각각의 cartItem에 selected 속성을 추가할까 생각했었습니다. 이러면 원본 상태가 하나가 되고, 이 상태 하나로 모든 파생 상태를 다룰 수 있을 것 같았습니다.
그러나 서버에서 주는 데이터에서 필요한 값만 꺼내서 사용하는 것도 하나의 방법이라고 생각하여 현재는 selectedIds 라는 새로운 상태를 만들어서 했습니다. 이렇게 하니까 선택된 장바구니 아이템들을 식별해야할 때 좀 더 편하게 했던 것 같습니다. 예를 들어 선택 관련 로직은
selectedIds상태만 업데이트하고 참조하면 되므로, 복잡한 상태 업데이트를 할 필요가 없었습니다.서버에서 가져오는 데이터를 가공하여 클라이언트에서 사용하는 경우, 1번이 하나의 원본 상태만 갖고 있기 때문에 “SSOT가 잘 지켜지고, 불필요한 상태를 만들지 않는다” 라는 생각과 “2개의 방법이 큰 차이가 없는 것 같다”는 생각이 들고 있습니다.
케빈은 밑의 2가지 방법에 대해 어떻게 생각하시는지 궁금합니다!
훅과의 결합
useShoppingCart 내부에서 핸들러 함수
handleCartItemQuantity를 만들고 있었습니다. 이 핸들러 함수안에서 수량 변경하는 PATCH 요청이 되고, 다시 서버에 장바구니 아이템을 요청하는 refetch 함수가 실행이 되어야 했습니다.이때, 각각의 버튼에서 이 핸들러 함수를 실행할 때 사용자가 2번 연속 잘못 누르는 것을 방지하기 위해, 이 버튼이 사용되고 있는지 loading 상태가 필요했습니다. 따라서 핸들러 함수를 커스텀 훅으로 분리해서 loading 상태를 갖게 했습니다.
이렇게 하고 나니까 고민이 생겼습니다. 서버에 장바구니 아이템을 요청하는 refetch 함수를 어디서 이 핸들러 함수와 결합시킬지가 고민이었습니다.
현재는 이 핸들러 함수가 useShoppingCart와 강하게 결합되어 있다고 생각하여 refetch 함수를 인자로 전달했습니다.
그러나 다른 방법으로는 useShoppingCart에서 핸들러 함수를 한번 더 래핑해서 refetch를 추가해도 될 것 같습니다.
이렇게 어디선가 훅과 함수를 결합해야하는데, 이 결합이 괜찮은 결합인지 궁금하고, 어떤 방식으로 이것을 피할수 있는지, 아니면 이정도는 묵인되는 패턴인지 알고 싶습니다.
사용자 경험
현재 장바구니 페이지에서는 상품의 수량을 변경하거나 삭제하는 기능을 구현했습니다.
혹시 장바구니 사용 흐름에서 더 나은 사용자 경험을 제공하기 위해 보강할 만한 부분이 있을까요?
예를 들어,
등에 대해 케빈의 의견을 듣고 싶습니다!!
✅ 리뷰어 체크 포인트
1. 클라이언트 상태관리
2. MSW/Test