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

[권주현] Week19 #493

Conversation

kuum97
Copy link
Collaborator

@kuum97 kuum97 commented Jun 30, 2024

요구사항

기본

  • 변경된 api를 활용해 주세요.
  • [] 모달에 필요한 api 요청을 만들어 기능을 완성해 주세요.
  • api 요청에 TanStack React Query를 활용해 주세요.

주요 변경사항

  • 인증부분 탠스택 쿼리 적용

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@kuum97 kuum97 requested a review from 1005hoon June 30, 2024 10:54
@kuum97 kuum97 self-assigned this Jun 30, 2024
@kuum97 kuum97 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Jun 30, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

api 요청 경로 변경 코드입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기본 html 태그 타입을 상속해주었습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이메일 중복체크와 회원가입 요청 및 라우팅을 커스텀훅으로 묶어주었습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

유저 인증 폼의 타입을 따로 파일을 만들어 분리해주었습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useSignup과 마찬가지로 쿼리 적용과 커스텀훅으로 묶어 구현해주었는데 인증 후 리다이렉트는 미들웨어를 구현해야 하는데 아직 미구현 상태라서 이 부분은 미완성 상태입니다.

Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

@kuum97

주현님 오랜만에 다시 뵈어요 ㅎㅎ
전반적으로 파트1에서 드렸던 가이드를 잘 따라가며 개발하신 부분이 보여 잘 발전하고 계시는구나 생각되어요.
이제 더 개선시킬 수 있는 방법으로 다음 요소들을 한번 고민해볼까요?

  1. NextJS의 데이터 관리와 베스트 케이스 프랙티스 (server data와 client data)
  2. typescript와 nextjs 프로젝트 컨벤션 (타입 네이밍, 변수와 함수 이름 등)
  3. React Query와 Server Rendered 요소 함께 활용하는 방법 (https://supabase.com/blog/react-query-nextjs-app-router-cache-helpers, https://tanstack.com/query/v4/docs/framework/react/guides/ssr)
  4. ui / service / controller layer 분리 (https://blog.testdouble.com/posts/2019-11-04-react-mvc/)

다음 요소들을 한번 고려하며 코드 리뷰 살펴보면 도움이 될 거에요!
위클리 미션 하시느라 고생 많으셨고, 궁금한거 있으시면 코멘트 남겨주세요.
멘토링 때 뵙겠습니다

@@ -9,6 +8,7 @@ import {
Response,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

NextJS에서 이미 서버 리스폰스 객체를 위한 Response라는 예약어가 사용되고 있을텐데요,
page router를 사용할때는 충돌이 없었나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

예약어는 NextResponse로 설정되어 있어 충돌은 없지만 Response라는 단어가 너무 포괄적인 단어라 수정해야 할 듯 합니다.

Comment on lines 3 to 4
FolderData,
LinkData,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

타입 또는 인터페이스를 선언할 때, 해당 타입들이 특정 데이터의 타입임을 표현하고 있는데요.
굳이 ~~Data 라는 식으로 타입 네이밍을 하는게 과연 좋을 지 한번 고민해볼 수 있으면 좋겠어요.

@@ -9,6 +8,7 @@ import {
Response,
UserData,
} from "./types/api";
import { FormValues } from "./types/form";
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

앞으로 form value들이 굉장히 다양하게 존재하게 될 텐데요.
지금 사용되는 formvalues 타입의 경우, 계정 정보 활용을 위한 폼 데이터를 관리하는 요소로 보여요.
그렇다면 form value보다는 더 좋은, 그리고 직관적인 이름이 있을텐데요.
어떻게 변경하면 좋을 지 한번 고민해볼까요?

@@ -134,70 +134,72 @@ export async function getLinksByFolderId({

// POST

export async function postEmailCheck(email: string): Promise<void | string> {
const response = await fetch(`${CODEIT_BASE_URL}/check-email`, {
export async function postEmailCheck(email: string): Promise<boolean | string> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

크게 프로젝트 아키텍쳐를 레이어로 나누어보면, 네트워크 요청을 전담하는 controller layer와 비즈니스 로직을 담당하는 service layer, 그리고 결과물을 사용자에게 보여주고 ui interaction을 처리하는 ui layer로 구분해볼 수 있어요.

여기서 postEmailCheck 과 같은 함수들이 우리가 말하는 비즈니스 로직을 담고 있는 서비스 레이어로 평가되는데요.
비즈니스 로직은 ui 레이어에서 최대한 필요한 데이터만 받아와 활용할 수 있도록 해주는 목적을 지니고 있습니다.

지금 작성된 코드를 살펴보면, ui 레이어에서 바로 필요한 데이터를 받아와 활용하기 어려워보여요.
특히, 이 함수의 반환값이 object일수도 있고, string일수도 있고, error 가 던져질 수도 있기 때문인데요.
이를 어떻게 개선 시킬 수 있을지 생각해보면 어떨까요?

Comment on lines 158 to 165
interface postData {
data: {
accessToken: string;
refreshToken: string;
};
data:
| {
accessToken: string;
refreshToken: string;
}
| { message: string };
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97
마찬가지로 postData라는 인터페이스의 이름은 직관적이지 않아요. 어떤 응답에 대한 타입을 설정한건지를 명시를 해주면 어떨까요?

또한, 서버에서 반환되는 응답이 data type이거나 오류인경우 message: string으로 반환된다면, 이를 공통적으로 활용할 수 있는 ServerResponse와 같은 인터페이스와 제네릭을 조립하는 형태로 타입을 사용하는게 더 좋을 것 같은데 어떻게 생각하시나요?

@@ -28,7 +27,7 @@ export function useLink({ folderId, localAccessToken }: LinkProps) {
});
return data;
},
enabled: IS_CLIENT,
enabled: !!window,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

NextJS에서 서버사이드 렌더링 환경과 클라이언트 사이드 환경에 구분없이 react query를 사용하기 위해선 서버 렌더와 하이드레이션을 통해 활용하는게 더 적합할텐데요 https://tanstack.com/query/latest/docs/framework/react/guides/ssr.
공식문서를 참고해 프로젝트 구조를 개선할 필요가 보입니다.

Comment on lines +12 to +13
mutation.mutate({ email, password });
router.replace("/");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

mutate 요청이 끝난 후 router.replace가 실행되어야 하죠?
지금 상황에서는 mutate에서 필요한 비동기 호출 처리가 끝나는걸 기다리지 않을텐데요.
mutate 끝난 후 router.replace되도록 수정이 필요합니다.

const emailCheckMutation = useMutation({
mutationFn: postEmailCheck,
onSuccess: (data) => {
if (typeof data === "string") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

서비스 레이어에서 반환된 응답이 문자열인 경우를 오류케이스로 처리하는건 상당히 Fragile한 로직이에요.
좀 더 명시적인 로직 처리를 할 수 있도록 하는게 좋겠어요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

몇가지 개선될 수 있는 부분이 보입니다.

  1. 초기 상태 명시적으로 설정해 상태의 기본값을 명확히 하고, searchQuery와 links의 타입을 명확히 해주세요
  2. 함수 네이밍이 리액트의 useState컨벤션과 동일한데요, 이를 zustand 스탠더드에 맞춰보면 어떨까요? domain / setDoamin 흐름보단 더 직관적인 네이밍을 사용하는게 좋겠어요.
  3. 데이터 타입 이름 개선을 해볼 수 있겠어요. UserData -> User NewFolderData / FolderData -> Partial<Folder> 등으로요

Comment on lines +15 to +16
const localAccessToken =
typeof window !== "undefined" && localStorage.getItem("accessToken");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuum97

사용자 로그인 인증 / 인가 정보를 담고있는 accessToken을 브라우저 localStroage에 관리하는게 과연 보안 차원에서 좋은 방법일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 부분은 당시에 프로젝트 요구사항이라 이런 식으로 처리해주었는데 19주차부터 api가 바뀌어서 수정이 안된 사항입니다. 일단은 진입점부터 다시 수정해나가자는 방향으로 홈(/)과 유저 인증 부분부터 수정했는데 제가 PR을 확실하게 올리지 않아 번거롭게 해드려 죄송합니다.

@1005hoon 1005hoon merged commit 05bdaec into codeit-bootcamp-frontend:part3-권주현 Jun 30, 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