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

[2단계 - 장바구니 미션] 버건디(전태헌) 미션 제출합니다. #286

Merged
merged 40 commits into from
May 31, 2024

Conversation

brgndyy
Copy link

@brgndyy brgndyy commented May 24, 2024

배포링크


안녕하세요 브콜 ! step2로 다시 찾아 뵙게 되었습니다.

❗️ step1과 마찬가지로 사이트 설정 -> 안전하지 않은 콘텐츠 -> 허용을 해주시면 배포 사이트 확인이 가능합니다 ❗️

이번 미션이 개인적으로 제일 재밌기도 했고, 생각해볼수 있는 부분이 많았던 미션이어서 많이 배워가는 중인데요!

많이 부족한 코드이지만, 놓치고 있는 부분 말씀 주시면 성장하는데에 큰 도움 될거 같습니다 🙇

이번 미션을 진행하면서 가장 크게 배우고 느꼈던 부분은 전역 상태 관리 레이어 인데요.

해당 부분 및 그 외 여쭤보고 싶은 질문들 코멘트로 남겨놓겠습니다.

이번 step2도 잘부탁드립니다!

brgndyy added 30 commits May 24, 2024 14:20
Copy link
Author

Choose a reason for hiding this comment

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

이번에 돈 계산 관련된 로직이 많다보니, 테스트 코드의 유용함을 많이 느낄 수 있었습니다!

그래서 테스트 코드도 각 케이스 별로 나눠서 짜보았는데요.

한번 봐주시면 감사하겠습니다 🙇

여기서 궁금한점은, 브콜은 현업 생활 하시면서 테스트 코드를 많이 짜시나요 ?

사실 현재 우테코에서 배워가는 과정에 있다보니 테스트 코드를 짜는게 당연히 큰 도움되고 얻어갈수 있는 부분이 크다고 생각하는데요.

아무래도 현업에서는 시간이 굉장히 촉박하다보니 테스트 코드를 도입하는거 자체가 큰 비용이 되지 않을까 ?

만약에 초기부터 테스트 코드 문화가 잡혀있지 않다면, 중간에 도입하기엔 굉장히 어려운 문화 아닐까 생각이 들었습니다.

아직 현업에 임해보지 못해서 드는 추상적인 걱정일수도 있는데, 현업에 실제로 계신 브콜께 한번 여쭈어 보고 싶습니다!

Choose a reason for hiding this comment

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

이거 답변을 못드렸었네요!
말씀하신 것처럼 현업에서 테스트 코드를 짜는 게 생각보다 큰 비용이더라구요~ㅎㅎ
특히 비교적 빠른 주기로 바뀌는 View영역은 테스트 보다 QA에 힘을 쏟는데 더 낫지않나라는 생각을 하기도 합니다.
그럼에도 최소한의 테스트 코드를 짜려고 하는 편이에요. 재사용성이 높은 유틸이나 도메인과 깊게 관련있는 로직 등에는 테스트를 자세히 합니다. 그리고 저희 팀은 자체적으로 디자인 시스템, 위지윅 에디터 등을 만들어 사용하는데요! 이런 공통 요소들에는 ladle(storybook 같은 친구에요!)을 붙여 최대한 자세히 유즈케이스를 명세합니다!

Copy link
Author

@brgndyy brgndyy May 24, 2024

Choose a reason for hiding this comment

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

서버상태의 초기 셋팅 레이어

저번 장바구니 아이템도 그렇고, 이번에 쿠폰도 그렇고 서버에서 받아온 상태를 아톰에 넣어주기 전에,

  1. 초기에 fetch를 통해 데이터를 받아온다.
  2. 포맷팅 레이어를 두어서 필요한 프로퍼티들을 추가해준다.

의 흐름을 가지고 있습니다.

이번에도 쿠폰을 받아올때, 현재 장바구니에 담은 아이템들의 총 가격, 갯수들을 기반으로

isAvailable => 초기에 disabled로 해줄것인가 말것인가?
isChecked => 체크표시가 되어있는가 아닌가 ?

같은 클라이언트측에서 필요한 프로퍼티들을 추가해주고 있는데요!

이렇게 하면, 전 관리가 필요한 상태 들이 응집성 있게 되어서 상태추적에 용이하다고 생각했습니다.

결국 이 상태는 근본 atom이 되어서, 그 후에 파생 상태들 또한 쉽게 만들어줄수 있다고 판단했구요!

스크린샷 2024-05-24 오전 8 11 50

사진으로는 이렇게 나타낼수 있을거 같은데요.

근데

disabled나 checked 값은 클라이언트에서 다루어야하는 상태인데, 서버상태에 해당 프로퍼티들을 넣어주는것이 옳은가?

에 대한 의구심도 들었습니다.

제가 놓치고 있는 부분이나 더 생각해보아야할 점이 있다면 말씀주시면 감사하겠습니다!

Choose a reason for hiding this comment

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

  • 서버 상태는 무엇인가요?
  • 그 '서버 상태'에 disabled와 checked를 왜 넣어주면 안되나요?

Copy link
Author

Choose a reason for hiding this comment

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

서버 상태는 무엇인가요?

말그대로 서버에서 받아오는 백엔드 데이터이고, 여러 클라이언트에서 조작이 가능한 값이라고 알고 있습니다.

그 '서버 상태'에 disabled와 checked를 왜 넣어주면 안되나요?

disabled와 checked 같은 경우, 클라이언트측과 긴밀하게 연결이 되어있는 값이라고 판단했어요!

예를 들어서 쿠폰 같은 경우, 사용자가 얼마나 쿠폰 체크를 했냐는 것에 따라서 disabled 상태가 정해지고,

checked 도 마찬가지라고 생각했습니다!

그렇기 때문에 받아온 클라이언트에서 관리 되어야할 값이 서버상태와 혼용 되는건가..? 라는 생각도 들었는데요.

제가 놓치고 있는 부분이 있다면 말씀주시면 감사하겠습니다 🙇

Choose a reason for hiding this comment

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

정의상 서버상태도 서버에서 클라이언트로 넘어온 순간부터 이미 클라이언트에서 관리되는 상태 아닌가요?
'서버 상태라서 해당 필드를 넣지 말아야겠다' 말이 그렇게까지 공감은 안되는 것 같습니다.

오히려 버건디가 쎄한 느낌을 받는 건 관심사의 분리 차원에서 순수한 모델과 메타 데이터의 성격을 가지고 있는 정보들을 분리해야겠다는 생각이 아니었을까 싶어요!
서버에서 온 상품에 대한 정보를 가지고 그 나름의 모델은 생성하고 그 안에 메타데이터 형태로 disable같은 친구들을 두면 어떨까 제안 드려보니다.

interface CartItemMeta {
  disabled: boolean
  checked: boolean
}

interface Product {
  ...
}

interface CartItem extends Product {
  meta: CartItemMeta
}

Copy link
Author

@brgndyy brgndyy May 30, 2024

Choose a reason for hiding this comment

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

관심사의 분리 차원에서 순수한 모델과 메타 데이터의 성격을 가지고 있는 정보들을 분리

좋은 말씀 감사합니다!

현재 그래서 타입이

FormattedCoupon 이나 FormattedProduct 처럼 따로 포맷팅을 해준 후의 타입을 지정해준 상태인데요.

Meta라는 명세가 더 와닿는거 같아요! 🙇

Copy link
Author

Choose a reason for hiding this comment

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

- 복잡한 타입 선언

step1 때는 recoil 자체를 처음 써보다보니 일단 되는대로 구현을 했었습니다.

물론 지금도 당연히 recoil과 친한건 아니지만, 기능 구현이 다 되어있는 상태에서 코드를 살펴보니 너무 하드코딩 느낌으로 파생상태들을 선언해주고 이를 무지성으로 사용하고 있다는 느낌이 들었어요!

그래서 이를 한곳에서 관리해주는 레이어를 하나 더 만들어보았는데요!

useManager가 리코일의 selector, action, state를 한곳에 모아주는 역할을 하고 이를 필요한 도메인 별로 useCartManager, useCouponManager, useShippingManager로 나누어보았습니다.

다만, 이 useMananger 코드 내부애서 타입들이 any로 대부분 선언이 되어있어서 상태값들을 불러올때 타입추론이 안되는 상황이에요 🥲

스크린샷 2024-05-24 오전 8 29 00

이런식으로 사용하는곳에서 직접 타입지정을 해주어야하는데요.

타입스크립트가 익숙치 않아서, 중앙 관리자 역할(useManager)의 타입이 동적으로 결정될수 있도록 하고 싶은데 아직 감조차 잡히지 않아 이는 구현을 하지 못한 상태입니다.

이번에 이를 구현해보면서 들었던 궁금증인데요.

현업에서는 타입스크립트의 기능들을 어느정도로 활용할까요?

useManager에서 사용 된 타입들도 직관적으로 한번에 이해가 되는 타입이 아니라서 따로 공부해볼 예정인데요!

현업에서 이런식의 복잡한 타입 관리? 같은걸 많이 하는 편인가요 ?

단순히 받아오는 api에 대한 타입선언을 넘어서서, 복잡한 타입선언을 해주는 경우는 언제인지, 많은 경우가 그러는지 여쭙고 싶어요!

Choose a reason for hiding this comment

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

동적으로 타입을 결정 => 사용처마다 유연하게 타입을 결정 하는 것으로 이해해본다면 제네릭을 적극적으로 사용해보시면 어떨까요? 제네릭이나 타입 유틸 등을 이용해 정확한 타입 추론을 제공하는 건 충분히 익숙해지셔야 합니다~
복잡하다고 안정성을 포기할 수는 없으니까요~
현업에서 많이 쓰냐라고하면.. 뭐 많은 회사를 경험해 본 건 아니라 잘모르겠지만 적어도 저희 팀에서는 세세하게 타입을 잡으려고 많이 애를쓰구요..! 재사용성이 높은 함수, 다양한 유틸 등을 구현하다보면 필연적으로 복잡한 타입이 필요합니다. 그런 함수들도 안정적으로 잘 쓰려면 복잡한 타입들도 익숙하게 잘 써야겠죠!
혹시 타입에 무심한 개발팀이 있다면 그건 그 팀의 문제라고 생각해요. 실제로 많이 쓰냐 마냐 그런건 별로 궁금해 하시지 않으셨으면 좋겠어요. 무엇이 좋은 제품을 만드는가를 기준으로 생각하시면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

무엇이 좋은 제품을 만드는가를 기준으로 생각하시면 좋겠습니다.

좋은 말씀 정말 감사합니다! 🙇

<Button variant="footer" disabled={true}>
결제하기
<Button variant="footer" onClick={handleBackButtonClick}>
장바구니로 돌아가기
Copy link
Author

Choose a reason for hiding this comment

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

PR을 올린후에 추가적으로 장바구니에 담았던 제품들이 초기화 되어야한다는 추가 요구사항을 받았는데요!

리뷰 받은 후에 수정사항 반영해보면서 추가적으로 구현해보도록 하겠습니다

Copy link

@Tanney-102 Tanney-102 left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디!
두번째 단계도 고생 많으셨습니다!
작성해주신 useManager훅과 질문주신 내용들 위주로 보려고 했어요! 또 추가로 신경쓰시면 좋을 내용들 코멘트 남겨두었으니 확인부탁드립니다!

src/api/Fetcher.ts Show resolved Hide resolved
src/utils/formatDateToKorea.ts Outdated Show resolved Hide resolved
src/store/cartStates.ts Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

  • 서버 상태는 무엇인가요?
  • 그 '서버 상태'에 disabled와 checked를 왜 넣어주면 안되나요?

Choose a reason for hiding this comment

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

동적으로 타입을 결정 => 사용처마다 유연하게 타입을 결정 하는 것으로 이해해본다면 제네릭을 적극적으로 사용해보시면 어떨까요? 제네릭이나 타입 유틸 등을 이용해 정확한 타입 추론을 제공하는 건 충분히 익숙해지셔야 합니다~
복잡하다고 안정성을 포기할 수는 없으니까요~
현업에서 많이 쓰냐라고하면.. 뭐 많은 회사를 경험해 본 건 아니라 잘모르겠지만 적어도 저희 팀에서는 세세하게 타입을 잡으려고 많이 애를쓰구요..! 재사용성이 높은 함수, 다양한 유틸 등을 구현하다보면 필연적으로 복잡한 타입이 필요합니다. 그런 함수들도 안정적으로 잘 쓰려면 복잡한 타입들도 익숙하게 잘 써야겠죠!
혹시 타입에 무심한 개발팀이 있다면 그건 그 팀의 문제라고 생각해요. 실제로 많이 쓰냐 마냐 그런건 별로 궁금해 하시지 않으셨으면 좋겠어요. 무엇이 좋은 제품을 만드는가를 기준으로 생각하시면 좋겠습니다.

src/services/selectMaxDiscountCase.ts Outdated Show resolved Hide resolved
src/services/selectMaxDiscountCase.ts Outdated Show resolved Hide resolved
src/services/calculateDiscount.ts Outdated Show resolved Hide resolved
src/services/calculateDiscount.ts Outdated Show resolved Hide resolved
src/pages/cart/components/Cart.tsx Outdated Show resolved Hide resolved
Copy link

@Tanney-102 Tanney-102 left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디! 확인이 늦었습니다.
많은 부분 반영해주셔서 감사해요!
추가 코멘트 몇개 남겨 두었으니 확인 해주세요! 당장 변경이 클 것 같다면 고민만 잘 해보시고 우선 머지해도 괜찮을 것 같습니다!

Copy link

@Tanney-102 Tanney-102 left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디~ 마지막까지 정말 고생 많으셨습니다.
이번 미션의 아쉬움을 거름 삼아 상태관리 라이브러리들을 더욱 폭 넓은 관점으로 바라보게 되시기를 바라며 이만 마무리하겠습니다!!

@Tanney-102 Tanney-102 merged commit 6e82b2a into woowacourse:brgndyy May 31, 2024
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.

2 participants