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

[장바구니 협업 Step2] 도밥(이도현) 미션 제출합니다. #172

Merged
merged 151 commits into from
Jun 12, 2023

Conversation

Creative-Lee
Copy link

@Creative-Lee Creative-Lee commented Jun 5, 2023

안녕하세요 @InSeong-So 👋 도밥입니다😁
협업 미션 step2 요구사항 반영하여 찾아왔습니다..ㅎ
파랑에게 처음이자 마지막으로 받는 피드백이 되겠네요..! (개인적으로 너무 아쉽습니다 ㅠㅠ)
부족한 코드이지만, 잘 부탁드리겠습니다...ㅎ

데모 페이지✨
스토리북🎨
API 명세 - 포스트맨
API 명세 - swagger
(포스트맨 배포된 DOCS는 Example들 때문에 조금 보기 더럽습니다.. 양해부탁드립니다.)

컴포넌트 구조도

image

재화요소 추가💰

- 5만원 이상 구매시 배송비 무료

- 결제 금액의 10% 포인트 적립 및 사용가능

위 두개의 기능을 적용했습니다.

아쉬운 점😅

이번에도 역시 챙기지 못한 것들이 많아서 아쉬운 점 부터 적게되네요!

  • fetch 사용처, error / loading 처리에 대한 일관성 X

이번 미션을 진행하면서 fetch로직과 이에 대한 로딩 에러처리가 여기저기 흩뿌려져 있는 상황이 발생했습니다.
일관성이 없음을 인지하고 여러 노력을 했지만, 이를 해결하지 못했어요. 많이 아쉽네요.
아래 고민했던 내용과 질문 사항에 추가 설명해 두었습니다.

  • 랜더링 최적화에 대한 고민을 하지 못함.

구현, 디자인, recoil, 에러, suspense 등을 우선시 하고 미루고 미루다가 결국 고민하지 못했습니다. ㅠㅠ
랜더링에 대한 부분을 신경쓰지 못한게 아쉽습니다.

고민했던 부분...💆‍♂️

관리하기 어려운 API 로직과 loading, error처리

자세히 보기
우선, 스스로도 확실하게 정리되지 않아, 다소 횡설수설하고 어지러운 글 쓰게된 점 죄송합니다🙇‍♂️
고민은 정말 많이 했는데, 당최 정리가 되지 않습니다...ㅠㅠ
제 상황을 어떻게든 설명드려야할 것 같아 아래에 나열해놓았습니다.

현재 API, 로딩, 에러 처리 구조

  • 상품목록, 포인트, 주문 목록, 주문 상세 GET -> useFetch 커스텀 훅으로 해당 컴포넌트에서 사용.

    • 에러처리 : useFetch훅에서 제공하는 errorState를 통해 처리
      • 에러 관련 컴포넌트를 보여주고 에러 메세지 그대로 보여줌. reFetch 하는 방법을 제공
    • 로딩처리 : useFetch훅에서 제공하는 isLoading을 통해 처리
      • 로딩중... div 랜더, 보여지는 text를 다르게함.
  • 장바구니 수량 변경 관련 POST, PATCH, DELETE -> useShoppingCart 커스텀 훅으로 관리

    • 에러처리: 함수 내부에서 try catch를 통해 처리
      • 직접 적어놓은 메세지를 alert으로 보여줌. 개발자에겐 console.error 보여줌
        • 당시에는 충분하다고 생각했는데, 지금은 사용자에게 추가적인 무언가를 제시해야 하나? 고민이 계속됩니다.
    • 로딩처리: 없음.
      • 요청 성공시에만 장바구니 전역 상태를 변경하고 UI 변경함.
  • 장바구니 데이터 GET -> recoil atom 기본값으로 설정,
    해당 atom을 사용하는 컴포넌트중 트리 제일 상단에 위치한 Header에서 fetch함(여기선 RecoilLoadable 사용)
    (미션 초기에 useFetch를 사용을 고려했으나, useEffect를 통해 setRecoilValue 해주는 것이 어색하다고 판단했습니다.)

    • 에러처리: 없음.
      • 에러가 발생하면 앱이 터지는 상황.
      • Header에서 하는 fetch가 실패하면 어짜피 해당 atom을 사용하는 컴포넌트 랜더링이 불가능하다.
      • 어떤 방식으로 사용자에게 알려줘야 할까? 고민 했지만 답이 안 나왔었습니다.
    • 로딩처리: RecoilLoadable이 제공하는 'loading'으로 처리
      • 사용자에게는 장바구니 물품 수량을 0으로 보여주다가 fetch가 완료되면 해당 수량을 보여줌.

이 밖에
Can't perform a React state update on an unmounted component 에러가 발생함에 따라 Suspense를 써야만했던?! 곳도 있어요. (Router.tsx의 ShoppingCartPage route 부분)


복잡한 머리속을 위와 같이 정리를 해보았는데요..
분명 일관성이 없다고 생각을 하면서도, 그때 그때의 기억을 되살려보면 합당했거나, 어쩔수 없었던것 같아요...
제가 어떤 부분이 부족한지를 잘 모르는 상태 같습니다.
정말 죄송하지만, 어떤 말씀이라도 해주시면 감사하겠습니다...

그밖에...

자세히 보기

디렉터리 구조는 어떤지.
커스텀 훅들은 잘 만들어진 상태인지, 그 역할은 잘 파악되는지 궁금합니다.

그리고 파랑이 제 코드를 보고 떠오르는 말들을 여과없이 해주셨으면 합니다.
어떤 부분을 더 신경쓰고 보완해야 하는지, 어떤 부분을 추가로 학습해봐야 할지 조언 부탁드립니다.


마지막 리뷰라고 생각하니 지금까지 잘해왔나 돌아보게 되네요.
긴 PR 읽어주신다고 고생하셨습니다. 파랑
정말 감사합니다.

Creative-Lee and others added 30 commits May 28, 2023 20:54
1. 상수 객체의 키값 대문자 스네이크 케이스로 변경
2. 주문 페이지 추가
1. 로컬스토리지 유틸 추가
2. 컴포넌트 return문에 존재하던 로컬스토리지 로직 상단으로 분리
1. 메인 로고 적용
2. 주문 목록 아이콘 적용
3. 컬러 및 반응형 디자인 수정
4. 장바구니 수량 아이콘 absolute 처리
5. hover 기능 추가
flex, sizing을 인자로 받는 디자인용 컴포넌트 구현
1. 배경 hover 컬러 적용
2. 삭제할 상품 없을 시 disble 처리
provider 사용으로 내부 상태를 공유하도록 구현
@woowahan-cron
Copy link

5만원 배송비 무료는 넘심...
도밥은 네고왕 만나야 정신차리시겠네요
image

@InSeong-So InSeong-So self-requested a review June 8, 2023 13:00
@InSeong-So InSeong-So self-assigned this Jun 8, 2023
Copy link

@InSeong-So InSeong-So left a comment

Choose a reason for hiding this comment

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

마지막 미션인데 리뷰가 엄청 지연됐네요😭😭😭
정말 건강이 너무 안 좋아져서... 죄송합니다🙇‍♂️

도밥이 작성했던 아쉬웠던 점에 대해

일관성이 없음을 인지했다면, 어떻게 일관성을 지킬지에 대해 엄청난 고민을 했을 거에요. 명확한 답을 못 찾았겠지만... 도움이 될 수 있는 말을 주자면, 현 시점에서 도밥이 해왔던 다른 미션의 코드를 보는 겁니다. 어떻게 변경해야 하는지 복기해보세요.

렌더링 최적화는 차치하면 좋겠네요. 이건 두 마리 토끼를 잡는 문제가 아니라 한 마리 토끼와 한 마리 고등어를 잡는 것과 같습니다. 무슨 이야긴가 하면, 같은 맥락으로 접근하면 안 된다는 거에요.

비동기 통신에 대한 핸들링은 아래에서 이어 이야기해봅시다.

고민했던 부분

비동기 통신을 처음 접하면서 생기는 문제는 모두 usecase의 빈곤에서 옵니다. 어디서부터 어디까지 고려해야 하는지 모르기 때문에 하나하나 날 것으로 작성하게 되는 거죠. 잘못 한 게 아니에요!

결국 모든 내용은 비동기 통신을 어떻게 제어해야 하는가로 귀결되는 것 같아요. 그런 도밥에게 4가지 조건을 줄게요.

  1. 사용자에게 필요한 에러인가?
  2. 서버와 클라이언트의 상태가 분리되어 있는가?
  3. 특정 상황에 무언가를 실행해야 하는가?
  4. 비동기 통신에 필요한 데이터가 서로 의존하는가?

top-down입니다. 잘 생각해보고, 현재 도밥의 코드를 수정해보세요. 코드를 작성하면서 고민하면 생각보다 답이 안 나오니까 종이에 직접 손으로 써보는 것을 추천드립니다.

디렉토리 구조는 직관적이어서 좋아요! 절대 경로도 잘 적용해줬네요. 하나 아쉬운 것은 apis 디렉토리의 index.ts 입니다. 자동완성하면 import apis from 'apis'; 이 될 수 있고, index.ts는 암묵적인 엔트리 포인트이므로 다른 개발자가 보았을 때 자칫 상호 참조를 구성할 수도 있어요.

고생 많았어요! 몇 가지 코멘트 확인해보고, 재요청 주세요!

src/components/@common/Accordion/AccordionProvider.tsx Outdated Show resolved Hide resolved
src/apis/cart/index.ts Outdated Show resolved Hide resolved
src/apis/index.ts Show resolved Hide resolved
src/utils/debounce.ts Show resolved Hide resolved
Comment on lines +5 to +8

const useCartPriceText = () => {
const { checkedCartProductIds } = useCartCheckBox();
const cartTotalPrice = useRecoilValue(checkedCartProductsTotalPriceState);

Choose a reason for hiding this comment

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

여기도 훅으로 관리하는 것보다 유틸리티로 분리하는 게 어떨까요?

src/components/CartPriceSection/CartPriceSection.tsx Outdated Show resolved Hide resolved
src/apis/points/index.ts Outdated Show resolved Hide resolved
src/apis/orders/index.ts Show resolved Hide resolved
@Creative-Lee
Copy link
Author

Creative-Lee commented Jun 9, 2023

안녕하세요 파랑😁
힘든 상황에도 신경써서 코멘트 달아주셔서 정말 진심으로 감사합니다..!
현재까지의 반영사항 담아 재PR 올립니다.
리뷰에 도움되도록 아래에 리뷰 반영 요약표를 첨부했습니다.
아래 질문도 이어집니다..

📜 리뷰 반영 요약표

번호 리뷰 내용 관련 코멘트 반영 커밋
1 외부 의존성 없는 함수 useCallback 적용해보기 #172 (comment) 15d40a7
2 map -> forEach #172 (comment) 6773b3d
3 context의 state, dispatch 분리해보기 #172 (comment) 8c498b7
4 불필요한 조건문을 삼항식으로 #172 (comment) 56d2e93
5 Header내 함수에 Memo, Callback 적용하기 #172 (comment) 아래 커밋과 함께 처리
6 SelectBox memo 적용 #172 (comment) a3aad30
7 불필요한 useCallback #172 (comment) ef488dc
8 불필요한 return #172 (comment) 아래커밋과 함께 처리
9 api 실패에도 실행되는 후행로직 #172 (comment) 425dd52
10 인라인 함수 #172 (comment) a69c602
11 구조분해 할당으로 라인 줄이기 #172 (comment) 70ac1ce
12 아코디언 컨텐츠 컴포넌트 관련 코멘트 #172 (comment) 질문 응답 대기중입니다😀
13 FlexBox의 수많은 props #172 (comment) 질문 응답 대기중입니다😀
14 Location 조작은 외부에서? #172 (comment) 질문 응답 대기중입니다😀
15 text 관련 훅은 유틸리티로..? #172 (comment) 질문 응답 대기중입니다😀
16 낙관적 업데이트와 방어처리 #172 (comment) 시도 했으나, 실패... 추가 학습이 필요함...

제가 이해하지 못한 내용이 너무 많아서 질문이 정말 많습니다! 넓은 이해를 바라요 파랑!! ㅠㅠ

파랑의 비동기 관련 본문 답변

사용자에게 필요한 에러인가?
서버와 클라이언트의 상태가 분리되어 있는가?
특정 상황에 무언가를 실행해야 하는가?
비동기 통신에 필요한 데이터가 서로 의존하는가?

더 많이 생각해 봐야겠지만, 정말 어렵네요. 특히 3, 4번 조건이 정말 어렵습니다....
어렴풋한 추측만 해볼수 있더라고요 ㅠㅠ

특정 상황에 무언가를 실행해야 하는가?

  • post, patch, delete 등의 로직을 실행한 후 get을 실행하여 상태업데이트를 해야하는가?
  • (선 상태 변경을 했다면) 위 로직이 실패한 후, 기존의 상태로 롤백해야 하는가?

비동기 통신에 필요한 데이터가 서로 의존하는가?

  • 통신시 전달해야하는 data(상태) 관련 이야기인 걸까요..?

관련해서 조금 더 힌트를 주실 수 있을까요?
4개의 조건으로는 생각이 진전되지 못하는 상태같아요...

Copy link

@InSeong-So InSeong-So left a comment

Choose a reason for hiding this comment

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

무지막지한 피드백을 주었는데 고생 많았습니다🙇‍♂️
일단 코멘트를 좀 남겨 두었으니 천천히 확인해주세요.

특정 상황에 무언가를 실행해야 하는가?

  • 현재 자신의 관점으로 보면 좀 어렵고, 내가 만든 API를 누군가가 import 해서 쓴다면이라는 조건으로 접근해보길 추천합니다.
  • axios 라이브러리는 interceptor등을 두어서 어떤 로직을 실행하기 처리하는 로직을 끼워 넣을 수 있게 만들어 두었습니다. 이러한 선행/후행 처리는 사용자로 하여금 풍부한 확장성을 느끼게 하거든요.

비동기 통신에 필요한 데이터가 서로 의존하는가?

  • A 통신에서 나온 결과 데이터로 B 통신에 parameter로 넣어야 하는 케이스를 일컫습니다.
  • 모든 API를 병렬적으로 설계해봐야 이러한 관계를 잘 풀어낼 수 있어요.

어려운 내용입니다🥲
조금씩 프로젝트를 하면서 느끼게 될 부분일 수도, 책을 읽다가 느낄 수도, 다른 사람의 코드를 보다가 깨달을 수도 있습니다.

긴 기간 고생 많았습니다 !

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.

None yet

3 participants