Skip to content

[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

Merged
merged 37 commits into from
Jun 4, 2025

Conversation

youdame
Copy link

@youdame youdame commented May 29, 2025

📦 장바구니 미션

이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.

1단계

  • 클라이언트 상태를 효과적으로 모델링하고 관리할 수 있다.
  • Jest, React Testing Library(RTL)를 활용하여 주요 기능에 대한 테스트를 작성할 수 있다.

🕵️ 셀프 리뷰(Self-Review)

제출 전 체크 리스트

  • 기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?

  • RTL 테스트 케이스를 모두 작성했나요?

  • 배포한 데모 페이지에 정상적으로 접근할 수 있나요?

리뷰 요청 & 논의하고 싶은 내용

리뷰 요청 & 논의하고 싶은 내용

1) 상태 설계 의도

  • useCheckList 커스텀 훅
    • 체크 가능한 항목들의 상태를 Map<id, boolean> 형태로 관리
    • 항목을 개별로 토글하거나 전체 선택/해제를 지원하는 기능 포함
    • isAllChecked 유틸 값 제공으로 전체 선택 상태를 외부에서 쉽게 확인 가능
    • 제네릭 기반의 유연한 인터페이스
      • 항목 타입 T, 키 타입 K를 제네릭으로 받아 다양한 도메인에 재사용 가능
      • getKey 함수 주입을 통해 외부에서 키 추출 방식 유연하게 정의
    • useReducer를 사용하여 액션 정의
      • 'TOGGLE', 'CHECK_ALL', 'UNCHECK_ALL' 세 가지 명확한 액션 존재
    • 그리고 각 dispatch 함수를 한 번 더 감싸서 함수를 내보내도록 했습니다.

2) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점

  • useCheckList를 처음 설계할 때는 체크된 항목의 상태를 Record<number, boolean> 형태로 관리했습니다.

    import { useReducer } from 'react';
    
    type CheckState = Record<number, boolean>;
    
    type Action =
      | { type: 'TOGGLE'; id: number }
      | { type: 'CHECK_ALL' }
      | { type: 'UNCHECK_ALL' };
    
    function reducer(state: CheckState, action: Action): CheckState {
      switch (action.type) {
        case 'TOGGLE':
          return { ...state, [action.id]: !state[action.id] };
        case 'CHECK_ALL':
          return Object.fromEntries(Object.keys(state).map((id) => [id, true]));
        case 'UNCHECK_ALL':
          return Object.fromEntries(Object.keys(state).map((id) => [id, false]));
        default:
          return state;
      }
    }
    
    export function useCheckList(ids: number[]) {
      const initialState: CheckState = Object.fromEntries(ids.map((id) => [id, true]));
    
      const [state, dispatch] = useReducer(reducer, initialState);
    
      const toggle = (id: number) => dispatch({ type: 'TOGGLE', id });
      const checkAll = () => dispatch({ type: 'CHECK_ALL' });
      const uncheckAll = () => dispatch({ type: 'UNCHECK_ALL' });
    
      const isAllChecked = Object.values(state).every(Boolean);
    
      return {
        state,
        toggle,
        checkAll,
        uncheckAll,
        isAllChecked
      };
    }

    하지만 이 방식은 다음과 같은 한계가 있어, Map 자료구조로 변경했습니다.

    • Object.fromEntries()를 계속해서 사용해야해서 직관적이지 못했습니다.
    • 초기 파라미터를 number[] 형태에 맞추기 위해 실제 사용처에서 매번 그 구조에 맞게 가공해줘야 하는 번거로움이 생겼습니다.

이를 제네릭을 사용한 Map 자료구조로 대체하면서 불필요한 전처리 없이도 다양한 데이터에 적용 가능하도록 개선되었습니다.

3) 이번 리뷰를 통해 논의하고 싶은 부분

  • 요구사항이 첫 렌더링시 모든 체크 박스가 체크되어있다 이기에 현재 Map의 초기 상태가 모두 true로 설정되어 있습니다. 따라서 초기 상태를 기반으로 작성된 테스트는 도메인 로직이 변경될 경우 쉽게 깨질 수 있는다는 생각이 들었습니다.
  • mocking 데이터나 초기 체크 상태에 대한 요구사항이 변경되면 테스트도 수정해야 하는 구조인데, 이게 적절한 테스트 방식인지 고민이 됩니다.
  • 즉, 테스트는 어떤 기준값에 의존해야 할까요?
    • "기능의 목적"을 기준으로 테스트해야 할지, 아니면 "mocking된 초기 상태"에 의존해도 되는 건지 리뷰어의 의견이 궁금합니다.
    • 그런데 의존하지 않게 테스트 코드를 짜는 법도 잘 모르겠습니다…

✅ 리뷰어 체크 포인트

1. 클라이언트 상태관리

  • 원본상태/파생상태를 적절히 구분하여 선언하였나요?
  • React state 를 불필요하게 선언한 부분은 없나요?
  • 상태 관리 로직의 책임이 적절히 응집/분리되었나요? (ex. reducer, hook, Context)

2. MSW/Test

  • 주요 기능을 적절히 정의하였나요?
  • 주요 기능/예외에 대한 테스트가 충분히 이루어졌나요?

woowa-euijinkk and others added 27 commits May 27, 2025 14:21
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Co-authored-by: JinseoKim <[email protected]>
Copy link

coderabbitai bot commented May 29, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@yujo11 yujo11 left a 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>
Copy link

Choose a reason for hiding this comment

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

최신 버전의 react-rotuer에서는 createBrowserRouter 사용이 권장되는데요. 어떤 부분이 어떻게 다른지 찾아보고 적용해보셔도 좋을거 같아요

Copy link
Author

@youdame youdame Jun 3, 2025

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 등) 를 활용할 수 있는 방식으로, 더 구조적인 라우팅 구성이 가능합니다.

  1. 데이터 패칭과 라우팅의 통합 (loader/action)
  2. 정적 라우트 선언을 통한 테스트 용이성
  3. SEO 및 서버 사이드 렌더링(SSR)에 더 유리한 구조

등의 장점 때문에 createBrowserRouter 기반의 사용이 현 시점에서 공식적으로도 권장되고 있는 상황이네요.

이번 프로젝트에서는 아직 loader나 action은 쓰지 않고 있지만,
장기적으로 확장성을 고려했을 때 createBrowserRouter 방식으로 리팩토링해두는 게 더 나은 선택이라 판단되어 적용해봤습니다 :)

Copy link

Choose a reason for hiding this comment

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

잘 찾아주셨군요 멋집니다 메타 👍 👍 👍

src/App.tsx Outdated
Comment on lines 15 to 16
<Route path="/" element={<CartPage />} />
<Route path="/order" element={<OrderPage />} />
Copy link

Choose a reason for hiding this comment

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

path들은 상수화 해서 사용해주셔도 좋을거 같네요

Copy link
Author

Choose a reason for hiding this comment

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

b0d9653

상수화했습니다!

Comment on lines 9 to 12
headers: {
Authorization: `Basic ${btoa(`${import.meta.env.VITE_USER_ID}:${import.meta.env.VITE_PASSWORD}`)}`,
'Content-Type': 'application/json'
}
Copy link

Choose a reason for hiding this comment

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

api를 요청하는 부분에서 반복적으로 사용되는 부분들이 눈에 들어오는데요. 추상화를 통해 중복되는 코드를 줄여보셔도 좋을거 ㄱ타아요

Copy link
Author

Choose a reason for hiding this comment

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

9a1be91
여기서 추상화를 시도해봤습니다!

Comment on lines 4 to 6
if (cartItemId === undefined) {
throw new Error('cartItemId가 정의되지 않았습니다.');
}
Copy link

Choose a reason for hiding this comment

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

이 부분은 parameter에 대한 부분 (클라이언트의 문제)이고

Comment on lines +15 to +17
if (!res.ok) {
throw new Error('장바구니에서 상품을 삭제하는 데 실패했습니다.');
}
Copy link

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에 대한 검증은 사용하는 곳에서 하게 하면 어떨까요?

Copy link
Author

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로 예외처리도 진행했습니다:)

Comment on lines +8 to +14
const timer = setTimeout(() => {
setVisible(false);
}, duration);

return () => {
clearTimeout(timer);
};
Copy link

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.

현재는 에러는 하나만 관리되고, 새 에러가 발생하면 기존 에러를 덮어쓰는 방식인데요. 큐를 이용해서 여러 에러 토스트를 보여줄 수 있게 개선하는 게 좋을까요?

Copy link

Choose a reason for hiding this comment

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

여유가 된다면 시도해보셔도 좋겠네요 ㅎㅎ 지금 당장 적용하실 필요는 없을거 같습니다

Copy link

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.

사용하지 않는 파일이라 삭제했습니다!

return {
data: data[key] as T | undefined,
isLoading,
error,
Copy link

Choose a reason for hiding this comment

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

ErrorContext에서 제공하는 에러와 여기서 제공하는 에러는 어떤 차이점이 있는걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

error가 발생할 경우, ErrorContext의 showError 함수에서 에러를 토스트로 보여줍니다!

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);
Copy link

Choose a reason for hiding this comment

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

에러를 다루는 로직이 이곳저곳에서 보이는데 API 호출 시 에러가 발생하면 어떤 플로우를 통해 어떻게 처리되는지 설명을 해주실 수 있을까요?

Copy link
Author

@youdame youdame Jun 4, 2025

Choose a reason for hiding this comment

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

  1. API 함수 내부에서는 getCartItems, patchCartItem, deleteCartItem 등의 함수가 서버 요청을 보내고, 응답이 실패했을 경우 throw new Error(...)를 통해 명시적으로 에러를 발생시키는 구조입니다.
  2. GET 요청의 경우에는 useApiContext라는 커스텀 훅을 통해 API 호출을 감싸고 있으며, 에러가 발생하면 해당 훅이 error 상태를 외부로 노출시켜줍니다. 현재 이 error 값을 사용하고 있지 않아서, 명시적으로 처리해주도록 변경했습니다! (64da96c ,072073)
  3. PATCH나 DELETE 요청의 경우에는 대부분 try-catch 블록 안에서 실행되며, catch 블록 안에서 useErrorContext()의 showError(error)를 명시적으로 호출하고 있습니다.
    2,3번 둘 다 이렇게 하면 에러가 context의 상태에 저장되고, 컴포넌트를 통해 사용자에게 에러 메시지가 표시되도록 처리되고 있습니다.

const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<Error | null>(null);

const controllerRef = useRef<AbortController | null>(null);
Copy link

Choose a reason for hiding this comment

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

useRef로 선언한 상태와 useState로 선언한 상태는 어떤 차이가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

이 훅은 사용하지 않아 제거했습니다.
useRef로 선언한 상태는 렌더링에 영향을 미치지 않습니다.
따라서 상태값은 유지하되, 리렌더링을 유발시키지 않고 싶을 때 사용하면 좋습니다!

Copy link

@yujo11 yujo11 left a 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(
Copy link

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>
Copy link

Choose a reason for hiding this comment

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

잘 찾아주셨군요 멋집니다 메타 👍 👍 👍

Comment on lines +8 to +23
return (
<>
{!isLoaded && <div css={skeletonCss} />}
<img
src={imgSrc}
alt={alt}
css={[!isLoaded && hiddenCss]}
onLoad={() => setIsLoaded(true)}
onError={() => {
setImgSrc('./assets/default.png');
setIsLoaded(true);
}}
{...props}
/>
</>
);
Copy link

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 ... />
}

Comment on lines +8 to +14
const timer = setTimeout(() => {
setVisible(false);
}, duration);

return () => {
clearTimeout(timer);
};
Copy link

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 {
Copy link

Choose a reason for hiding this comment

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

깔끔하게 잘 구현해주셨네요 👍 👍 👍

@yujo11 yujo11 merged commit f6fa740 into woowacourse:youdame Jun 4, 2025
1 check passed
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