-
Notifications
You must be signed in to change notification settings - Fork 204
[1단계 - 장바구니] 메타(조유담) 미션 제출합니다. #336
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
Conversation
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 메타 만나서 반갑습니다 ~~ 마지막 미션 잘 부탁드려요 !
전체적으로 코드를 일관성 있고 깔끔하게 잘 작성해주셔서 코멘트 남길 내용이 많지 않았던거 같아요. 코드를 보면서 개선시켜 볼만한 부분들과 궁금한 부분들에 코멘트 남겼으니 확인해주시면 좋을거 같아요!
즉, 테스트는 어떤 기준값에 의존해야 할까요?
"기능의 목적"을 기준으로 테스트해야 할지, 아니면 "mocking된 초기 상태"에 의존해도 되는 건지 리뷰어의 의견이 궁금합니다.
그런데 의존하지 않게 테스트 코드를 짜는 법도 잘 모르겠습니다…
질문의 내용을 완전히 이해하지 못 했는데요. 메타가 말씀하신 "기능의 목적"이 구현이 변경되더라도 기능이 동일한 경우를 말씀하시는 것이라면 기능의 목적에 따라 테스트를 작성하는건 좋은 방법이라고 생각합니다.
테스트는 개발자의 의도대로 코드가 동작하는지 확인하기 위한 목적으로 작성 되는데요. 그렇기 때문에 어떤 의도로 테스트를 작성했는지가 중요하다고 생각해요. 만약 첫 렌더링시 모든 체크 박스가 체크되어있다
라는 명제를 놓고 봤을 때 이 동작을 테스트 하기 위해 여러 방법이 있을텐데요. 초기의 Mock Data에 의존하지 않고 구현한다면 별개의 동작으로 나눠서 접근해봐도 좋을거 같아요
- 컴포넌트 테스트 -> ex) checked가 true일 때 체크된 상태로 렌더되어야 한다.
- 상태관리 hook 테스트 -> ex) 초기 상태의 아이템은 모두 체크된 상태여야 한다.
메타의 질문 의도와 벗어난 지점에서 제가 답변을 드렸거나 더 얘기해보고 싶은 부분이 있다면 컨텍스트를 더 파악할 수 있게 조금 더 상세한 내용과 함께 다시 질문 주시면 좋을거 같습니다!
미션 진행하느라 고생 많으셨습니다 ~~ 👍 👍 👍
src/App.tsx
Outdated
<ErrorContextProvider> | ||
<ApiProvider> | ||
<div css={RoutesStyle}> | ||
<Router> |
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.
최신 버전의 react-rotuer에서는 createBrowserRouter 사용이 권장되는데요. 어떤 부분이 어떻게 다른지 찾아보고 적용해보셔도 좋을거 같아요
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.
refactor: createBrowserRouter 적용
공식 문서에서도 강조하듯이, createBrowserRouter는 React Router v6.4부터 도입된 데이터 API (loader, action, errorElement 등) 를 활용할 수 있는 방식으로, 더 구조적인 라우팅 구성이 가능합니다.
- 데이터 패칭과 라우팅의 통합 (loader/action)
- 정적 라우트 선언을 통한 테스트 용이성
- SEO 및 서버 사이드 렌더링(SSR)에 더 유리한 구조
등의 장점 때문에 createBrowserRouter 기반의 사용이 현 시점에서 공식적으로도 권장되고 있는 상황이네요.
이번 프로젝트에서는 아직 loader나 action은 쓰지 않고 있지만,
장기적으로 확장성을 고려했을 때 createBrowserRouter 방식으로 리팩토링해두는 게 더 나은 선택이라 판단되어 적용해봤습니다 :)
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/App.tsx
Outdated
<Route path="/" element={<CartPage />} /> | ||
<Route path="/order" element={<OrderPage />} /> |
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.
path들은 상수화 해서 사용해주셔도 좋을거 같네요
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/api/deleteCartItem.ts
Outdated
headers: { | ||
Authorization: `Basic ${btoa(`${import.meta.env.VITE_USER_ID}:${import.meta.env.VITE_PASSWORD}`)}`, | ||
'Content-Type': 'application/json' | ||
} |
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.
api를 요청하는 부분에서 반복적으로 사용되는 부분들이 눈에 들어오는데요. 추상화를 통해 중복되는 코드를 줄여보셔도 좋을거 ㄱ타아요
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.
9a1be91
여기서 추상화를 시도해봤습니다!
src/api/deleteCartItem.ts
Outdated
if (cartItemId === undefined) { | ||
throw new Error('cartItemId가 정의되지 않았습니다.'); | ||
} |
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.
이 부분은 parameter에 대한 부분 (클라이언트의 문제)이고
if (!res.ok) { | ||
throw new Error('장바구니에서 상품을 삭제하는 데 실패했습니다.'); | ||
} |
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.
이 부분은 server 요청의 문제(서버, 네트워크 등)인데요. 같은 레벨로 처리되고 있는게 조금 어색하게 느껴지는거 같아요.
deleteCartItem
은 서버와의 요청을 통해 응답을 반환하거나 에러를 throw하는 책임만 지게 하고, parameter로 받는 cartItemId
를 string 타입으로 정의하고, parameter에 대한 검증은 사용하는 곳에서 하게 하면 어떨까요?
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.
0720733
이전에 수행했던 과제에서는 item.id가 undefined로 넘어갈 가능성이 있어서 이런 식으로 에러를 처리했었는데요.
이제는 undefined로 넘어갈 일이 없어서 해당 코드를 제거했습니다!
예외처리가 안되어있어서 showError로 예외처리도 진행했습니다:)
const timer = setTimeout(() => { | ||
setVisible(false); | ||
}, duration); | ||
|
||
return () => { | ||
clearTimeout(timer); | ||
}; |
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.
현재는 에러는 하나만 관리되고, 새 에러가 발생하면 기존 에러를 덮어쓰는 방식인데요. 큐를 이용해서 여러 에러 토스트를 보여줄 수 있게 개선하는 게 좋을까요?
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/constants/categoryOption.ts
Outdated
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.
사용하지 않는 파일이라 삭제했습니다!
return { | ||
data: data[key] as T | undefined, | ||
isLoading, | ||
error, |
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.
ErrorContext
에서 제공하는 에러와 여기서 제공하는 에러는 어떤 차이점이 있는걸까요?
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.
error가 발생할 경우, ErrorContext의 showError 함수에서 에러를 토스트로 보여줍니다!
src/hooks/useFetch.ts
Outdated
function useFetch<T>({ fetchFn, deps = [] }: UseFetchOptions<T>) { | ||
const [data, setData] = useState<T | null>(null); | ||
const [isLoading, setIsLoading] = useState(false); | ||
const [error, setError] = useState<Error | null>(null); |
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.
에러를 다루는 로직이 이곳저곳에서 보이는데 API 호출 시 에러가 발생하면 어떤 플로우를 통해 어떻게 처리되는지 설명을 해주실 수 있을까요?
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.
- API 함수 내부에서는 getCartItems, patchCartItem, deleteCartItem 등의 함수가 서버 요청을 보내고, 응답이 실패했을 경우 throw new Error(...)를 통해 명시적으로 에러를 발생시키는 구조입니다.
- GET 요청의 경우에는 useApiContext라는 커스텀 훅을 통해 API 호출을 감싸고 있으며, 에러가 발생하면 해당 훅이 error 상태를 외부로 노출시켜줍니다. 현재 이 error 값을 사용하고 있지 않아서, 명시적으로 처리해주도록 변경했습니다! (64da96c ,072073)
- PATCH나 DELETE 요청의 경우에는 대부분 try-catch 블록 안에서 실행되며, catch 블록 안에서 useErrorContext()의 showError(error)를 명시적으로 호출하고 있습니다.
2,3번 둘 다 이렇게 하면 에러가 context의 상태에 저장되고, 컴포넌트를 통해 사용자에게 에러 메시지가 표시되도록 처리되고 있습니다.
src/hooks/useFetch.ts
Outdated
const [isLoading, setIsLoading] = useState(false); | ||
const [error, setError] = useState<Error | null>(null); | ||
|
||
const controllerRef = useRef<AbortController | null>(null); |
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.
useRef
로 선언한 상태와 useState
로 선언한 상태는 어떤 차이가 있나요?
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.
이 훅은 사용하지 않아 제거했습니다.
useRef로 선언한 상태는 렌더링에 영향을 미치지 않습니다.
따라서 상태값은 유지하되, 리렌더링을 유발시키지 않고 싶을 때 사용하면 좋습니다!
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은 여기서 이만 merge 해도 좋을거 같네요! Image 컴포넌트에 소소한 코멘트 남겼으니 확인해주시면 좋을거 같습니다!
고생 많으셨습니다 ~~ 👍 👍 👍
const isTest = import.meta.env.MODE === 'test'; | ||
const basename = isTest ? '' : '/react-shopping-cart'; | ||
|
||
const router = createBrowserRouter( |
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/App.tsx
Outdated
<ErrorContextProvider> | ||
<ApiProvider> | ||
<div css={RoutesStyle}> | ||
<Router> |
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.
잘 찾아주셨군요 멋집니다 메타 👍 👍 👍
return ( | ||
<> | ||
{!isLoaded && <div css={skeletonCss} />} | ||
<img | ||
src={imgSrc} | ||
alt={alt} | ||
css={[!isLoaded && hiddenCss]} | ||
onLoad={() => setIsLoaded(true)} | ||
onError={() => { | ||
setImgSrc('./assets/default.png'); | ||
setIsLoaded(true); | ||
}} | ||
{...props} | ||
/> | ||
</> | ||
); |
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.
isLoaded
에 따른 분기가 명확하게 눈에 들어오지 않는데요. 아래처럼 작성하는 것도 생각해 볼 수 있을거 같네요
if (!isLoaded) return <div css={skeletonCss} />
return {
<img ... />
}
const timer = setTimeout(() => { | ||
setVisible(false); | ||
}, duration); | ||
|
||
return () => { | ||
clearTimeout(timer); | ||
}; |
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.
여유가 된다면 시도해보셔도 좋겠네요 ㅎㅎ 지금 당장 적용하실 필요는 없을거 같습니다
@@ -0,0 +1,58 @@ | |||
class HTTPClient { |
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.
깔끔하게 잘 구현해주셨네요 👍 👍 👍
📦 장바구니 미션
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
RTL 테스트 케이스를 모두 작성했나요?
배포한 데모 페이지에 정상적으로 접근할 수 있나요?
리뷰 요청 & 논의하고 싶은 내용
리뷰 요청 & 논의하고 싶은 내용
1) 상태 설계 의도
useCheckList
커스텀 훅Map<id, boolean>
형태로 관리isAllChecked
유틸 값 제공으로 전체 선택 상태를 외부에서 쉽게 확인 가능T
, 키 타입K
를 제네릭으로 받아 다양한 도메인에 재사용 가능getKey
함수 주입을 통해 외부에서 키 추출 방식 유연하게 정의'TOGGLE'
,'CHECK_ALL'
,'UNCHECK_ALL'
세 가지 명확한 액션 존재2) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
useCheckList
를 처음 설계할 때는 체크된 항목의 상태를Record<number, boolean>
형태로 관리했습니다.하지만 이 방식은 다음과 같은 한계가 있어, Map 자료구조로 변경했습니다.
Object.fromEntries()
를 계속해서 사용해야해서 직관적이지 못했습니다.이를 제네릭을 사용한
Map
자료구조로 대체하면서 불필요한 전처리 없이도 다양한 데이터에 적용 가능하도록 개선되었습니다.3) 이번 리뷰를 통해 논의하고 싶은 부분
첫 렌더링시 모든 체크 박스가 체크되어있다
이기에 현재Map
의 초기 상태가 모두true
로 설정되어 있습니다. 따라서 초기 상태를 기반으로 작성된 테스트는 도메인 로직이 변경될 경우 쉽게 깨질 수 있는다는 생각이 들었습니다.✅ 리뷰어 체크 포인트
1. 클라이언트 상태관리
2. MSW/Test