Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1단계 - 장바구니 미션] 초코(강다빈) 미션 제출합니다. #274

Merged
merged 101 commits into from
May 21, 2024

Conversation

00kang
Copy link
Member

@00kang 00kang commented May 16, 2024

안녕하세요 호프! 초코🍫입니다.
만나서 반갑습니다 :>
시작부터 끝까지 우여곡절 많았던,, 장바구니 미션 리뷰 잘 부탁드립니다.


💻 로컬 실행

npm run dev
npm run test

🔗 배포 링크

바로가기


🔑 키워드 및 학습 목표

  • State Management, Recoil, RTL, API
  • Recoil을 사용하여 클라이언트 상태를 관리할 수 있다.
  • React Testing Library(RTL)를 활용하여 주요 기능에 대한 테스트를 작성할 수 있다.

😮‍💨 상황공유

저는 우아한테크코스를 시작하며 웹 프론트엔드에 입문했어요. 그래서 리액트에 대한 이해도도 낮고, recoil은 이번 미션에서 처음 접하게 되었어요. 이를 참고하여, 키워드 및 학습 목표에 맞게 미션을 수행하고 있는지에 집중하여 리뷰해주시면 감사하겠습니다!

+) 페이지 배포 이후에 useEffect의 의존성 배열에 상태가 들어가 있다는 걸 깨달아서 배포 브랜치에서 수정하게 되었습니다. 코드리뷰를 받는 step1 브랜치에서는 수정되지 않은 점 양해 부탁드립니다! 추후 리팩터링 단계에서 수정하겠습니다.

+) 추가로 페이지 확인할 때 chomre의 설정에서 안전하지 않은 콘텐츠를 허용으로 바꿔줘야 제대로 보인다고 합니다!


❓ 질문사항

  • 이번 미션에서는 UI 작업하는 데에 시간을 많이 사용한 것 같아요. 저는 이번에 Styled Component를 사용하면서 네이밍에도 너무 힘이 들었고, 컴포넌트를 분리하다보니 depth가 이렇게까지 깊어도 되는 걸까? 라는 생각이 들었어요. 지금까지 미션을 진행하면서 UI에 대한 피드백을 많이 못받아서 호프에게 UI적인 리팩터링도 많이 받고 싶어요! UI 작업할 때 호프만의 팁이나 신경쓰는 부분이 있을까요?

  • rcoil,, 개념은 쉬운 것 같았는데 쉽지 않더라구요. 실습시간에 잠깐 접한 걸로 이번 미션에서 수행하기에는 어려움이 많았던 것 같아요. 사용은 했지만 recoil을 제대로 사용한 건지 의문이에요. 특히 다른 상태 관리 라이브러리도 많은데 이번 미션에선 왜 recoil을 사용한 건지 모르겠어요. redux보다 학습 곡선이 낮다는 것 이외에 이유가 있을까요?

  • test,, 도 처음엔 초기값만 테스트 가능할 정도로 어려움이 너무 많았는데요. API 통신이 이뤄지는 걸 직접 테스트하기에는 어려움이 있다고 생각했어요. 그래서 msw 를 사용해야 하나? 싶었는데 이것저것 시도해보다 도저히 내가 뭘 하고 있는 건지 파악이 안될 정도로 어려워서 mockData를 넣어주면서 테스트하도록 진행했어요. 이런식으로 테스트 하는 게 맞는건지 궁금해요. API 통신을 통해 데이터를 가져오게 될 경우 테스트하는 다른 방법도 있을까요?

  • recoil을 작업하면서 atom, selector, atomFamily, selectorFamily ,, 어떻게 관리할지 정하는 게 어렵더라고요. 그래서 현재는 atom과 selector에 익숙해지자는 목표로 이 두가지 방법만 생각했는데요, atomFamily나 selectorFamily 등을 사용하여 더 나은 방향으로 리팩터링할 요소가 있다면 짚어주시면 감사하겠습니다.

00kang and others added 30 commits May 14, 2024 14:57
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
Co-Authored-By: Yuseon Kim(썬데이) <74897720+useon@users.noreply.github.com>
@00kang
Copy link
Member Author

00kang commented May 19, 2024

안녕하세요 호프, 날씨 좋았던 주말 잘 보내셨나요~ 🌞
여전히 리팩터링할 부분이 더 있지만, 더 많은 피드백을 받고 싶어 일단 리뷰 요청 드립니다!
이전 피드백에 남긴 질문도 확인해주시고 리팩터링할 부분 짚어주시면 감사하겠습니다


💻 로컬 실행

npm run dev
npm run test

🔗 배포 링크

바로가기


이전 피드백에 남긴 질문 바로가기


주요 피드백 반영 사항 커밋 바로가기


리팩터링하고 싶은 부분

  • 로컬스토리지와 리코일을 동기화 하기 위한 코드에 리코일의 atom effects 사용해보기
  • 새로운 상품 추가 시 선택된 채로 장바구니에 추가하기
  • 이미 담겨진 상품은 새롭게 추가하는 게 아니라 수량 조절하기

❓ 질문사항

  • 현재 Swagger API를 통해 직접 상품을 추가하면, 장바구니에 이미 담겨진 상품일지라도 새로운 상품으로 인식되어 장바구니 리스트에 업데이트 되는 것 같습니다. 이건 API 설계의 문제인지 아니면 제가 추가 로직을 잘못 이해하고 테스트를 하는 건지 확인해 주시면 감사하겠습니다!

Copy link

@moonheekim0118 moonheekim0118 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 초코~ 🍫 개선해주시느라 고생많으셨어요. 정말 뚝딱 잘 개선해주셔서 크게 추가로 코멘트 남길만한 부분은 없었습니다ㅎㅎ

남겨주신 질문에 대한 답변도 코멘트로 달아두었으니, 다음 단계에 가시기 전에 확인해주세요!

추가로 남겨주신 장바구니 아이템 추가 API 관련 질문은..저도 테스트 해봤을 때
동일 상품을 추가해도 다른 id 값으로 API 응답값이 넘어오더라고요, 아마 요청별로 아이템을 저장하나봅니다. (예리하십니다 👏 )

스크린샷 2024-05-21 오후 12 15 58

제 생각으로는 이건 클라이언트에서 별도로 처리해주지 않아도 된다고 생각합니다. 만약 클라이언트에서 가공로직을 붙여서 동일 상품을 필터링해준다고 해도, 페이지네이션 기능이 추가 되면 클라이언트에서 유의미한 가공 처리는 거의 불가능하게 됩니다. 하더라도 서버와 클라이언트 양측에게 매우 비효율적이지요.
이번 기회에 클라이언트의 책임은 어디까지인가?를 한번 고민해봐주셔도 좋겠습니다 :)

너무 잘 개선해주셔서 이번 단계는 여기서 마무리하려고 합니다. 추가로 개선해주시고 싶으신 사항은 2단계에서 함께 진행해주세요 :) 고생많으셨어요! 2단계에서 뵈어요~

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

훅 만들어주신 것 넘 좋습니다 👍
개인적으로 useDecreaseCartItemQuantity, useDeleteCartItem, useIncreaseCartItemQuantity 는 기능상 세트로 묶여서 같이 사용될 상황이 많을 것 같아서,
하나의 훅으로 묶는편이 코드 응집도를 더 높이지 않을까~생각해봅니다ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 삭제는 수량 변경과 다르게 사용될 수 있다고 생각했습니다. 그래서 delete는 별도의 훅으로 두고, increase와 decrease를 하나의 훅으로 합치도록 수정했습니다!

Comment on lines +14 to +28
const [cartItems, setCartItems] = useRecoilState(cartItemsState);
const { uniqueItemCount } = useRecoilValue(cartSummarySelectorState);

useEffect(() => {
const fetchCartItems = async () => {
try {
const items = await getCartItems();
setCartItems(items);
} catch (error) {
console.error(ERROR_MESSAGES.FETCH_CART_ITEMS, error);
}
};

fetchCartItems();
}, []);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금보니 요 부분(장바구니 아이템 초기 api 요청 후 전역상태에 저장)도 중요한 로직이니, 훅으로 분리해도 좋겠네요ㅎㅎ

Comment on lines +6 to +9
interface CartItemQuantity {
id: number;
quantity: number;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 타입은 공통 타입으로 분리해주신것들로 활용해서 쓸 수 있지 않을까요?

@moonheekim0118 moonheekim0118 merged commit 9d32849 into woowacourse:00kang May 21, 2024
@00kang
Copy link
Member Author

00kang commented May 21, 2024

코멘트 달아주신 것들 2단계에서 반영할 수 있도록 해보겠습니다! 감사합니다 :)

@skiende74
Copy link

아톰 :
단순히 값이 잘 설정되는지 확인하는 테스트는 한 두 종류만 남기고,
로컬스토리지 값이 실제로 잘 바뀌는지 테스트 하면 더 좋을 것 같아요.

셀렉터 :
각 셀렉터들에 대해 세분화해서 테스트를 하면 좋을 거 같아요. (주문금액, 배송비 등)
(예. 주문금액이 10만원 미만일 때, 배송비는 3천원이다. 10만원 이상일 때 0원이다 등)

아이템 삭제, 체크상태 변경에 영향을 받는 셀렉터들이 많기 때문에
이 경우에 대한 검사도 다양하게 추가되면 좋을 것 같아요

훅 :
API 실패시 에러를 띄우고, 상태를 바꾸지않는지 검사와
1인 상태에서는 더 내려가지않게 검사
체크되지 않은 아이템이 삭제되었을때, checkedItemState에서도 제거가 되는지 검사 해주면 좋을거같아요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants