-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[장바구니 미션 Step 2] 황펭(양이진) 미션 제출합니다. #211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
황펭 안녕하세요! 2단계 미션까지 클리어하셨군요! 😃
msw로 API 모킹도 진행하고, 모킹한 API를 가져와서 사용하시는 작업까지 진행하셨군요. 진정한 프론트엔드의 세계에 오신 것을 환영합니다 👏 👏 👏
각 코드라인에 피드백을 남겨두었으니, 확인 부탁드릴게요!
selector를 사용하면서 checkedState나 cartProductState가 일부 필요한 컴포넌트만 구독하는 형태가 되어 필요한 상태들이라는 생각이 들지만 상태가 많아졌다는 생각도 듭니다. 체프는 어떻게 생각하시나요.?
selector를 사용한 것을 상태라고 말할 수 있을까요? 저는 파생된 상태(derived state)가 더 맞다고 생각해요. 상태값은 관리해야 하지만, 파생된 상태는 별도로 값을 관리할 필요 없이 알아서 계산해주니 별도로 관리해야 할 필요는 없습니다. state가 많아지는 것은 문제가 될 수 있지만, selector가 많아지는 것은 큰 문제가 되지 않는다고 생각해요. 오히려 selector로 걱정해야 하는 부분은 연산 비용이 많이 드는가?
인 것 같습니다.
하지만 클라이언트에서 로컬 스토리지 업데이트를 즉각 반영하려면 다시 get 요청을 보내야 했는데요... 요청을 여러번 보내기 보다 atom 상태를 두어 atom과 로컬 스토리지 두 군데에서 업데이트 해주도록 했습니다.
하지만 실패 경우는 따로 처리해주지 않아 실패해도 클라이언트 상태는 업데이트 되는 경우도 있겠다는 생각이 듭니다..🥲
마땅한 다른 방법이 생각나지 않네요.. 두 상태를 동기화 하는 방법이 있을까요..?
서버의 데이터가 바뀌었는지 확실하게 확인하는 방법은 결국 서버에 get 요청을 보내는 방법밖에 없다고 생각해요.
- post/patch/put 요청을 보내고 응답을 받는다.
- get 요청을 보내고 응답을 받는다.
- 받은 데이터로 상태를 업데이트한다.
서버 요청이 갔더라도 직접 get해오지 않는 이상 서버에 정말로 반영되었는지 알 수 있는 방법은 없어요. 말씀하신 것처럼, 실패했을 경우에 클라이언트 상태만 올바르지 않게 업데이트 될 수 있죠.
하지만 post/patch/put 요청 이후 get을 해와서 클라이언트 상태를 변경한다면, get해오는 시간으로 인해서 클라이언트 상태가 업데이트되는 속도가 느려지는 문제가 있어요. 이는 곧 UX 저하로 이어지구요. 이를 위해서 클라이언트 상태를 먼저 업데이트한 다음에, get 요청을 통해서 정말로 서버 상태가 변경되었는지 확인하는 optimistic update를 적용해볼 수 있을 것입니다.
@@ -42,6 +42,7 @@ | |||
"dependencies": { | |||
"react": "^18.2.0", | |||
"react-dom": "^18.2.0", | |||
"react-error-boundary": "^4.0.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오잉, 라이브러리를 사용해도 괜찮나요?
직접 ErrorBoundary 컴포넌트를 작성해보는 것도 좋다고 생각해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorBoundary 라이브러리는 사용 가능하다고 이야기 들었습니다.!
우선 요구 사항과 기능에 집중을 해 사용했습니다 😊
다음 번엔 직접 작성해 보아야겠습니다 👍
findTargetChecked(get(checkedState), id), | ||
}); | ||
|
||
export const checkedCartProductCountState = selector({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
답변에서도 언급드렸다시피 selector를 state라고 하긴 어렵다고 생각해요. checkedCartProductCountSelector로 네이밍이 변경되면 어떨까 싶네요!
src/hooks/useCheckedCount.ts
Outdated
const useCheckedCount = () => { | ||
const checkedCount = useRecoilValue(checkedCartProductCountState); | ||
|
||
return checkedCount; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이렇게 단순한 로직은 그냥 가져다가 사용해도 괜찮겠지? 하는 유혹에 빠질 수 있는데, 커스텀 훅으로 로직을 잘 래핑해주셨네요. 좋은 습관이라고 생각합니다 👍
src/hooks/useCartPrice.ts
Outdated
const deliveryFee = useMemo( | ||
() => (isAllUnchecked ? 0 : DELIVERY_FEE), | ||
[isAllUnchecked] | ||
); | ||
|
||
const totalPrice = useMemo( | ||
() => totalProductPrice + deliveryFee, | ||
[deliveryFee, totalProductPrice] | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 보았을 때는 꽤 단순한 연산 같은데 useMemo를 사용하면 오히려 성능 저하로 이어질 여지가 있어보여요. useMemo는 정말 특별한 경우 아니면 꼭 사용하지 않아도 된다고 생각합니다. 정말정말 처리량이 어마어마할 때만 사용하면 돼요.
최근에 읽었던 아티클 하나를 공유드립니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 아티클 공유 감사합니다 😊
단순한 연산에서 사용하면 오버헤드 비용이 재연산보다 더 들기 때문에 useMemo를 사용하지 않는 것이 더 좋겠네요!
src/hooks/useMultipleChecked.ts
Outdated
setChecked((prev) => filterCartProductChecked(prev, false)); | ||
|
||
checked.forEach((item) => { | ||
if (item.isChecked) cartProductApis.delete(item.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked된 상품 ID 목록을 순회하면서 delete 요청을 보내고 있는데요. 다음 경우의 에러 처리는 어떻게 하면 좋을까요?
- 삭제에 모두 실패한 경우
- 삭제에 일부만 실패한 경우
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.all을 사용해 하나라도 삭제 실패하는 경우 에러 처리할 수 있게 했습니다!
src/apis/cartProducts.ts
Outdated
async get() { | ||
const response = await fetch(cartProductApis.getUrl()); | ||
|
||
await handleResponseError(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleResponseError에도 도달하지 못하는 실패 케이스에 대해서도 대응되었으면 좋겠어요! try catch문을 사용해보시는 건 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleResponseError가 반복되어서 작성되어 있는데요. fetch API를 한 번 wrapping하고 그 안에 handleResponseError를 포함하는 건 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch api를 한 번 wrapping 한 getData와 mutateData를 만들었습니다
각 요청 메서드가 사용하는 곳에서 try/catch문을 사용해 에러 처리 해두었습니다 😊
<CartProductList /> | ||
) : ( | ||
<MessageWrapper> | ||
<Message type='cartEmpty' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금은 메시지의 종류가 많지 않아서 괜찮지만, 더 많고 다양한 타입의 메시지를 보여주어야 할 때는 적합한 형태는 아니라고 생각해요. Message 컴포넌트가 비대해질 것 같거든요.
각 메시지마다 별도의 컴포넌트로 만드는 게 더 좋다고 생각해요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoadingMessage, ErrorMessage 등 컴포넌트 단위로 나누는 말씀인가용.?
확실히 하나의 컴포넌트에 모든 처리를 다한다면 커질 수 밖에 없다고 생각이 드네요.!
loading, success, error 등의 상태로 컴포넌트 단위로 나누고, 그 안에 타입으로 분리해도 괜찮겠다는 생각이 듭니다 😊
src/hooks/useProductQuantity.ts
Outdated
}; | ||
|
||
const subtractCount = () => { | ||
setCartProducts((prev) => subtractTargetQuantity(prev, id)); | ||
setCartProducts((prev) => updateTargetQuantity(prev, id, quantity - 1)); | ||
cartProductApis.patch(id, quantity - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 useEffect를 이용한 삭제를 subtractCount로 이동해 해결했습니다 😊
- 단순한 Endpoint 변경으로 실제 API 사용이 가능하도록 작업 | ||
|
||
3. 테스트 | ||
장바구니 페이지에서 다양한 사용자 인터렉션에 대한 테스트 케이스를 고민하고, 선택한 테스트 도구를 이용하여 검증 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드가 보이지 않는데요. 스토리북으로 대체하신 걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리북으로 대체 했습니다!
인터렉션 맛보기 수준으로만 진행했습니다..😅
src/hooks/useCartProducts.ts
Outdated
useEffect(() => { | ||
if (!targetProduct) return; | ||
|
||
if (targetProduct.quantity === 0) { | ||
setCartProducts((prev) => deleteTargetProduct(prev, id)); | ||
deleteProduct(); | ||
} | ||
}, [id, setCartProducts, targetProduct]); | ||
}, [deleteProduct, targetProduct]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- useEffect의 사용을 지양해야 한다고 생각하는 입장으로서, 저는 이 로직이 subtractCount에 있어야 한다고 생각해요.
위 의견에 어떻게 생각하시는지 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEffect에 장바구니 삭제와 체크 상태에 추가하는 로직이 있었습니다.
이펙트로 두니까 StrictMode에서 체크 상태에 같은 상품이 두 번 추가되는 등 예측하기에 어려움이 있었습니다 🥲
클릭 이벤트에 의한 상태 변화나 요청이므로 예측할 수 있게 각 핸들러로 이동 시켰습니다.
체프는 어떤 점에서 지양해야 한다고 생각하시나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 useEffect는 코드의 흐름을 읽기 어렵게 만드는 주범이라고 생각해서, 웬만하면 사용을 지양해야 한다고 생각하고 있어요. 디버깅도 힘들고요. 😅
체프 안녕하세요.! 협업 미션 진행 후 수정하느라 많이 늦었습니다.. 죄송합니다 🥲 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 반영해주신 부분 모두 확인했습니다! 수고 많으셨어요 :)
남은 장바구니 협업 미션도 화이팅입니다! 💪
배포주소
스토리북
안녕하세요 체프! 주말 잘 보내셨나요?
장바구니 Step 2로 돌아왔습니다 😊
이번 미션은 MSW를 활용하여 실제 서버와 연동될 수 있는 API Mocking이 주요 요구사항입니다.
MSW를 처음 사용해 많은 어려움이 있었는데요... 어찌어찌 해봤습니다.!
리뷰 잘 부탁 드립니다 😄
필수 요구 사항
기능 목록
Recoil
리코일은 간단한 문법이 큰 장점이라고 생각합니다.!
useState
를 다루는 느낌이 들어 리액트를 쓴다면 금방 적용해 볼 수 있다고 생각합니다. 하지만 기본 기능 외에는 공식 문서 외에 자료가 많이 없고(제가 찾아봤을 때는...)unstable
한 부분도 많아 이 부분이 단점이라고 생각합니다.!공유되는 비슷하지만 비슷하지 않은 상태가 있을 때 적용해보면 좋을 것 같습니다. 저는
selector
를 써보면서 인상 깊었는데요, 이번 미션을 진행하면서 장바구니 목록이나 선택 항목 등의 상태가 있었는데, 이때selector
를 이용해 두 상태를 공유해 파생된 상태인 상품과 체크된 정보의 상태(checkedCartProductState
)를 만들었습니다. 업데이트하면checkedCartProductState
도 자동으로 업데이트 되어 파생된 다양한 상태들(체크된 개수, 체크된 가격)도 업데이트 되는.. 다른 라이브러리에서 어떨지는 써보지 않아 모르겠지만.. 이런 흐름이 있더라구요.!체크 상태 관리
checkedState
를 전역 상태로 두고 이 상태와cartProductState
에서 파생된checkedCartProductState
를 사용해 구현했습니다.초기 장바구니 상품을 가져와
isChecked
를false
로 두고, 장바구니에 추가/삭제되면 리스트 업데이트,isChecked
값이 변하면checkedState
의 상태를 업데이트 해주었습니다. 체크된 값들만 필터링 거쳐 개수와 가격을 구했습니다 😊selector
를 사용하면서checkedState
나cartProductState
가 일부 필요한 컴포넌트만 구독하는 형태가 되어 필요한 상태들이라는 생각이 들지만 상태가 많아졌다는 생각도 듭니다. 체프는 어떻게 생각하시나요.?MSW
API 예상 명세를 보고 MSW 핸들러를 작성해 보았습니다. 장바구니 추가, 수량 변경, 삭제는 로컬 스토리지를 이용해 구현했습니다.
요청을 보내면 로컬스토리지를 업데이트하고 응답을 줍니다.!
하지만 클라이언트에서 로컬 스토리지 업데이트를 즉각 반영하려면 다시
get
요청을 보내야 했는데요... 요청을 여러번 보내기 보다atom
상태를 두어atom
과 로컬 스토리지 두 군데에서 업데이트 해주도록 했습니다.하지만 실패 경우는 따로 처리해주지 않아 실패해도 클라이언트 상태는 업데이트 되는 경우도 있겠다는 생각이 듭니다..🥲
마땅한 다른 방법이 생각나지 않네요.. 두 상태를 동기화 하는 방법이 있을까요..?