Skip to content

Review용 PR - #1

Open
Yeonseo-Jo wants to merge 53 commits into
emptyfrom
review
Open

Review용 PR#1
Yeonseo-Jo wants to merge 53 commits into
emptyfrom
review

Conversation

@Yeonseo-Jo

@Yeonseo-Jo Yeonseo-Jo commented Apr 11, 2024

Copy link
Copy Markdown
Owner

🔗 배포 링크

📌tanstack-react-query 커스텀 훅들

(1) 유저 프로필 정보, 팔로워&팔로잉 정보 get : hooks > user > useGetCombinedUserInfo.ts

  • 한 페이지에서 두 useQuery(프로필 정보 get, 팔로워, 팔로잉 정보 get)를 병렬적으로 실행할수 있도록 useQuries로 구현
  • combine 문법으로 data, loading, error 처리 한 번에 return

(2) mutation 후 invalidate 요청 처리 하는 커스텀 훅 : hooks > common > useGetMutate.ts

  • queryKey, mutationFn, invalidateOption을 받아서 mutate 후, 성공 (onSuccess) 시 해당 queryKey의 데이터를 invalidate 요청하는 로직을 하나의 커스텀 훅으로 만들어서 사용

(2) 팔로우 하기 (useUpdateFollow) : hooks > follow > useUpdateFollow.ts

  • useGetMutate를 사용해서, putFollower 데이터 패칭 함수 성공하면 'userFollowInfo'의 queryKey를 가지는 데이터를 재요청할수 있도록 하는 mutate를 return함
  • 즉, put (update) 통신 후 값 재검증
  • 버튼 클릭시 mutate 메서드 인자에 새로운 데이터 주입하도록 onClick handler에서 사용

(3) 언팔로우하기 (useUpdateUnfollow) : hooks > follow > useUpdateUnollow.ts

  • useGetMutate를 사용해서, deleteFollower 데이터 패칭 함수 성공하면 'userFollowInfo'의 queryKey를 가지는 데이터를 재요청할수 있도록 하는 mutate를 return함
  • 즉, delete 통신 후 값 재검증
  • 버튼 클릭시 mutate 메서드 인자에 새로운 데이터 주입하도록 onClick handler에서 사용


📌트러블 슈팅

1. vanilla extract 관련 패키지를 yarn 패키지 매니저로 설치 시 invalid URL 에러 발생

  • 해결 : node v.18.18.0 -> node v.21.7.2(latest)으로 최신 버전 업데이트로 해결
  • 🔗참고 자료

2. vanilla extract 관련 설정을 위해 vanliila extract 공식 문서에 나와 있듯이 next.config.mjs 수정 -> require, module에서 문법 에러

  • 해결 : es6 문법에 맞게 수정 (require -> import // module -> export default 등)

3. broswer caching으로 인해 invalidate한 데이터가 최신 데이터를 받아오지 못하는 이슈 해결을 위해, header에 browser caching 관련 설정 넣어줌 -> cors 에러

  • 해결 : headers: { 'If-None-Match': '' }
    위와 같이 헤더를 수정하여 해결
  • 🔗참고 자료

4. next.js에서 sessionStorage 접근 시 sessionStorage is not defined 에러 발생

  • 원인 : next.js에서는 csr 이전 ssr 렌더링 수행하는데, sessionStorage가 있는 window 객체는 client-side에서만 사용 가능 (ssr 단계에서는 window, document와 같은 client side 전역 객체 접근 및 사용 불가능) -> 그래서 client side가 로드 될때까지 sessionStorage 접근 불가능
  • 해결 : typeof window !== 'undefined' 구문 추가로 client side 로드 시에 접근할 수 있도록 구문 추가

@Yeonseo-Jo Yeonseo-Jo self-assigned this Apr 11, 2024
@vercel

vercel Bot commented Apr 11, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
github-follow-detector-refactor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 7:32am

@Arooming Arooming left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

허업 너무너무 꼼꼼하게 신경써서 해준 티가 많이 나네요..!
덕분에 리뷰하면서 많이 배워가기도 하고 제 프젝에도 반영해야겠다 느낀 부분이 많습니다!! 수고 많았서용 💖💖💖

ps. css 네이밍은 vanilla-extract 공식문서에도 나와있는 만큼, camelCase로 정의해주면 더 좋을 것 같습니당 !!

Comment thread app/(home)/page.tsx

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P5: 혹시 여기 라우팅 주소를 home이 아니라 (home)으로 하신 이유가 뭔지 물어보려고 했는데,
찾아보니 괄호로 감싸주면 url 주소에 영향을 미치지 않고 라우트를 구성할 수 있다고 하네요..!
덕분에 새로운 내용 알아갑니당 ! 👍👍

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

마자요! 토큰을 입력하는 뷰의 url은 /로 유지하되, follow-list 페이지와 동일한 폴더 위계를 만들어주고 싶어서 url 주소에 영향을 미치지 않는 route group으로 묶어줬습니당~

Comment on lines +15 to +16
queryKey: QUERY_KEYS.user.followInfo,
queryFn: () => getUserFollowInfo(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P5: 이런 식으로 정의하니까 훨씬 깔끔하고 좋네요..! 저도 이렇게 리팩토링해보겠습니다 . ..

Comment thread next.config.mjs Outdated
/** @type {import('next').NextConfig} */
const nextConfig = {
images: {
domains: ["avatars.githubusercontent.com"],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ASK: 이거 혹시 터미널에 imgaes/domains는 deprecated되었으니 remotePatterns를 활용해라..! 라는 워닝이 뜨지 않나요!?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

오 대박 뜨네용 !! 루밍이꺼 보고 수정 해보겠습니당!

Comment thread app/follow-list/page.tsx Outdated
Comment on lines +13 to +20
const token: string | null =
typeof window !== "undefined"
? getSessionStorageHandler().getItem("token")
: null;
const isHasToken =
typeof window !== "undefined"
? getSessionStorageHandler().hasItem("token")
: false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ASK: 혹시 null과 false를 반환하는 형태로 구현하신 이유가 따로 있을까요!?
조건부 처리를 할때 빈 값을 반환하는게 좋은 패턴은 아니라고 들어서요..!
특별한 이유가 없다면 아래처럼 수정해봐도 좋을 것 같습니당 😺😺

Suggested change
const token: string | null =
typeof window !== "undefined"
? getSessionStorageHandler().getItem("token")
: null;
const isHasToken =
typeof window !== "undefined"
? getSessionStorageHandler().hasItem("token")
: false;
const token: string = typeof window !== "undefined"
&& getSessionStorageHandler().getItem("token");
const isHasToken = typeof window !== "undefined"
&& getSessionStorageHandler().hasItem("token");

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

좋은 패턴 제안 감사합니다!

token 값은 값이 있거나(string) 없을 때는 명시적으로 null 값이 되도록 타입을 지정해주었는데요, 삼항연산자로 window가 undefined 일 때 null 임을 정의해주지 않으면 (&&연산자 사용) 값이 false(boolean type)으로 인식되어 타입 에러가 나더라구요!

반면 isHasToken은 boolean 값의 type을 갖는 값이기 때문에 제안 주신것처럼 삼항 연산자가 아닌 && 연산자로 구현해도 문제가 없었습니다!

따라서 token은 원래 로직대로 삼항 연산자로 winodw값이 잡히지 않을시 null 값을 명시해주고,
isHasToken은 && 연산자로 window 값이 잡히지 않을 시 false의 boolean 타입이 추론되도록 로직 수정했습니다 ~

Comment on lines +27 to +30
return [
{ type: LIST_TYPE.unMatched, list: unMatchedList },
{ type: LIST_TYPE.matched, list: matchedList },
];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

!!! 너무 깔끔하고 좋네요 👍👍

return deleteFollower(username);
};

return useGetMutate(queryKey, mutationFn, { exact: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

오 이렇게 쓰니까 중복코드도 줄어들고 가독성도 훨씬 좋네요! 배워갑니당 . . .. 👍👍

Comment thread components/home/TokenInputContainer.tsx Outdated
const handleClickConfirmFollowBtn = (
e: React.MouseEvent<HTMLButtonElement>
) => {
if (!token) e.preventDefault();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: 토큰이 없다면 클릭이벤트를 핸들링하는 함수가 동작하지 않도록 하기 위해 이 부분을 추가해주신 부분이 맞을까요?
그렇다면 아래 버튼 태그의 disabled 속성을 이용해보는 건 어떨까요!?
버튼 비활성화를 위해 만들어진 속성이니 이를 활용하는 것도 좋을 것 같습니다!

<button disabled={!token}> ... </button>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

disabled 속성 너무 좋네요! 제가 의도했던 동작을 더 잘 구현해주는 방식이라 바로 반영했습니다✨

Comment thread apis/index.ts

// token 입력 후 헤더에 저장하는 함수
export const setInstanceToken = (token: string) => {
instance.defaults.headers.common["Authorization"] = `Bearer ${token}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P5: 반드시 토큰을 넣어줘야 하니까 요청이 가기 전에 이걸 뺏어와서 토큰 값을 넣어줘야겠다! 라고 생각해서, interceptor를 활용해서 구현했는데요..!
다시 axios랑 AxiosInstance에 대해 찬찬히 찾아보니, 어차피 기본적으로 토큰이 있어야 동작하는 부분이라서 interceptor와 같은 예외적인 설정을 해줄 필요없이 기본값(defaults)으로 넣어주는게 더 맞는 것 같다는 생각이 들었습니당. . !

명확한 기준이 있다기 보단 그때 그때 상황을 잘 판단하고 잘 선택하는게 중요한 것 같다는 걸 다시 한 번 느끼고 갑니다 ..~


color: "#C9D1D9",

borderRadius: "10px",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P5: 개인적인 취향일 수 있지만 여기도 단위를 통일해주면 좋을 것 같습니다 !

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

border과 같은 속성은 반응형 구현이 크게 유의미하지 않다고 생각하여 px 단위를,
나머지 속성들은 rem 단위를 사용해주었습니다!

display: "flex",
justifyContent: "center",

height: "75%",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ASK: 요런 식으로 구체적인 % 값으로 높이나 너비 값을 준 부분이 꽤 있는 것 같은데요..!
% 단위는 컨테이닝 블록의 영향을 많이 받기 때문에 특히 마진이나 패딩으로의 사용은 지양하는 걸로 알고 있는데,
혹시 % 단위로 구현한 이유가 따로 있을까용?! (마진, 패딩 값을 %로 준 건 아니지만 !!)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

followList의 유저 리스트 부분이 내부 스크롤이 될수 있도록 구현하기 위해 고정된 높이 값이 필요했는데요,
디자인이 없는 상황에서 프로필 영역과 유저 리스트 영역의 고정된 높이 지정을 용이하게 하기 위해 %를 사용했습니다!
따라서 높이에 %를 사용한 부분은 모두 요 높이 영역 지정을 위한 부분들입니다!

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