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

코드리뷰 용 PR #2

Open
wants to merge 56 commits into
base: empty
Choose a base branch
from
Open

코드리뷰 용 PR #2

wants to merge 56 commits into from

Conversation

Arooming
Copy link
Owner

@Arooming Arooming commented Apr 13, 2024

📌 react-query 커스텀 훅

(1) libs > hook > useGetUserInfo()

  • 토큰을 입력한 사용자의 정보를 불러오는 커스텀 훅
  • 에러 발생 시, 세션 스토리지에 저장된 토큰 값을 버리고 홈으로 라우팅

(2) libs > hook > useGetFollowInfo()

  • 사용자의 팔로우 목록을 불러오는 커스텀 훅
  • 불러온 팔로우 목록을 가공하여 맞팔로우, 언팔로우 리스트 생성

(3) libs > hook > usePutFollower()

  • 선택한 사용자를 팔로우 목록에 추가하는 커스텀 훅
  • putFollower 함수를 호출 → 데이터 패칭 성공 시, query-key에 해당하는 함수에 저장된 데이터를 날리고 새로운 데이터 refetch
  • 즉, putFollower()에 데이터가 성공적으로 패칭된 경우, 팔로워를 불러오는 함수에 저장된 팔로우 목록을 지우고, 새로운 팔로우 목록을 불러오도록 함.

(4) libs > hook > useDeleteFollower()

  • 선택한 사용자를 팔로우 목록에서 삭제하는 커스텀 훅
  • usePutFollower()와 같은 방식으로 구현함.
    • deleteFollower 함수를 호출 → 데이터 패칭 성공 시, query-key에 해당하는 함수에 저장된 데이터를 날리고 새로운 데이터 refetch
    • 즉, deleteFollower()에 데이터가 성공적으로 패칭된 경우, 팔로워를 불러오는 함수에 저장된 팔로우 목록을 지우고, 새로운 팔로우 목록을 불러오도록 함.

📌 트러블 슈팅


(1) full reload warning

  • 새로고침 시, full reload 워닝 발생
    • 편집 중인 파일 외의 다른 컴포넌트가 export 되고 있는 경우
    • 컴포넌트의 이름이 PascalCase가 아닌 camelCase인 경우
  • 위와 같은 경우 해당 워닝이 발생할 수 있음을 파악
    • camelCase로 정의했던 컴포넌트 이름을 PascalCase로 수정하여 이슈 해결
  • 참고: 🔗 https://nextjs.org/docs/messages/fast-refresh-reload

(2) The "images.domains" configuration is deprecated. Please use "images.remotePatterns" configuration instead

(3) browser caching 으로 인해 최신 데이터를 받아오지 못하는 이슈

  • 캐싱 비활성화를 위해 Cache-Control 설정 → CORS 에러 발생
    • If-None-Match를 활용하여 에러 해결

      headers: {
            "If-None-Match": "",
            ...
      }

      If-None-Match에 빈 값을 할당하여 서버에 캐싱된 값을 보내지 않도록 함.**

@Arooming Arooming self-assigned this Apr 13, 2024
Copy link

@Yeonseo-Jo Yeonseo-Jo left a comment

Choose a reason for hiding this comment

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

짧은 기간에 한건데 너무너뮤너무 디테일하게 잘 구현했네용😎!!!
저번 구현사항에 대한 피드백도 잘 담겨 있어서 더 좋은것 가타요~
고생 많아써 ~~

import Header from "./Header";
const inter = Inter({ subsets: ["latin"] });

const queryClient = new QueryClient();

Choose a reason for hiding this comment

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

nextjs에서 root layout 계층에 선언하는 queryClient는, 성능적인 측면과 참조 동일성 유지(라이프사이클에서 한 번만 초기화 될 수 있도록)를 위해 useState를 사용해 설정해주는게 좋다고 합니다!

참고한 자료 링크도 함께 남겨요~

Copy link
Owner Author

Choose a reason for hiding this comment

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

오!! 참고해볼게요 감사합니다 !

Comment on lines +21 to +23
<CommonLayout>
<body className={inter.className}>{children}</body>
</CommonLayout>

Choose a reason for hiding this comment

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

CommonLayout이 body 태그로 이루어져 있는데, 하위에 body 태그가 또 있어서 중복 코드 같습니다!
폰트 provide 해주는 body 태그까지 한번에 CommonLayout에 넣어주고 여기 body 태그는 삭제하면 더 좋을것 같습니당💪🏻

Suggested change
<CommonLayout>
<body className={inter.className}>{children}</body>
</CommonLayout>
<CommonLayout>
{children}
</CommonLayout>

};

return (
<main className={styles.HomeWrapper}>

Choose a reason for hiding this comment

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

요기는 vanilla extract의 css 정의 네이밍이 PascalCase로 되어 있네유
수정해주면 좋을것 같습니다!

Comment on lines +11 to +12
const { isLoading: userInfoLoading, data: userInfoData } = useGetUserInfo();
const { isLoading: followInfoLoading, data: followInfoData } =

Choose a reason for hiding this comment

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

저는 여기서 두 useQuery문을 병렬적으로 실행하기 위해 useQueires문을 사용했는데요,
데이터 return 되는 타입이 복잡해진다는 단점이 있지만 loading, data 값을 한 번에 관리할 수 있고(tanstack query v5의 combine 문법 사용) 지금처럼 한 뷰에 보여져야 하는 데이터들을 병렬적으로 호출할 수 있다는 장점이 있는것 같습니다!

useQuries 공식 문서 링크 보고 한 번 비교해봐도 좋을것 가타요~

Copy link
Owner Author

@Arooming Arooming Apr 16, 2024

Choose a reason for hiding this comment

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

안 그래도 연서님 코드 리뷰하면서 useQueries에 대해 더 깊게 알아보고 또 직접 코드에 적용해봐야겠다고 생각했습니다!

useQuery를 사용하면서 쿼리 수가 증가함에 따라 loading과 data 값의 수 역시 함께 증가해서 관리가 복잡하다고 느꼈는데요..!
이런 상황에 사용하는게 useQueries인 것 같네요!! 참고하겠습니당 :)

const { isLoading: followInfoLoading, data: followInfoData } =
useGetFollowInfo();

return userInfoLoading || followInfoLoading ? (

Choose a reason for hiding this comment

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

isLoading이라는 것을 판단하는 변수를 만들어서 써주면 더 직관적일것 같아요!

Suggested change
return userInfoLoading || followInfoLoading ? (
const isLoading = useInfoLoading || followInfoLoading
return isLoading ? (

queryClient.invalidateQueries(["get-follow-info"]);
},
});
return mutation;

Choose a reason for hiding this comment

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

이 useMutation에서 사용되는것은 mutate 메서드이기 때문에,
return 자체를 mutation.mutate로 해줘도 좋을것 가타요!

Suggested change
return mutation;
return mutation.mutate;


const useGetFollowInfo = () => {
const { isLoading, data } = useQuery({
queryKey: ["get-follow-info"],

Choose a reason for hiding this comment

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

queryKey는 상수 파일로 빼서 관리하면 더 좋을것 가타요!

Comment on lines +12 to +15
onError: () => {
alert("토큰을 정확히 입력해주세요.");
router.push("/");
sessionStorage.clear();

Choose a reason for hiding this comment

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

헙 에러 핸들링을 이렇게 커스텀 훅 내에서 해주는거 너무 좋아요!
저는 sessionStorage 지워주는 로직이 없는데 이거 참고해서 추가 해봐야겠어용✨

Comment on lines +30 to +38
<p className={styles.title}>
{list === matchedList
? "맞팔 중인 사용자"
: "내가 팔로우 안 한 사용자"}
</p>
<p className={styles.title}>{`${list.length} 명`}</p>
</div>

<ListLayout list={list} isUserInfo={false} isFollowingBtn={list !== matchedList} />

Choose a reason for hiding this comment

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

list === matchedList 로직을 여러 곳에서 사용하고 있는것 같아요!
플래그를 만들어줘서 활용하면 더 가독성이 좋아질것 같습니당

Suggested change
<p className={styles.title}>
{list === matchedList
? "맞팔 중인 사용자"
: "내가 팔로우 안 한 사용자"}
</p>
<p className={styles.title}>{`${list.length} 명`}</p>
</div>
<ListLayout list={list} isUserInfo={false} isFollowingBtn={list !== matchedList} />
const isMatchedList = list ===matchedList
<p className={styles.title}>
{isMatchedList
? "맞팔 중인 사용자"
: "내가 팔로우 안 한 사용자"}
</p>
<p className={styles.title}>{`${list.length} 명`}</p>
</div>
<ListLayout list={list} isUserInfo={false} isFollowingBtn={!isMtachedList} />

import Image from "next/image";
import * as styles from "../style/Common/ListLayout.css";

const ListLayout = ({ list, isUserInfo, isFollowingBtn }: ListLayoutTypes) => {

Choose a reason for hiding this comment

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

흐음 요건 개인적인 의견인데...
맞팔 중인 리스트 / 아닌 리스트를 구분해서 1) 버튼에 들어갈 텍스트 구분 2) mutate function 구분을 하는 플래그인 isFollowingBtn이 살짝 버튼 네이밍에 의존적인것 같아요...!
ListType 이런식으로 바꿔보면 더 가독성 있고, 내부 로직을 보여주지 않는 플래그 네이밍이 될수 있을것 같습니당!

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.

None yet

2 participants