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

Part3 김민섭 week19 제출 #497

Conversation

striker1826
Copy link
Collaborator

주요 변경사항

  • 기존 코드 리펙토링
  • 기존 API들 react-query로 변경

멘토에게

  • 커밋이 한꺼번에 많이 올려드려서 죄송합니다.
  • 기존 코드 리펙토링을 중점적으로 진행했습니다.

@striker1826 striker1826 requested a review from kiJu2 July 1, 2024 04:53
@striker1826 striker1826 added the 순한맛🐑 마음이 많이 여립니다.. label Jul 1, 2024
@kiJu2
Copy link
Collaborator

kiJu2 commented Jul 1, 2024

수고 하셨습니다 ! 스프리트 미션 하시느라 정말 수고 많으셨어요.
학습에 도움 되실 수 있게 꼼꼼히 리뷰 하도록 해보겠습니다.

@kiJu2
Copy link
Collaborator

kiJu2 commented Jul 1, 2024

커밋이 한꺼번에 많이 올려드려서 죄송합니다.

아하핳 괜찮습니다 ! 모든 코드 보면서 리뷰해보도록 할게요 !

기존 코드 리펙토링을 중점적으로 진행했습니다.

넵넵 😊

@kiJu2
Copy link
Collaborator

kiJu2 commented Jul 1, 2024

commit 단위를 더욱 자주, 작게 해보시는건 어떠실까요?

git을 다룰 때 commit은 "언제 해야 하는가"를 생각해보신 적 있으신가요?
흔히 하는 말이 있습니다:

커밋은 합칠 수 있지만 나눌 수 없습니다.

그럼 커밋을 언제 해야 할까요?

저는 다음과 같은 룰을 지키며 커밋을 하는걸 권장 드립니다:

  1. 커밋을 하는 단위는 커밋 메시지 한 줄로 설명할 수 있는 행동
  2. 하나의 목표 혹은 액션이 달성될 때

관련하여 읽으시면 좋은 아티클을 추천드릴게요:

tl;dr

관련 변경 사항 커밋
커밋은 관련 변경 사항에 대한 래퍼여야 합니다. 예를 들어 두 개의 다른 버그를 수정하면 두 개의 별도 커밋이 생성되어야 합니다. 작은 커밋을 통해 다른 개발자가 변경 사항을 더 쉽게 이해하고 문제가 발생한 경우 롤백할 수 있습니다. 준비 영역과 같은 도구와 파일의 일부만 준비하는 기능을 통해 Git을 사용하면 매우 세부적인 커밋을 쉽게 만들 수 있습니다.

자주 커밋
커밋은 커밋을 작게 유지하고 관련 변경 사항만 커밋하는 데 도움이 되는 경우가 많습니다. 또한 이를 통해 코드를 다른 사람들과 더 자주 공유할 수 있습니다. 이렇게 하면 모든 사람이 정기적으로 변경 사항을 통합하고 병합 충돌을 방지하는 것이 더 쉬워집니다. 대조적으로, 대규모 커밋을 갖고 이를 드물게 공유하면 충돌을 해결하기가 어렵습니다.

미완성 작업을 커밋하지 마십시오
논리적 구성 요소가 완료된 경우에만 코드를 커밋해야 합니다. 자주 커밋할 수 있도록 기능 구현을 빠르게 완료할 수 있는 논리적 청크로 분할합니다. 깨끗한 작업 복사본이 필요하기 때문에(브랜치 확인, 변경 사항 가져오기 등) 커밋하고 싶은 유혹이 든다면 Git의 «Stash» 기능을 대신 사용하는 것이 좋습니다.

커밋하기 전에 코드를 테스트하세요
완료되었다고 생각하는 일을 저지르고 싶은 유혹에 저항하세요. 철저하게 테스트하여 실제로 완료되었는지, 부작용이 없는지(알 수 있는 한) 확인하세요. 로컬 저장소에 설익은 것을 커밋하려면 자신만 용서하면 되지만, 코드를 다른 사람과 푸시/공유하는 경우에는 코드를 테스트하는 것이 훨씬 더 중요합니다.

원문 보기

또한 깃 커밋 메시지 컨벤션도 함께 읽어보세요:

tl;dr:

커밋 메시지 형식

type: Subject

body

footer

기본적으로 3가지 영역(제목, 본문, 꼬리말)으로 나누어졌다.

메시지 type은 아래와 같이 분류된다. 아래와 같이 소문자로 작성한다.

feat : 새로운 기능 추가
fix : 버그 수정
docs : 문서 내용 변경
style : 포맷팅, 세미콜론 누락, 코드 변경이 없는 경우 등
refactor : 코드 리팩토링
test : 테스트 코드 작성
chore : 빌드 수정, 패키지 매니저 설정, 운영 코드 변경이 없는 경우 등

원문보기

Comment on lines +11 to +19
export const useGetUserInfo = {
queryKey: () => key.user(),

queryOptions: () =>
queryOptions<{ data: { data: User[] } }>({
queryKey: useGetUserInfo.queryKey(),
queryFn: () => apiInstance.get(CODEIT_USER),
}),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 객체는 hook으로 보이지 않습니다 !:

Suggested change
export const useGetUserInfo = {
queryKey: () => key.user(),
queryOptions: () =>
queryOptions<{ data: { data: User[] } }>({
queryKey: useGetUserInfo.queryKey(),
queryFn: () => apiInstance.get(CODEIT_USER),
}),
};
export const userQuery = {
queryKey: () => key.user(),
queryOptions: () =>
queryOptions<{ data: { data: User[] } }>({
queryKey: useGetUserInfo.queryKey(),
queryFn: () => apiInstance.get(CODEIT_USER),
}),
};

보편적으로 use ~는 커스텀 훅에 사용됩니다. 위처럼 다른 네이밍을 사용해보는게 어떨까요 !?

Copy link
Collaborator

Choose a reason for hiding this comment

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

혹은 다음과 같이 쿼리들을 관리할 수도 있어요:

export const userQueries = {
  getUsers: () => null,
  getUserById: () => null
};

그리고 키들은 다음과 같이 !

export const userKeys = {
  getUsers: ['user'],
  getUserById: (id: string) => ['user', id]
};

const Header = () => {
const { userData, error } = useGetUserData();
export const Header = () => {
const { data: userData } = useQuery(useGetUserInfo.queryOptions());
Copy link
Collaborator

Choose a reason for hiding this comment

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

만약 전 리뷰처럼 수정된다면 다음과 같이 수정되겠네요 !:

Suggested change
const { data: userData } = useQuery(useGetUserInfo.queryOptions());
const { data: userData } = useQuery({ queryFn: userQueries.getUser, queryKey: userQueryKeys.getUser });

Comment on lines +3 to +21
export function useAuthGuard() {
const router = useRouter();

type GuardPath = "token" | "notToken";

const guardPath: Record<GuardPath, Record<string, boolean>> = {
token: { "/folder/[[...folderId]]": true },
notToken: { "/signin": true },
};
// SSR에서의 접근을 막기 위한 로직입니다.
if (typeof window === "undefined") return;
const pathname = router.pathname;
const token = localStorage.getItem("accessToken");

if (!token && guardPath["token"][pathname]) {
router.push("/signin");
return;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오호? 커스텀 훅으로 인가가 되지 않았을 때 접근을 방어하셨군요? 🫢

Comment on lines +9 to +10
token: { "/folder/[[...folderId]]": true },
notToken: { "/signin": true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

만약, signup도 화이트 리스트(notToken)에 추가하고 싶다면?

만약 singup페이지도 추가하고 싶다면 코드에 변경이 필요할 것 같아요. 화이트 리스트는 보편적으로 배열로 관리하기에, 배열로 만들어두는게 어떨까요?

Comment on lines +9 to +10
token: { "/folder/[[...folderId]]": true },
notToken: { "/signin": true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 변수명 제안드립니당 😉:

Suggested change
token: { "/folder/[[...folderId]]": true },
notToken: { "/signin": true },
requireLoggedIn: { "/folder/[[...folderId]]": true },
requireLoggedOut: { "/signin": true },

Comment on lines +4 to +6
const key = {
linkList: () => ["users/1/links"],
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

함수로 만든 이유가 있을까요?

Suggested change
const key = {
linkList: () => ["users/1/links"],
};
const key = {
linkList: ["users/1/links"],
};

값이므로 배열 값으로 충분할 것 같아요 !

혹은 다음과 같이 동적으로 작성할 수도 있을 것 같아요:

Suggested change
const key = {
linkList: () => ["users/1/links"],
};
const key = {
linkList: (id: string) => ['users', id, 'links'],
};


export const signinApi = async (data: { email: string; password: string }) => {
const res = await apiInstance.post("sign-in", data);
if (!res) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

API 통신부에서 에러가 났을 때 어떻게 대처하면 될까요?

제가 예상하는 문제는 로그인을 하는데 "이미 등록된 아이디임"과 "아이디에 특수문자 들어가면 안됨" 이라는 에러 UI를 출력한다고 생각해봅시다..!

현재 error를 그냥 undefined로 반환해주고 있기에 해당 함수를 사용하는 컴포넌트에서는 분기처리하기 힘들거예요 !

그렇다면 어떻게 할까요?

방법은 다양합니다만, 지금 바로 해볼 수 있는 방법은 throw를 해보는거예요:

Suggested change
if (!res) return;
try {
} catch (error) {
console.error(`Failed to fetch data: ${error}`); // 통신부에서 처리할 로직 및 로깅
throw(error);
}

위처럼 throw를 해준다면 서버에서 보내준 에러의 메시지를 사용자에게 toast든, 모달이든, 알러트든 보여줄 수 있겠죠?

다음과 같이요 !!:

  // Component
  useEffect(() => {
    try {
      getItemsComments();
  // 이어서..
    } catch (err) {
      alert(err.message) 
    }
  }, [])

import apiInstance from "@/shared/model/axios";

export const signinApi = async (data: { email: string; password: string }) => {
const res = await apiInstance.post("sign-in", data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

반환 타입을 지정해볼까요?:

Suggested change
const res = await apiInstance.post("sign-in", data);
const res = await apiInstance.post<SignInResponse>("sign-in", data);

제네릭으로 위와 같이 반환 타입을 지정할 수 있습니다 !


localStorage.setItem("accessToken", result.data.accessToken);
window.location.replace("/folder");
console.log("signin");
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.log를 남기게 되면 추 후 디버깅이 어려울 수 있습니다 !:

git add {path} -p 옵션

git add . -p를 사용하게 되면 변경사항을 스테이징에 올릴 때 파일 내 코드 단위로 잘라서 올릴 수 있습니다 ! 상당히 유용하므로 히스토리를 신경쓰신다면 꼭 사용해보세요 😊

어떻게 사용하지?

git add . -p

Comment on lines +22 to +23
localStorage.setItem("accessToken", result.data.accessToken);
window.location.replace("/folder");
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 함수는 클라이언트 사이드에서만 사용할 수 있겠군요? 🤔

@kiJu2
Copy link
Collaborator

kiJu2 commented Jul 1, 2024

수고하셨습니다 민섭님 !! 😊
변경사항이 깔끔해서 집중해서 리뷰할 수 있었어요 !
그리고 FSD 아키텍쳐를 적용해보신 것 같아요. 도전성이 좋으시군요 😉👍

@kiJu2 kiJu2 merged commit 65bb9c0 into codeit-bootcamp-frontend:part3-김민섭 Jul 1, 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