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

[정성현] sprint12 #138

Conversation

jsh1147
Copy link
Collaborator

@jsh1147 jsh1147 commented Dec 7, 2024

배포사이트 바로가기

제출 기한 안에 목표만큼 완성하지 못했으나,

곧 미션이 종료되어 코드 리뷰 받을 기회가 없기도 해서

아쉬운 것 같아 늦게라도 제출드렸습니다.

요구사항

기본

중고마켓

  • '상품 등록하기' 버튼을 누르면 "/additem" 로 이동합니다.
  • 각 상품 클릭 시 상품 상세 페이지로 이동합니다.
  • 상품 상세 페이지 주소는 "/items/{productId}" 입니다.

상품 상세

  • 내가 등록한 상품일 경우 상품 수정, 삭제가 가능합니다.
  • 문의하기 input창에 값을 입력 후 '등록' 버튼을 누르면 댓글이 등록됩니다.
  • 내가 등록한 댓글은 수정, 삭제가 가능합니다.

상품 등록

  • 이미지를 제외하고 input 에 모든 값을 입력하면 '등록' 버튼이 활성화 됩니다.
  • 활성화된 '등록' 버튼을 누르면 상품 등록이 완료됩니다.
  • 등록이 완료되면 해당 상품 상세 페이지로 이동합니다.

심화

  • api 요청에 TanStack React Query를 활용해 주세요.

주요 변경사항

  • API 리팩토링
    • Axios 적용
    • 타입 & 상수 분리
  • 로그인 상태 전역상태화
    • React Context & State를 활용하여 로그인 상태 관리
    • 회원가입/로그인 페이지는 못 건들었기 때문에, 로컬 스토리지에 토큰을 직접 넣어야 함
  • _app.tsx 리펙토링
    • provider들과 layout을 각각 Component로 정리
  • 중고마켓 - 베스트 상품 구현
    • React Query를 활용하여 구현
    • media가 결정되고, params가 결정된 뒤에 query가 동작하도록 구현함

스크린샷

  • 베스트 상품(PC, 로그인X)
    image
  • 베스트 상품(PC, 로그인O)
    image
  • 베스트 상품(태블릿)
    image

멘토에게

  • axios를 적용하면서 타입은 types/api.ts, 경로는 constants/api.ts로 옮겼는데
    적절한 방식으로 분리한 걸까요?
    전에는 apis/파일명.type.ts 로 했는데,
    전처럼 apis 안에 모여있기 vs 기능대로 분리돼있기 중 어떤 기준으로 결정해야 할지 확신이 안섭니다.
  • 값이 초기 렌더링 이후 or 비동기로 정해질 때,
    초기값을 주기보단 미결정 상태(undefined, isLoading)로 두도록 코드를 짜고 있습니다.
    이런 방향을 고수하는 게 나중에 문제 없을까요? 아니면 초기값 방식을 좀 더 시도해보는게 나을까요?

- 페이지명/index.tsx 방식으로 변경
- Axios, Tanstack Query, React Hook Form 설치
- Fetch API에서 Axios 라이브러리로 전환
- API 상수와 타입을 분리
- 컨텍스트를 활용하여 로그인 상태 전역상태화
- products API 추가
- 탠스택 쿼리의 useQuery 활용
@jsh1147 jsh1147 self-assigned this Dec 7, 2024
@jsh1147 jsh1147 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Dec 7, 2024
@jsh1147 jsh1147 requested a review from GANGYIKIM December 7, 2024 20:17
Copy link
Collaborator

@GANGYIKIM GANGYIKIM left a comment

Choose a reason for hiding this comment

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

성현님 이번주 스프린트 미션 고생하셨습니다~
주차 요구 사항을 다 구현하지 않으셔도 편하게 제출하셔도 됩니다.
다음주면 스프린트 미션도 마무리되고, part4에서는 다른 멘토분과 함께하게 되시니 이번이 마지막 리뷰네요.
마지막 스프린트 미션도 화이팅하세요 😁

기존 코드에 react-query라는 라이브러리를 새로 도입하시느라, 고민이 많으셨을거라 생각됩니다!
react-query의 경우 문서에 TkDodo라는 라이브러리 메인테이너 블로그가 링크되어 있는데
어떻게 해당 라이브러리를 사용해야 하는지 잘 설명 되어 있으니 한번 읽어보시면 많이 도움이 될거라고 생각합니다.


axios를 적용하면서 타입은 types/api.ts, 경로는 constants/api.ts로 옮겼는데 적절한 방식으로 분리한 걸까요? 전에는 apis/파일명.type.ts 로 했는데, 전처럼 apis 안에 모여있기 vs 기능대로 분리돼있기 중 어떤 기준으로 결정해야 할지 확신이 안섭니다.
=>
위와 같은 경우 지금처럼 분리하신게 더 좋다고 생각합니다. 데이터 타입 정의와 Routing Constants가 한 파일에 있는 것보다 다른 역할을 하니 분리하는게 관심사 분리의 원칙에 적합합니다.

값이 초기 렌더링 이후 or 비동기로 정해질 때, 초기값을 주기보단 미결정 상태(undefined, isLoading)로 두도록 코드를 짜고 있습니다. 이런 방향을 고수하는 게 나중에 문제 없을까요? 아니면 초기값 방식을 좀 더 시도해보는게 나을까요?
=>
우선 이러한 고민을 하시는 이유가 무엇인지 궁금합니다~
제 생각에 두 방식은 UX 측면에서 차이가 있을 것 같아 현재 환경에서 이에 집중해 답변을 드리겠습니다.
만약 다른 측면 때문에 고민하시는거라면 더 자세히 설명해주시면 좋을 것 같습니다.

react-query의 경우 데이터에 대한 cache를 이용하기에 데이터를 한번 불러온 후 다시 접근시, 더 빠르게 데이터에 접근할 수 있습니다. 결국 유저가 데이터가 없을 때 적절한 UI-loading spinner, placeholder component 등-를 보고, 후에 다시 접근시 바로 cache된 데이터를 통해 최종 UI에 접근하게 됩니다. 만약 react-query의 placeholderData를 통해 기본값을 주신다고 해도 위와 동일하게 동작합니다. (처음에 placeholderData 보여짐, 후에 data fetching, 해당 결과 cache, 재접근시 cache읽어오게 되므로 빠르게 UI 그려짐)

만약 초기에 특정 placeholder 요소를 보여줄시 이를 위한 데이터가 필요하거나, 다른 데이터에서 현 페이지에 필요한 데이터를 가지고 있거나, DX 측면에서 이를 선호한다면, 초기값 설정을 고려해볼 것 같습니다.

그러나 초기값없이도 UX에 문제가 없고, 요구사항을 구현하는데 무리가 없다면 기존 방식을 사용할 것 같습니다.

Comment on lines +3 to +9
export const PATH = {
ME: "users/me",
PRODUCTS: "products",
SIGNUP: "auth/signUp",
LOGIN: "auth/signIn",
REFRESH: "auth/refresh-token",
} as const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
두개를 사용처에서 조합해서 사용하는 것 보다 아래처럼 하시는게 더 좋지 않을까싶네요.
분리해야할 이유를 모르겠어서요~

Suggested change
export const PATH = {
ME: "users/me",
PRODUCTS: "products",
SIGNUP: "auth/signUp",
LOGIN: "auth/signIn",
REFRESH: "auth/refresh-token",
} as const;
export const PATH = {
ME: `${BASE_URL}/users/me`,
PRODUCTS: `${BASE_URL}/products`,
SIGNUP: `${BASE_URL}/auth/signUp`,
LOGIN: `${BASE_URL}/auth/signIn`,
REFRESH: `${BASE_URL}/auth/refresh-token`,
} as const;

@@ -13,26 +16,38 @@ export const pretendard = localFont({
weight: "45 920",
});

const queryClient = new QueryClient();
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
react-query에서 stale이라는 값이 있습니다.
이는 해당 데이터가 오래된 데이터인지 판단하는 값으로, 해당 시간이 지나면 해당 데이터는
stale 되었다고 판단하고 새로 불러오게 됩니다.
react-query에서 staleTime의 기본값은 0이므로, 매번 새롭게 데이터를 불러오게 됩니다.
최신 상태가 필요한 경우가 아니라면 이는 불필요하게 많은 데이터 패칭의 원인이 됩니다.
각 쿼리별로 staleTime을 설정해줄 수 있으니, 기본적으로는 20초를 권장한다고 TK 블로그에서 언급합니다.
아래 글을 참고해보세요~

Suggested change
const queryClient = new QueryClient();
const queryClient = new QueryClient({
defaultOptions: {
queries: {
// ✅ globally default to 20 seconds
staleTime: 1000 * 20,
},
},
});

https://tkdodo.eu/blog/react-query-as-a-state-manager#customize-staletime

Comment on lines +8 to +10
<Head>
<title>중고마켓 - 판다마켓</title>
</Head>
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
👍

Comment on lines +3 to +4
import heartIcon from "#/icons/heart_inactive.svg";
import styles from "./Item.module.css";
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
svg, module.css에 해당하는 타입정의가 없다고 에러가 나오네요~
아마 type declation 파일을 로컬에만 가지고 계신게 아닌가 생각이 듭니다!
한번 확인해보세요~
스크린샷 2024-12-09 오후 4 44 21

Comment on lines +6 to +15
interface ItemProps {
data: {
id: number;
name: string;
description: string;
price: number;
images: string[];
favoriteCount: number;
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
해당 데이터가 아마 product 응답값의 부분일 것 같은데 이렇게 따로 정의해주시기보다 기존 타입을 활용하시는 것이 더 좋습니다.
이렇게 새로 정의를 해주시면 어떤 값의 부분인지 알 수 없고, 응답값의 구조가 변경될 시 다 따로 변경해주어야 합니다.

// 1. 기존 productRes type 에서 product관련 타입 분리
export interface ProductProps { ... }
export interface GetProductsRes {
  totalCount: number;
  list: ProductProps[]
}

// 2. ProductProps 활용
type ItemProps = Omit<ProductProps, 'createAt' | 'ownerNickname' | 'ownerId' | 'tags'>

export default function Item({ id, name, description, price, images, favoriteCount }: ItemProps) {}

}: PostSignUpParams) => {
const bodyObj = { email, nickname, password, passwordConfirmation };

const response = await instance.post<PostSignUpRes>(PATH.SIGNUP, bodyObj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
타입 정의 좋습니당 👍

src={profileImage}
alt="프로필 사진"
/>
{isLoading ? undefined : user ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
이중 삼항연산자는 가독성이 떨어지니 조건을 더 단순화하거나 분리하시는 것이 좋을 것 같습니다.
만약 isLoading일 시 undefined가 반환되야 한다면, 즉 아무것도 렌더링될 필요가 없다면 아래 조건도 가능할 것 같습니다.

Suggested change
{isLoading ? undefined : user ? (
{!isLoading && user ? (

import Item from "./Item";
import styles from "./BestItemsSection.module.css";

const pageSizeTable = { PC: 4, TABLET: 2, MOBILE: 1 };
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
as const를 통해 더 정확한 타입을 추론해보세요~

Suggested change
const pageSizeTable = { PC: 4, TABLET: 2, MOBILE: 1 };
const pageSizeTable = { PC: 4, TABLET: 2, MOBILE: 1 } as const;

@GANGYIKIM GANGYIKIM merged commit b84553b into codeit-bootcamp-frontend:Next-정성현 Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants