[1단계 - 장바구니] 캉골(김문경) 미션 제출합니다. #352
Conversation
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: kimyou1102 <[email protected]>
른 활성화 로직 추가 Co-authored-by: kimyou1102 <[email protected]>
Co-authored-by: Yugyeong Kim <[email protected]>
Co-authored-by: kimyou1102 <[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 (
|
There was a problem hiding this comment.
안녕하세요 캉골 ㅎㅎㅎ 오랜만입니다 👋
마지막 미션을 함께하게되서 기분이 좋네요 😄
우선 상태 설계의도에 대해 이야기해보면,
이번 미션에서는 원본 상태와 파생 상태의 구분에 신경을 많이 쓴 것 같습니다.
최소한의 state를 사용해서 잘 동작할 수 있도록 초점을 맞추었습니다
테이블로 너무 잘 정돈 되어있어서 인상깊었습니다ㅎㅎ 아주 좋은 방식인것 같아요.
(저도 이런 식으로 잘 정리해가며 학습했었다면 삽질을 덜 했을것 같네요 😅)
정리된 내용의 원본/파생 관계도 모두 올바르게 짜여진것 같습니다.
이번 상태관리에서 사용한 훅은
useState와useEffect만을 사용하여 관리하였습니다.
contextAPI를 사용하지 않은 이유는 전역 상태를 사용하지 않고도 충분히 구현 가능하다고 판단되었고, Router를 통한 페이지 이동 시에도useNavigate를 통해 전달할 수 있다고 생각했습니다.
이부분도 캉골의 의견에 어느정도 동의합니다.
다만, 구현 가능여부에 더불어서 '코드나 데이터의 흐름이 잘 정돈될 수 있냐~!' 도 판단기준이 될 수 있겠죠. 요런 부분도 생각해보시면 좋겠어요.
(사용하지 않아도 구현할 수 있나? + 만약 사용하면 어떤게 더 나아지나? )
고민했던 문제도 슬쩍 보면,
1️⃣ 장바구니 체크 버튼 동기화 문제
처음 페이지가 로딩될 때만 '전체 선택' 상태가 자동으로 동기화되도록 하고 싶었기 때문에,
'최초 진입 여부'를 판단할 수 있는 상태값을 따로 관리해야 했습니다.
그에 따라isLoading상태를 조건으로 사용하게 되었습니다.
최초 화면 로딩 상태와 데이터 fetching 상태를 구분해서 관리해야 하는 이유를 체감했습니다.
만약 장바구니 Data fetch -> 완료 시 Data를 기반으로 선택된 장바구니 상품 배열(클라이언트 상태) 초기화 의 순으로 해결할 수 있다면 참 좋겠죠?
관련된 내용을 코멘트로 남겨보았습니다~!
이어서 질문 주신 내용도 보면,
1️⃣ ContextAPI 사용
이번 미션에서 굳이 Context API를 사용할 필요는 없다고 판단했습니다.
그 이유는 페이지가 변경되었음에도 전역 상태가 유지되는 구조가 오히려 부자연스럽게 느껴졌기 때문입니다.
페이지 간 이동이라면, 상태 역시 필요에 따라 전달되고 초기화되는 것이 더 명확한 흐름이라고 생각했어요.
캉골의 생각도 일리가 있어요.
내용의 늬양스를 살짝만 바꿔서...
저는 '페이지가 변경되었을때 전역상태가 유지되는것은 부자연스럽다.' 라는 관점보다는
'페이지가 변경되었을 때도 유지되어야 하는 상태인가?'(페이지 끼리도 공유가 필요한 데이터인가?) 라는 관점으로 바라봐야한다고 생각해요!
페이지간 상태 공유를 통해 구현해야하는 요구사항도 충분히 있을 수 있으니까요~!
(= 상황에 따라 유지되는게 자연스러울수도, 부자연스러울수도 있다.)
(캉골이 어떻게 생각하느냐? 에 따라 결정.)
도밥은 이번 미션에서 ContextAPI를 사용하는 것이 얼마나 유용하다고 생각하시나요?
특히 페이지가 바뀌었음에도 불구하고 여전히 전역 상태를 유지하는 구조가
자연스러운 흐름이라고 볼 수 있는지도 고민이 됐어요.
이전 미션에서는 같은 페이지 안에서 컴포넌트 간의 데이터 공유를 위해 Context API를 사용했는데,
이번처럼 라우팅이 발생하는 상황에서도 Context를 쓰는 게 좋은 선택일지에 대해
좀 더 명확한 기준을 갖고 싶습니다.
자연스러운 흐름에 대해 물어보신 요 질문에는 위쪽에 드린 답변으로 갈음하겠습니다ㅎㅎ
context api 대해서는 기본적으로 '컴포넌트 트리에 얽매이지 않고 의존성을 주입할 수 있는 도구' 라는 관점을 가지고 계속 활용하며 기준을 세워보시기 바라요.
2️⃣ MSW 사용
좋은 고민을 해주신것 같아요.
말씀해주신 대로 MSW의 주 목적은 서버 작업 진행에 구애받지 않고, 서버 의존적인 작업을 진행할 수 있는것에 있겠죠.
위의 목적을 이루면서 그밖에 테스트 작성시에 도움이 되니 겸사겸사 사용한다면 괜찮다고 생각하는데, 테스트만을 위해 MSW를 구축한다고 하는것은 조금 앞뒤가 안맞지 않나 생각합니다.
개인적인 의견은 이렇지만 상황에 따라 또 다르게 생각할 수도 있을것 같네요ㅎㅎ
(크루들은 어떻게 생각하나, 다른 리뷰어들은 어떻게 생각하나 한번 이야기 나누고 둘러보는것도 좋겠습니다 😄)
고생많으셨습니다 캉골~ ㅎㅎ
마지막까지 힘내봐요 💪
| "dependencies": { | ||
| "@emotion/react": "^11.14.0", |
There was a problem hiding this comment.
[여러번 들었을수도 있는 질문]
이모션 선택의 이유가 궁금합니다.
There was a problem hiding this comment.
공교롭게도 이번 Level 2 과정 동안 CSS Module, styled-components, emotion 세 가지 스타일링 방식을 모두 사용해볼 수 있었습니다. 각각을 직접 사용해보면서 느낀 장단점을 간단히 정리해보았습니다.
styled-components
가장 처음 사용하게 된 스타일링 라이브러리는 styled-components였습니다.
함께 개발했던 페어가 해당 라이브러리를 이전에 사용해본 경험이 있었기 때문에 자연스럽게 선택하게 되었습니다.
✅ 장점 ) props를 통해 조건부 스타일링이 가능하다는 점이 유용했습니다.
☑️ 단점 ) 2025년 봄 기준으로 styled-components는 는 기능 개발이 중단되고 유지보수 모드로 전환된 상태라, 장기적인 관점에서 대규모 프로젝트에 사용하기에는 어려움이 있을 것 같다는 생각이 들었습니다.
☑️ 단점 ) CSS 스타일과 JavaScript 로직이 하나의 파일에 함께 존재하다 보니 점점 가독성이 떨어진다는 인상을 받게 되었습니다.
CSS Module
npm 에 배포할 모달 모듈을 만들때 사용하게 되었어요. 이전에 사용했던 styled-components의 단점을 보완하고자 스타일과 로직을 분리할 수 있는 방식을 선택하게 되었습니다.
✅ 장점 ) 별다른 라이브러리 설치 없이 바로 사용 가능했습니다.
✅ 장점 ) 스타일과 로직이 분리되어 있어 가독성이 높아졌습니다.
☑️ 단점 ) (치명적임) npm에 배포했을 때 가장 크게 문제가 되었던 부분입니다. npm 배포 시 스타일이 정상적으로 적용되지 않았다는 점입니다. 이를 해결하기 위해 별도의 설정이 필요했으며, 이 과정이 다소 번거로웠습니다.
emotion
앞선 두 가지 방식에서 아쉬운 점을 겪은 후, 마지막으로 선택한 라이브러리입니다.
emotion을 초반에 선택하지 않았던 이유는, 다양한 스타일링 방식을 제공한다는 것이 오히려 혼란을 줄 수 있을 것 같았기 때문입니다.
✅ 장점 ) styled-component에서는 props를 통한 조건부 스타일링 문법이 조금 어렵게 느껴졌습니다. 반면에 emotion에서는 JS 표현식을 그대로 사용할 수 있어 훨씬 직관적이고 쉽게 느껴졌습니다.
✅ 장점 ) 컴포넌트 단위(Ex. const Button = styled.button)로 스타일을 선언하기보단, className을 작성하는 방식이 더 가독성이 좋았던 것 같습니다.
☑️ 단점 ) className을 일일이 선언해줘야 했던 부분이 번거로웠습니다.
결론~
세 가지 스타일링 라이브러리를 모두 사용해본 결과, emotion이 가장 편리하고 익숙하게 느껴졌습니다. 다른 두 라이브러리(styled-components, CSS Module)는 각각 명확한 단점이 있었던 반면, emotion은 아직까지 뚜렷한 단점을 체감하지 못하기도 했구요.
- ) 개인적으로는 tailwindCSS를 더 좋아합니다 ㅎㅎ
There was a problem hiding this comment.
개인적으로 알아본 내용
너무 경험적인 측면에서 얘기한 부분이 있기도 하고, 기술적인 근거가 부족하다고 생각되어서 조금 더 찾아본 내용을 추가해봅니다!
혹시 내용 중에 부정확한 부분이 있다면 피드백 부탁드립니다 😊
📌 스타일 적용 방식
-
정적 스타일링 : 빌드 타임에 CSS가 완성되어 성능이 뛰어남 (
CSS Module)
→ 대규모 앱, 퍼포먼스 중심 앱에 유리 -
런타임 스타일링 : JS가 실행되면서 CSS가 동적으로 생성됨 (
styled-components,emotion)
→ 동적 스타일링 변화가 많을 경우 유리
📌 번들 크기 비교
- 예상 번들 크기 비교:
CSS module(거의 없음) <styled-component<emotion
🔍 참고: CSS Module은 별도 패키지가 아닌 빌드 도구 설정을 통해 사용하는 기능이기 때문에
번들 크기 측정 사이트(bundlephobia)에서는 직접 비교가 불가능합니다.
💬 정리하며
- 정적인 페이지나 성능이 중요한 환경에서는
css-module이 가장 적합하다고 느껴졌습니다. - npm에 모듈을 배포해야 하는 상황이라면, 번들 크기를 고려하여
styled-component의 선택지가 더 중요해질 수 있을 것 같아요.css-module은 별도의 설정이 필요해서 사용하지 않을 것 같습니다. - 일반적인 프로젝트에서 사용하게 된다면 저는 개인적으로
emotion을 가장 선호할 것 같습니다 😊
There was a problem hiding this comment.
이것저것 잘 정리해주셨네요 ㅎㅎ
npm 모듈 배포 맥락에서는 오히려 더 css-module을 사용해야하지 않을까? 싶었어요.
모듈 개발자 입장에서 귀찮음(이런 저런 설정)을 감수해서라도 사용자들에게 번들사이즈가 작은 라이브러리를 제공해야되지 않을까? 생각했습니다. 캉골처럼 번들포비아 등을 분석하는 사람이 많을테니까요~!
There was a problem hiding this comment.
앗 그러게요 🥹
생각해보니 모듈 배포 맥락이라면 말씀해주신대로 사용자의 입장을 더 고려해보는게 좋을 것 같습니다
너무 개발자 입장에서만 고려를 해봤었네요 😂
좋은 의견 감사합니다~!
| "msw": { | ||
| "workerDirectory": [ | ||
| "public" | ||
| ] |
src/App.tsx
Outdated
| <> | ||
| <h1>react-shopping-cart</h1> | ||
| </> | ||
| <BrowserRouter basename="/react-shopping-cart"> |
There was a problem hiding this comment.
[미션과 관계 없으니 나중에 슬쩍 봐보기]
createBrowserRouter 라는 인터페이스도 나중에 한번 찾아보셔요 ㅎㅎ
src/api/getShoppingCart.ts
Outdated
| const token = import.meta.env.VITE_APP_TOKEN; | ||
| const baseUrl = import.meta.env.VITE_BASE_URL; |
There was a problem hiding this comment.
공통 변수 및 기본 fetch로직(header, 기본에러 처리 등)을 포함한 fetcher 함수를 공통으로 만들어서 재사용하도록 해보면 좋겠습니다~!
모든 api에 이런 변수들을 매번 적어주긴 쪼금 귀찮으니까요 ㅎㅎ
(장바구니 관련된 api만 벌써 4개이기도 하네요😅)
There was a problem hiding this comment.
헉 그렇네요!
이전에는 apiClient라는 함수를 만들어서 사용했었는데 시간적인 여유가 없어서 미처 적용하지 못했던 것 같습니다.
apiClient라는 api 통합 함수를 만들어서 공통으로 사용 가능하도록 수정했습니다!
사용 예시를 아래 간단히 첨부해두겠습니다.
// 삭제
return apiClient.delete("cart-items", productId.toString());
// 가져오기
const params = new URLSearchParams({
page: String(page),
size: String(size),
sort: sort ?? "",
});
return await apiClient.get("cart-items", params);
// 수정
return apiClient.patch("cart-items", productId, {
id: productId,
quantity,
});
src/components/Button/Button.tsx
Outdated
There was a problem hiding this comment.
요 컴포넌트는 도메인과 상관없이 공통으로 사용할 수 있는 컴포넌트이니,
component/common/ 등의 디렉터리에 따로 보관해도 좋을 것 같네요 ㅎㅎ
(checkbox 등의 컴포넌트도 마찬가지입니다)
There was a problem hiding this comment.
앗 시간부족으로 마무리가 부족했던 부분인 것 같네요 🥹
변경된 폴더구조는 domain 중심 폴더구조를 적용해봤습니다.
이전 미션에서도 사용해본 경험이 있는데요, 꽤 괜찮은 구조라고 느껴져서 이번 미션에서도 그대로 가져가게 되었습니다.
domain 기반 구조의 장점은 공통적으로 사용하는 코드와 각 도메인에 특화된 코드를 명확히 분리 할 수 있다는 점입니다.
특히, 토스와 같은 기업에서도 도메인 기반 폴더 구조를 채택하고 있는 것으로 알고 있는데,
특정 기능을 제거할 때 해당 디렉토리만 삭제하면 관련 코드가 모두 정리되는 구조라는 점이 가장 인상깊었습니다.
반면 기능 중심 구조는 소규모 프로젝트에서는 유용할 수 있지만, 규모가 커질수록 관리가 어려울 것 같다는 생각이 들었어요.
아래는 이전 PR에서 사용했었던 폴더 구조입니다. 참고용으로 첨부드려요! :)
There was a problem hiding this comment.
저도 어느정도 규모가 있는 프로젝트에서는 도메인 기반의 디렉터리 구조가 더 효율적이라고 생각합니다ㅎㅎ
(사내에서도 비슷한 구조를 선택하고 있고요 ㅎㅎ)
| useEffect(() => { | ||
| if (!isLoading && cartItem) { | ||
| setSelectedCartId(cartItem.map((item) => item.id.toString())); | ||
| } | ||
| }, [isLoading, cartItem]); |
There was a problem hiding this comment.
풀어야하는 문제나 요구사항을 구현을 나열해보면,
-
장바구니 데이터 fetch 후, 최초 1회만 선택된 상품 id 배열을 초기화 해야한다.
-
캉골은 장바구니 상품 수량 변경시에, cartItem 상태를 변경하도록 구현했다.
캉골의 해결은 다음과 같아요.
- 최초 fetch 후
cartItem이 변경되었을 때 선택 여부를 초기화하는 effect를 작성. - 이것이 상품 수량 변경시
cartItem업데이트시에도 실행되어서 버그가 발생. - 이를 해결하기 위해 isLoading으로 처리
나쁘진 않지만, 그리 깔끔한 방법도 아닌것 같습니다.
우선 선택된 상품 배열 초기화와 상품 수량변경
이 둘의 관계를 끊어서 구현하는게 가장 이상적인 해결일것 같아요.
react에는 Suspense라는 개념이 있습니다.
이를 활용하면 장바구니 데이터 fetch가 완료될 때 까지 해당 컴포넌트 랜더링을 멈출 수 있습니다.(그동안은 fallback ui를 표시할수도 있고요)
핵심은 fetch 완료된 후 그 데이터를 기반으로 상태를 초기화 할 수 있다는 점입니다.
그럼 지금처럼 useEffect에서 setSelectedCartId 하는것과 뭐가다르냐는 의문이 생길 수 있는데,
기존처럼 setter를 사용하지 않고, useState의 초기값에 넣을 수 있다는 차이점이 있습니다.
대략 아래와 같은 구현이 가능해지겠죠.
const cartItems = useCartItems() // data fetch hook
// suspense 활용 시 promise가 resolve될 때까지 해당 컴포넌트 랜더링을 멈추고 다른 컴포넌트 랜더링을 수행. resolve되면 랜더링을 이어감.
const ids = cartItems.map((item) => item.id.toString())
const [selectedCartId, setSelectedCartId] = useState(ids)Suspense를 학습하고 적용해봤으면 좋겠습니다.
There was a problem hiding this comment.
말씀 주신 피드백을 바탕으로 Suspense를 적용해보려 했는데, 실제로 진행하면서 몇 가지 어려움이 있었습니다.
우선 두 가지 문제가 있었습니다.
- 현재 프로젝트의 리액트 버전이 18이라서, 데이터를 직접 가져오기 위해 사용하는
use()함수를 사용할 수 없다는 점이었습니다. - 두 번째는
use()를 쓰지 않고 데이터를 비동기적으로 처리하려면react-query같은 외부 라이브러리를 사용해야 하는데, 이번 과제에서는 새 라이브러리 설치가 제한되어 있어 사용할 수 없었습니다.
이런 이유로, 리액트 버전을 올려서 use()를 써보려고 시도해봤지만
use()를 처음 사용하는 것이다 보니, 정확한 사용법에 대한 감이 부족했고,
무엇보다 자동으로 use가 import되지 않아서 react-use.d.ts라는 타입 정의 파일을 직접 만들어야 하는 상황도 발생했습니다.
버전이나 설정 문제일 수도 있어서 정확히 파악하기가 어려웠습니다 ㅠㅠ
이러한 기술적 제약들 때문에 이번에는 Suspense 적용을 완료하지 못했습니다.
다른 프로젝트에서 suspense 도입
그래서 아예 별도로 간단한 프로젝트를 새로 만들어서 Suspense를 직접 적용해보았습니다.
기존 프로젝트에서는 버전 및 라이브러리 제한으로 어려움이 있었지만,
새 프로젝트에서는 React 버전을 19로 맞춰서 간단한 동작 방식을 확인해볼 수 있었어요
실제 사용한 예제는 다음과 같습니다:
// app.tsx
<Suspense fallback={<div>Loading...</div>}>
<h1>React Suspense Example</h1>
<SuspensePage />
</Suspense>그리고 SuspensePage에서는 use()를 활용해 데이터를 가져왔습니다:
export function SuspensePage() {
const data = use(
getCartItems({ page: 0, size: 10, sort: "desc" }) // ✅ 캐시된 Promise 반환
).content;
}데이터 fetch 함수는 아래와 같이 구현했는데, 동일한 요청에 대해서는 Promise를 캐싱해서 반환하도록 처리했습니다.
const cartCache = new Map<string, Promise<any>>();
export function getCartItems({ page, size, sort }: getCartItemsParams): Promise<any> {
const query = new URLSearchParams({
page: String(page),
size: String(size),
sort: sort ?? "",
});
const key = query.toString();
if (!cartCache.has(key)) {
const promise = fetch(`${import.meta.env.VITE_BASE_URL}/cart-items?${key}`, {
method: "GET",
headers: {
Authorization: `Basic ${import.meta.env.VITE_APP_TOKEN}`,
"Content-Type": "application/json",
},
}).then((res) => {
if (!res.ok) throw new Error("Failed to fetch cart items");
return res.json();
});
cartCache.set(key, promise);
return promise;
}
return cartCache.get(key)!;
}suspense를 사용해본 결과를 아래 gif 로 첨부합니다!
조금 급하게 코드를 작성하다 보니 아직 완전히 이해하고 활용했다고는 말하기 어렵지만 🥹
직접 구현해보면서 개념을 체감할 수 있었고, 특히 Suspense 기반 데이터 패칭 흐름에 대해 이해할 수 있는 좋은 경험이었습니다.
혹시 제가 시도한 방법 외에 현재 프로젝트에서 시도해볼 수 있을만한 방법이 있을까요?
There was a problem hiding this comment.
짧은시간동안 반영하기에는 조금 과할수도 있는 정보를 드렸는데, 훌륭하게 학습해주셨네요~! (힘들게해서 미안합니다...)
- 라이브러리 버전에 대해서는 react와 types/react 버전을 둘다 올려보신게 맞을지요~?
- use를 사용하기 힘들다면
Suspense구현체, Suspender, Suspense원리등의 키워드로 검색하면 나오는 구현체(아래 코드 예시)들을 우선 사용해볼 수 있겠습니다.- 아래 코드의 상세한 이해보다는, Suspense의 동작 원리정도는 이해하고 사용해보면 좋겠습니다.
- 다소 어려운 내용일 수 있습니다. 시간을 잘 조율해서 step2를 반영하고 따로 학습해보시면 좋겠습니다~!
export function wrapPromise<T>(promise: Promise<T>) {
let status = "pending";
let response: T;
const suspender = promise.then(
(res) => {
status = "success";
response = res;
},
(err) => {
status = "error";
response = err;
}
);
const read = () => {
switch (status) {
case "pending":
throw suspender;
case "error":
throw response;
default: // success
return response;
}
};
return { read };
}한가지 걱정인것은, 제가 suspense 적용을 기준으로 리뷰드린 코멘트들이 일부 있어서,
(선택된 장바구니 아이템 배열의 초기화 라던지)
사용하지 않았다는 조건하에는 어떤식으로 코드를 변경해야할지 조금 어려움이 있으실것 같아요.
당장은 지금의 구조에서 step2 요구사항을 우선적으로 진행하는것을 목표로 해봅시다..!
There was a problem hiding this comment.
짧은시간동안 반영하기에는 조금 과할수도 있는 정보를 드렸는데, 훌륭하게 학습해주셨네요~! (힘들게해서 미안합니다...)
아니예요 🥹
해당 방법을 알려주시지 않았다면 공부해볼 수 있는 기회가 없었을 것 같아요.
당장엔 조금 어려운 내용일지라도, 개념을 알고 있으면 앞으로 꼭 필요할 때 활용할 수 있을 거라 생각해요
이전에 suspense를 스치듯이 들은 적이 있었는데 이번 기회에 직접 사용해볼 수 있어서 좋았어요.
알려주셔서 감사합니다 ㅎㅎ 😊
라이브러리 버전에 대해서는 react와 types/react 버전을 둘다 올려보신게 맞을지요~?
앗 ㅎㅎ react의 버전만 올려 사용했던 것 같아요
역시나 .. 놓치고 있던 부분이 있었네요 🥹
use를 사용하기 힘들다면 Suspense구현체, Suspender, Suspense원리 등의 키워드로 검색하면 나오는 구현체(아래 코드 예시)들을 우선 사용해볼 수 있겠습니다.
오 이렇게도 사용할 수 있었네요
시야를 너무 좁게만 보고있었던 것 같습니다
step2를 진행하면서 여유가 된다면 함께 적용해보도록 하겠습니다. 감사합니다!
| <> | ||
| <CartProductContainer | ||
| cartItem={cartItem} | ||
| onChange={getCartItemData} |
There was a problem hiding this comment.
지금은 onChage라는 prop으로 일종의 refetch 및 cartItems상태 동기화를 함수를 전달하는 것 같아요.
상품을 delete하거나, 수량을 변경할 때(+, -) 항상 서버 data를 원천으로 동기화 하도록 설계하셨더라고요 ㅎㅎ
이런 기조라면, 이전 미션에서 만들었던 dataFetching hook(context를 활용한)을 적용해보면 어떨까 생각들었습니다!
물론 지금처럼 Page컴포넌트를 기점으로 prop으로 전달해도 괜찮지만, 생각보다 여러 곳/깊은 트리에서 해당 함수를 원하고 있어서요~!
또한 prop명이 최초 'onChange' 로 전달되고 이후에 여러 핸들러 함수로 랩핑되는데, 트리 깊은 곳까지 타고타고 가서 읽으려니 코드 흐름이 조금 어렵게 느껴지더라고요.
There was a problem hiding this comment.
생각보다 여러 곳/깊은 트리에서 해당 함수를 원하고 있어서요~!
처음에는 Context API를 사용할지 고민했지만,
도밥이 얘기해준 부분에 공감하면서 도입을 결정하게 되었습니다.
특히 onChange 함수가 생각보다 많은 컴포넌트를 거쳐 깊숙이 전달되고 있었고,
CartProductContainer가 받아야 할 props의 양이 많아지는 점도 크게 작용했어요.
Context API를 사용한 뒤로 props가 확연히 줄어든 것도 체감돼서, 전후 비교 예시를 함께 첨부합니다 😊
// ✅ before : props로 전달받았을 때
interface CartProductContainerProps {
cartItems: CartItemTypes[];
selectedCartIds: string[];
onDelete: (id: string) => Promise<void>;
updateCartItem: () => void;
handleCheckBox: (id: string) => void;
}
// ✅ after : contextApi 사용
interface CartProductContainerProps {
selectedCartIds: string[];
onDelete: (id: string) => Promise<void>;
handleCheckBox: (id: string) => void;
}추가로, selectedCartIds는 커스텀 훅으로 분리하면서 props drilling을 줄이기 위해
상태를 부모 컴포넌트로 끌어올리는 방식을 적용했습니다.
사용되는 범위가 대부분 페이지 내부로 한정되어 있고, 깊이가 깊지도 않아
Context API로 관리하지 않아도 되겠다는 판단이 들어 현재는 커스텀 훅으로 유지하고 있습니다.
| const handleCheckBox = (id: string) => { | ||
| if (id === "select-all") { | ||
| if (selectedCartId.length === 0) { | ||
| setSelectedCartId(cartItem.map((item) => item.id.toString())); | ||
| } else setSelectedCartId([]); | ||
| return; | ||
| } | ||
| if (selectedCartId.includes(id)) { | ||
| setSelectedCartId(selectedCartId.filter((itemId) => itemId !== id)); | ||
| } else { | ||
| setSelectedCartId([...selectedCartId, id]); | ||
| } | ||
| }; |
There was a problem hiding this comment.
id라는 인자로 전체선택이냐 아니냐를 분기하고 있는데,
전체선택 체크박스용 함수와 개별 선택용 함수로 분리하는게 가독성에 더 도움이 될 것 같습니다.
함수가 하나의 동작만을 포함하도록 해서 유지보수를 용이하게 하는 역할도 있고요.
There was a problem hiding this comment.
넵, 동의합니다! 앞서 커스텀 훅을 분리하는 과정에서 함께 정리된 부분입니다.
말씀해주신 것 처럼 **전체 선택용 함수(toggleSelectAll)**와 **개별 선택용 함수(toggleCartItem)**로 분리되었으며, 아래와 같이 사용하고 있습니다:
const handleCheckBox = (id: string) => {
if (id === "select-all") return toggleSelectAll();
else toggleCartItem(id);
};refactor: CartProductContainer 및 useSelectedCartIds 훅의 구조 개선 및 코드 정리
refactor: 아이템 삭제 후 selectedCartIds 배열에서의 id값 동기화 문제 해결
There was a problem hiding this comment.
함수는 분리는 잘 해주셨지만, 여전히 id를 통한 구현이 유지되어 있는데 이id 값이 하나의 개념을 나타내지 않기에 개념적인 혼동이 존재합니다.
(id가 항상 cartItem의 id가 아니고 어떤 상황에서는 "select-all" 와 같이 임의로 지정한 값임)
cartItem을 toggle하기위한 id 하나만 있으면 되지 않을까요?
if (id === "select-all") return toggleSelectAll(); 이 구문에서 select-all 이라는 임의의 id값을 지정하면서 까지 handleCheckBox 함수를 만들어 함수 하나만 prop으로 전달할 이유는 없는 것 같습니다.
이 코멘트는 Checkbox 컴포넌트에 달아놓은 코멘트와도 연계되어 있어요.
| const [cartItem, setCartItem] = useState<CartItemTypes[]>([]); | ||
| const [error, setError] = useState(""); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| const [selectedCartId, setSelectedCartId] = useState<string[]>([]); |
There was a problem hiding this comment.
넵! 적용해두었습니다. 😊
변수명을 지을 때 복수형과 단수형을 더 신경 써서 구분하도록 하겠습니다.
src/components/CheckBox/CheckBox.tsx
Outdated
| css={CheckBoxLayout} | ||
| id={id} | ||
| checked={isChecked} | ||
| onChange={() => onChange(id)} |
There was a problem hiding this comment.
체크박스 핸들러 함수를 분리 코멘트를 반영하고 나서,
이 CheckBox 컴포넌트의 인터페이스를 보다 표준적인 인터페이스로 변경하면 좋겠습니다.
(예를들어, input-checkBox의 onChange prop 콜백이 id를 전달하는 구조가 익숙하게 느껴지지 않습니다. - change event 객체를 예상함)
There was a problem hiding this comment.
더불어 외부에 check 관련 로직도 e.target.checked 등을 활용해서 구현할 수 있을 것 같아요!
There was a problem hiding this comment.
onChange prop 콜백이 id를 전달하는 구조가 익숙하게 느껴지지 않습니다
넵 동의합니다!
보통 on~ 형태의 이벤트 함수는 event 객체를 인자로 받는 경우가 많은 것 같아요.
onChange라는 인터페이스를 handleCheckBox로 명칭 변경했습니다!
|
추가로 현재 전체선택 관련한 버그가 있는것 같습니다~!
|
|
안녕하세요 도밥~! Context 관련 고민
결국 ContextAPI를 도입하긴 했지만! , 전체선택 버그 수정
문제는 장바구니 아이템에서 삭제할 때 |
| "dependencies": { | ||
| "@emotion/react": "^11.14.0", |
There was a problem hiding this comment.
이것저것 잘 정리해주셨네요 ㅎㅎ
npm 모듈 배포 맥락에서는 오히려 더 css-module을 사용해야하지 않을까? 싶었어요.
모듈 개발자 입장에서 귀찮음(이런 저런 설정)을 감수해서라도 사용자들에게 번들사이즈가 작은 라이브러리를 제공해야되지 않을까? 생각했습니다. 캉골처럼 번들포비아 등을 분석하는 사람이 많을테니까요~!
| return fetchFunction | ||
| .then((response) => { | ||
| if (!response.ok) | ||
| throw new Error(`잘못된 접근입니다: ${response.status}`); | ||
| if (!parseJson) return response; | ||
| return response.json(); | ||
| }) | ||
| .then((data) => data.content || data) | ||
| .catch((error) => { | ||
| throw new Error("API 통신중 오류가 발생했습니다 : " + error.message); | ||
| }); |
There was a problem hiding this comment.
try/catch + async/await 도 괜찮아 보이네요 ㅎㅎ
src/components/Button/Button.tsx
Outdated
There was a problem hiding this comment.
저도 어느정도 규모가 있는 프로젝트에서는 도메인 기반의 디렉터리 구조가 더 효율적이라고 생각합니다ㅎㅎ
(사내에서도 비슷한 구조를 선택하고 있고요 ㅎㅎ)
test/getTotalPrice.test.ts
Outdated
There was a problem hiding this comment.
가장 쉽게 접근해볼 수 있는 구조는 위쪽 domain 중심 폴더구조 코멘트에 첨부해주신 토스아티클 상의 디렉터리 구조인 것 같아요
디렉터리 구조에 정답은 없기에, 의도를 가지고 다양한 시도를 해보시고 그런 구조들이 유지보수 하기에 용이한지 스스로 평가해보시는것도 좋겠습니다 ~! ㅎㅎ
src/utils/getTotalPrice.ts
Outdated
There was a problem hiding this comment.
getTotalPrice 테스트 파일 또한 src/domains/shopping-cart/utils 에 같이 위치하면 된다고 생각합니다~! ㅎㅎ
(위쪽 코멘트로 인해 이미 반영된 것 같기도 하네요 ㅎ)
| @@ -19,7 +19,7 @@ export function CheckBox({ | |||
| css={CheckBoxLayout} | |||
| id={id} | |||
| checked={isChecked} | |||
| onChange={() => onChange(id)} | |||
| onChange={() => handleCheckBox(id)} | |||
There was a problem hiding this comment.
지금 이 변경에서는 여전히 이벤트 객체가 아닌, 외부에서 주입한 id를 그대로 콜백에 넣어 실행하는것으로 보입니다.
코멘트로 의도한 부분은 onChange 라는 네이밍의 변경이 아닌, 'event 객체 대신 id를 넣어 실행하는 구조를 변경해보자' 입니다.
CheckBox 컴포넌트의 인터페이스를 보다 표준적인 인터페이스로 변경하면 좋겠습니다.
표준적인 인터페이스 라는 표현이 바로 그 뜻이고요.
외부 사용처에서 input 태그의 랩핑 컴포넌트(그저 type='checkbox' 처리정도만 된)를 사용하는데에 있어서,
onChange의 타입이 어색하게 느껴진다는 말이었습니다.
input 태그 - onChange 타입을 그대로 사용할 수 있었으면 좋겠습니다.
(지금의 구현은 특별한 설계의도 없이, 마치 상품 선택 요구사항을 구현하기 위해 변경된 것처럼 느껴져요)
There was a problem hiding this comment.
코멘트로 의도한 부분은 onChange 라는 네이밍의 변경이 아닌, 'event 객체 대신 id를 넣어 실행하는 구조를 변경해보자' 입니다.
(지금의 구현은 특별한 설계의도 없이, 마치 상품 선택 요구사항을 구현하기 위해 변경된 것처럼 느껴져요)
앗 제가 의도를 잘 파악하지 못했던 것 같아요 🥹
마치 상품 선택 요구사항을 구현하기 위해 변경된 것 같다는 부분에 동의합니다!
만약 체크박스에 id가 아닌 다른 값으로 토글 여부를 판별해야 한다면 기존의 인터페이스를 다시 수정해야 하는 문제가 생길 것 같아요.
id를 바로 받아서 수정하기보다 event 객체를 받아서 그 속에서 id를 가져와 사용하는 방식으로 변경하였습니다!
const handleCheckBox = (e: React.ChangeEvent<HTMLInputElement>) => {
const id = e.target.id;
if (id === "select-all") return toggleSelectAll();
else toggleCartItem(id);
};- checkBox 인터페이스
interface CheckBoxProps {
isChecked: boolean;
handleCheckBox: (e: React.ChangeEvent<HTMLInputElement>) => void;
id: string;
dataTestId: string;
}| const handleCheckBox = (id: string) => { | ||
| if (id === "select-all") { | ||
| if (selectedCartId.length === 0) { | ||
| setSelectedCartId(cartItem.map((item) => item.id.toString())); | ||
| } else setSelectedCartId([]); | ||
| return; | ||
| } | ||
| if (selectedCartId.includes(id)) { | ||
| setSelectedCartId(selectedCartId.filter((itemId) => itemId !== id)); | ||
| } else { | ||
| setSelectedCartId([...selectedCartId, id]); | ||
| } | ||
| }; |
There was a problem hiding this comment.
함수는 분리는 잘 해주셨지만, 여전히 id를 통한 구현이 유지되어 있는데 이id 값이 하나의 개념을 나타내지 않기에 개념적인 혼동이 존재합니다.
(id가 항상 cartItem의 id가 아니고 어떤 상황에서는 "select-all" 와 같이 임의로 지정한 값임)
cartItem을 toggle하기위한 id 하나만 있으면 되지 않을까요?
if (id === "select-all") return toggleSelectAll(); 이 구문에서 select-all 이라는 임의의 id값을 지정하면서 까지 handleCheckBox 함수를 만들어 함수 하나만 prop으로 전달할 이유는 없는 것 같습니다.
이 코멘트는 Checkbox 컴포넌트에 달아놓은 코멘트와도 연계되어 있어요.
| try { | ||
| setLoading(true); | ||
| const response = await fetchFunction(); | ||
| setLoading(false); | ||
| if (updateLoading && response.ok) getCartItemData(); | ||
| return response; | ||
| } catch (error) { | ||
| setError(errorMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
로딩이 true인 상태에서 에러가 나면 false가 되지 않는 경우도 생기겠네요.
finally 등으로 처리해줘야할 것 같습니다.
There was a problem hiding this comment.
구현 측면에서는 에러나 로딩 관련 처리가 별도로 이루어져야 하지 않을까? 싶었어요.
지금은 삭제, 수정 등의 핸들러 함수를 같이 내보내며, 에러 로딩 상태도 하나로 같이 사용합니다.
(그만큼 상세한 처리가 어려워질 수 있겠죠.)
그러면에서 최초 분리 커밋의 useShoppingCart 정도가 나은 것 같다는 생각도 했습니다.(get api와 그에 관련한 상태만 포함한 훅)
다만, 개별 처리가 필요없다면(혹은 하지 않을것이라면) 지금의 구조도 괜찮아 보입니다.
step2에 요구사항에서도 문제가 없다면 캉골의 의도대로 유지하셔도 좋겠습니다.
There was a problem hiding this comment.
구현 측면에서는 에러나 로딩 관련 처리가 별도로 이루어져야 하지 않을까? 싶었어요.
지금은 삭제, 수정 등의 핸들러 함수를 같이 내보내며, 에러 로딩 상태도 하나로 같이 사용합니다.
아직은 개별 처리를 해주지 않고 있어서 그리 복잡하다는 생각은 하지 못했던 것 같은데
프로젝트의 규모가 커질수록 분리가 불가피해질 것 같단 생각이 들어요
마찬가지로 step2를 진행하면서 변경이 필요하다고 판단된다면 상태를 따로 분리해서 사용해보겠습니다.
| async function withErrorHandling( | ||
| fetchFunction: () => Promise<Response>, | ||
| errorMessage: string, | ||
| updateLoading?: boolean |
There was a problem hiding this comment.
refetch 여부를 결정하는 인자로 이해했는데 맞을까요?
맞다면 updateLoading 이라는 인자명이 다소 이해하기 어려웠습니다.
There was a problem hiding this comment.
넵 맞습니다!
데이터를 새로 받아와야 할 때 일반적으로 refetch라는 용어를 사용하는 것을 이번에 알게 되었어요.
관용적인 표현을 잘 몰라서 처음엔 update라는 이름을 사용했는데
이번에 refetch라는 용어로 변경해서 사용하도록 하겠습니다.
async function withErrorHandling(
fetchFunction: () => Promise<Response>,
errorMessage: string,
shouldRefetch?: boolean
}()| useEffect(() => { | ||
| if (!isLoading && cartItem) { | ||
| setSelectedCartId(cartItem.map((item) => item.id.toString())); | ||
| } | ||
| }, [isLoading, cartItem]); |
There was a problem hiding this comment.
짧은시간동안 반영하기에는 조금 과할수도 있는 정보를 드렸는데, 훌륭하게 학습해주셨네요~! (힘들게해서 미안합니다...)
- 라이브러리 버전에 대해서는 react와 types/react 버전을 둘다 올려보신게 맞을지요~?
- use를 사용하기 힘들다면
Suspense구현체, Suspender, Suspense원리등의 키워드로 검색하면 나오는 구현체(아래 코드 예시)들을 우선 사용해볼 수 있겠습니다.- 아래 코드의 상세한 이해보다는, Suspense의 동작 원리정도는 이해하고 사용해보면 좋겠습니다.
- 다소 어려운 내용일 수 있습니다. 시간을 잘 조율해서 step2를 반영하고 따로 학습해보시면 좋겠습니다~!
export function wrapPromise<T>(promise: Promise<T>) {
let status = "pending";
let response: T;
const suspender = promise.then(
(res) => {
status = "success";
response = res;
},
(err) => {
status = "error";
response = err;
}
);
const read = () => {
switch (status) {
case "pending":
throw suspender;
case "error":
throw response;
default: // success
return response;
}
};
return { read };
}한가지 걱정인것은, 제가 suspense 적용을 기준으로 리뷰드린 코멘트들이 일부 있어서,
(선택된 장바구니 아이템 배열의 초기화 라던지)
사용하지 않았다는 조건하에는 어떤식으로 코드를 변경해야할지 조금 어려움이 있으실것 같아요.
당장은 지금의 구조에서 step2 요구사항을 우선적으로 진행하는것을 목표로 해봅시다..!
안녕하세요 도밥~~ 캉골이라고 합니다.
미션 1때 한번 뵈었었는데, 이번에 다시 만나게 되었네요!! 😊
갑자기 마지막 미션이라고 하니 아쉽기도 하고, 정말 열심히 지내왔구나 하는 뿌듯함도 느껴져요 🥹
도밥을 통해 많은 배움의 기회들이 있었어서, 이번에도 많은 기대를 안고 PR 요청을 해봅니다
📦 장바구니 미션
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
RTL 테스트 케이스를 모두 작성했나요?
배포한 데모 페이지에 정상적으로 접근할 수 있나요?
리뷰어가 장바구니 추가를 쉽게 할 수 있도록 Curl 명령어를 DM 으로 전달해주세요.
리뷰 요청 & 논의하고 싶은 내용
1) 상태 설계 의도
이번 미션에서는 원본 상태와 파생 상태의 구분에 신경을 많이 쓴 것 같습니다.
최소한의 state를 사용해서 잘 동작할 수 있도록 초점을 맞추었습니다
페어와 함께 의논한 상태 설계는 다음과 같습니다
cartItemCartItemTypes[]selectedCartIdstring[]isCheckedbooleanselectedCartIdselectedCartId.lengthnumberselectedCartIdtotalPricenumbercartItem, selectedCartIddeliveryFeenumbertotalPricetotalPrice + deliveryFeenumbertotalPrice, deliveryFeedisabledbooleancartItem, selectedCartIdselectedCartTypenumberselectedCartIdselectedCartItemnumbercartItem, selectedCartIdtotalPricenumbertotalPrice테이블: 장바구니 속성 파생 관계 (관련 원본)
이번 상태관리에서 사용한 훅은
useState와useEffect만을 사용하여 관리하였습니다.contextAPI를 사용하지 않은 이유는 전역 상태를 사용하지 않고도 충분히 구현 가능하다고 판단되었고,Router를 통한 페이지 이동 시에도
useNavigate를 통해 전달할 수 있다고 생각했습니다.2) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
1️⃣ 장바구니 체크 버튼 동기화 문제
장바구니에서 상품이 선택되지 않은 상태에서
+또는-버튼을 클릭하면,장바구니 아이템이 다시 불러와지면서 checkbox 값이 초기화 되는 문제가 있었습니다.
이 문제는
cartItem값이 변경될 때마다selectedCartId값을 전체 갱신하면서 발생했습니다.즉, 버튼을 눌러 수량만 조절하고 싶을 뿐인데도 체크박스 선택 상태가 초기화되어버린 것입니다.
✅ 해결방법
처음 페이지가 로딩될 때만 '전체 선택' 상태가 자동으로 동기화되도록 하고 싶었기 때문에,
'최초 진입 여부'를 판단할 수 있는 상태값을 따로 관리해야 했습니다.
그에 따라
isLoading상태를 조건으로 사용하게 되었습니다.getShoppingCart()를 통해 데이터를 받아오기 전에,cartItem배열의 길이가 0이라면 사용자가 처음 페이지에 진입한 것으로 판단하고,그 시점에만
isLoading을false로 변경하도록 했습니다.💡 깨달은 점
위의 과정을 거치면서 깨달은 점이라면,
최초 화면 로딩 상태와 데이터 fetching 상태를 구분해서 관리해야 하는 이유를 체감했습니다.
이번 미션에서는 최초 진입 시 화면 초기화에만 초점을 맞췄지만,
조금 더 시간이 널널했더라면 데이터 갱신 상태에 대해서도 관리하고 싶습니다.
3) 이번 리뷰를 통해 논의하고 싶은 부분
1️⃣ ContextAPI 사용
다른 크루들의 구현 사례를 들어보니, Context API를 사용한 경우도 있었습니다.
왜 Context API를 사용했는지 물어보니, '주문 확인 페이지'에서 사용할 데이터를 전역 상태로 관리하고,
그 값을 Context를 통해 불러오기 위함이었다고 하더라구요.
하지만 제 경우에는 이번 미션에서 굳이 Context API를 사용할 필요는 없다고 판단했습니다.
그 이유는 페이지가 변경되었음에도 전역 상태가 유지되는 구조가 오히려 부자연스럽게 느껴졌기 때문입니다.
페이지 간 이동이라면, 상태 역시 필요에 따라 전달되고 초기화되는 것이 더 명확한 흐름이라고 생각했어요.
저는
useNavigate를 활용해 필요한 데이터를 state로 전달하는 방식으로 구현했습니다.이 방법만으로도 충분히 필요한 데이터를 다음 페이지로 넘길 수 있었고,
추가적인 전역 상태 관리 없이도 기능 구현에는 무리가 없었습니다.
💡도밥에게 묻고 싶은 점
도밥은 이번 미션에서 ContextAPI를 사용하는 것이 얼마나 유용하다고 생각하시나요?
특히 페이지가 바뀌었음에도 불구하고 여전히 전역 상태를 유지하는 구조가
자연스러운 흐름이라고 볼 수 있는지도 고민이 됐어요.
이전 미션에서는 같은 페이지 안에서 컴포넌트 간의 데이터 공유를 위해 Context API를 사용했는데,
이번처럼 라우팅이 발생하는 상황에서도 Context를 쓰는 게 좋은 선택일지에 대해
좀 더 명확한 기준을 갖고 싶습니다.
2️⃣ MSW 사용
이번 미션에서 테스트는 MSW + RTL를 사용해서 구현하게 되었는데요,
한 가지 의문이 들었습니다.
이미 서버가 요구사항에 맞게 잘 구현되어 있는 상황에서,
MSW를 사용하는 것이 맞는 선택일까? 하는 생각이 들었어요.
서버가 충분히 안정적이고 명확하게 구축되어 있다면
별도의 Mock Server 없이도 고정된 모킹 데이터만으로도 테스트가 가능하지 않을까 싶었습니다.
MSW는 이전 미션에서처럼 아직 서버가 준비되지 않은 상황에서
가짜 API를 만들어 테스트할 때 더 유용하지 않을까 하는 생각이 들었어요.
MSW 환경을 구성하고 유지하는 데도 생각보다 손이 많이 가니까요.
어떤 상황에서 MSW를 사용하는 것이 더 적절한 선택인지,
반대로 간단한 모킹 데이터만으로 충분한 경우는 어떤 경우인지
그 기준을 어떻게 잡아야 할지 궁금합니다.
✅ 리뷰어 체크 포인트
1. 클라이언트 상태관리
2. MSW/Test