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 #495

Conversation

sj0724
Copy link
Collaborator

@sj0724 sj0724 commented Jun 30, 2024

요구사항

기본

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

주요 변경사항

  • 리액트 쿼리 적용했습니다.
  • 전체적으로 리팩토링 같이 진행했습니다.

멘토에게

  • app 라우터를 이제 공부하고 있어서 아직 페이지 라우터로 적용되어있습니다. 이번주에 공부해서 app 라우터로 마이그레이션 진행해보도록 하겠습니다.
  • app 라우터를 사용하면 리액트 쿼리를 사용안한다고 하셨는데 next cache를 사용해서 리액트 쿼리를 사용하지 않는 것이 맞나요?
  • https://sj-linkbrary.vercel.app 배포 링크입니다.

sj0724 and others added 30 commits May 29, 2024 17:27
fix: react-hook-form정상적용
refactor: 폴더 리스트 페이지 전체 로직 리팩토링
Feature: 검색기능 리팩토링, 링크 리스트, 로그인 리액트 쿼리 적용
@sj0724 sj0724 requested a review from 1005hoon June 30, 2024 13:13
@sj0724 sj0724 self-assigned this Jun 30, 2024
@sj0724 sj0724 added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jun 30, 2024
@sj0724 sj0724 changed the title Part3 박상준 week15 [박상준]-Week19 Jun 30, 2024
@sj0724 sj0724 changed the title [박상준]-Week19 [박상준] Week19 Jun 30, 2024
@1005hoon
Copy link
Collaborator

@sj0724

상준님~ 먼저 질문 사항에 대한 답 드릴게요

app 라우터를 사용하면 리액트 쿼리를 사용안한다고 하셨는데 next cache를 사용해서 리액트 쿼리를 사용하지 않는 것이 맞나요?

멘토링에서는 빠르게 요약하기 위해 캐싱 레이어에 대해서만 이야기를 했는데요.

우선 React Query를 왜 사용해왔는지 먼저 고민해보면 좋겠어요. React Query는 화면 상 존재하는 데이터를 관리하기 위해 활용되는 라이브러리에요. 그런만큼, 복잡한 클라이언트 측 데이터 요청을 관리해주고, query key 기반으로 데이터를 캐싱하고, 조건에 따라 client side data를 update해주는 등 기능을 제공해 줍니다.

NextJS는 App router를 제공하며 framework를 제대로 사용하기 위해 client side와 server side를 어던 경우 사용해야 하는지를 명시했는데요. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns
위 문서를 살펴보면, 데이터를 불러오고 관리하며, 무거운 연산과 번들 사이즈가 큰 요소들은 서버쪽에 관리하도록 구조를 명시하고 있지요.

여기서 NextJS가 제공하는 기능과 React Query가 제공하는 기능 중 겹치는 부분들이 발생합니다.

  • 데이터 요청 처리 관리
  • 특정 키 기반 데이터 캐싱 (NextJS: Fetch Endpoint, React Query: Query Key)
  • 조건 부 데이터 업데이트 (NextJS: mutation을 기반으로 refresh / invalidation 처리, React Query: 다양한 케이스 존재)

가볍게 정리해봐도 상당히 많은 부분들이 공통으로 겹쳐지는걸 확인할 수 있죠?
그렇다면 NextJS에서 굳이 React Query를 사용해야만 하는 경우 - 예: 사용자가 다른 탭 이동했다가 돌아왔을 때 데이터 리프레시를 처리해야 하는경우 - 가 아니라면 React Query를 사용함으로 프로젝트의 데이터 관리를 이원화 할 필요는 없다 사료됩니다.

위 관점에서 명확하게 React Query가 사용되어야 할 이유가 없다면 굳이 NextJS 프로젝트에 React Query를 사용할 필요가 없지 않을까요?

@sj0724
Copy link
Collaborator Author

sj0724 commented Jul 2, 2024

주말에 nextjs 튜토리얼을 따라서 해보고 직접 적용해보려고 일단 위클리 미션에 적용했습니다.
현재는 로그인, 회원가입, 공유페이지까지 리팩토링 되었습니다. 코드 리뷰해주실때 추가적으로 확인해주시면 감사하겠습니다!

@1005hoon
Copy link
Collaborator

1005hoon commented Jul 3, 2024

배포 환경에서 한번 회원가입 ~ 로그인을 해보았는데요,
회원가입 후 /signin 페이지로 이동되어 404 페이지가 보여집니다
image

또한 로그인을 진행하면 로그인 후 잠깐동안 모든 에러가 활성화 되었다가 페이지가 이동되어요 !

@1005hoon
Copy link
Collaborator

1005hoon commented Jul 3, 2024

폴더와 링크 추가할 때 오류가 납니다
image

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.

@sj0724

상준님, 프로젝트 리팩토링 하느라 고생 많으셨습니다!
app router를 바로 적용하기가 쉽지 않으시죠 ㅎㅎ

그래도 데이터 페칭 부분을 어느정도 서버단으로 옮겨오신 만큼, 천천히 공식문서 참고하며 해보면 server component와 client component의 조합, 그리고 server side fetching, loading, error handling등 금방 익숙해지시리라 생각합니다.

전반적으로 콤포넌트 복잡도가 높은 편인데요, 이전 멘토링 때 이야기 했던 로직의 분리, 관심사의 분리를 한번 잘 생각해보며 리팩토링 해보면 좋겠어요.

또한 콤포넌트와 함수, 그리고 변수들의 이름들은 조금 더 직관적으로 사용되어도 좋겠습니다.

자세한 내용은 코드단 리뷰에 남겨두었으니 참고 부탁드려요.

고생 많으셨습니다!

api/api.ts Show resolved Hide resolved
@@ -52,9 +30,9 @@ export async function getSampleFolder() {
}
}

export async function getFolder(id: string) {
export async function getFolder() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

getFolder 등 UI에 필요한 불러오는 서비스 레이어에서 각 함수들이 어떤 값을 반환하는지 타입을 설정해주면 UI에서 사용하기 편하지 않을까요?

또한, 에러핸들링의 경우, 던져진 오류를 catch로 잡아주었는데, 굳이 다시 던져주는 이유가 있을까요~?

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 함수는 초기에 작성하고 리펙토링을 따로 진행하지 않아 다른 부분에서 코드를 인용해서 사용하고 그대로 뒀습니다. 그래서 따로 에러를 다시 던지는 이유는 없습니다.

Comment on lines +54 to +58
let query;
if (folderId) {
try {
const query = `/${id}/links?folderId=${folderId}`;
const { data } = await axios.get(`/users${query}`);
return data.data;
} catch (error) {
console.error('Error fetching folderList:', error);
throw error;
}
} else if (!folderId) {
try {
const query = `/${id}/links`;
const { data } = await axios.get(`/users${query}`);
return data.data;
} catch (error) {
console.error('Error fetching folderList:', error);
throw error;
}
query = `/folders/${folderId}/links`;
} else {
query = '/links';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

쿼리는 무언가를 조회하기 위한 조건을 이야기합니다.
그렇다면, 네트워크 요청이 실제 발생하는 요소는 뭐라고 하는게 좋을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

url이라는 이름으로 사용해도 될까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

url은 전체 주소를 주로 이야기를 하죠, 이 경우는 endpoint라는 변수로 만들어 사용해주면 어떨까 싶습니다

Comment on lines +60 to +66
const result = await axios.get(`${query}`);
return result;
}

export async function getUser(accessToken: string) {
try {
const { data } = await axios.get('/users', {
headers: {
Authorization: accessToken,
},
});
return data.data;
} catch (error) {
console.error('Error fetching user:', error);
throw error;
}
export async function getUser() {
const { data } = await axios.get('/users');
return data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

요 녀석들도 적합한 에러 핸들링이 필요해요

api/api.ts Outdated
email: id,
password: password,
});
localStorage.setItem('token', data.accessToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

accessToken은 로그인한 사용자에 대한 중요한 정보를 담고있어요.
누군가가 이 정보를 해킹한다면 바로 사용자 계정으로 접근해 핵심 정보를 추출해올 수 있겠죠.
그렇게 중요한 데이터를 localStorage에 관리하는건 좋ㅈ ㅣ않아 보이는데요, 어떻게 개선해볼 수 있을지 고민해볼까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

우선 nextauth를 사용해서 세션에 저장하도록 변경했습니다.

Comment on lines +39 to +43
if (data.favorite) {
await putLinkLike(data.linkId, false);
} else {
await putLinkLike(data.linkId, true);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

await putLInkLike(linkId, !favorite)

if else문 없이 바로 처리 가능할 듯 해요.

Comment on lines +45 to +69
onMutate: async (data: { linkId: string; favorite: boolean }) => {
if (folderId) {
const result = queryClient.getQueryData<any>(["links", folderId]);
const refetchLink = { ...result.data[index], favorite: !data.favorite };
let refetchLinkArr = [...result.data];
refetchLinkArr[index] = refetchLink;
queryClient.setQueryData(["links", folderId], (prev: any) => {
return {
...prev,
data: refetchLinkArr,
};
});
} else {
const result = queryClient.getQueryData<any>(["links"]);
const refetchLink = { ...result.data[index], favorite: !data.favorite };
let refetchLinkArr = [...result.data];
refetchLinkArr[index] = refetchLink;
queryClient.setQueryData(["links"], (prev: any) => {
return {
...prev,
data: refetchLinkArr,
};
});
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

이부분은 로직이 많이 복잡하네요, 콤포넌트가 전달받는 prop을 개선시킴으로 훨씬 간편하게 해볼 수 있겠는데요.
한번 콤포넌트 설계를 다시해볼 시간이 있다면 도전해보아도 좋겠어요!

fill
/>
</S.StarIcon>
{isActive && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

isActive라는 이름보다는 showFavoriteButton을 프롭값으로 해주는건 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

음 아래를 보니, 단순 showFavoriteButton로 해결되는게 아니라, 유저가 로그인 되어있는지에 따라 보이는 값이 달라지는 듯 싶네요. 그럼 showFavoriteButton보단, showButtons이 더 좋지 않을까 싶네요

@@ -14,45 +15,48 @@ export type LinkData = {

export interface Links extends Array<LinkData> {}

function useGetFolder(id: string, searchKeyword: string, folderId: string) {
function useGetFolder(deBounceValue: string, folderId: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

이런식으로 훅을 만들게 되면 앞으로 존재하는 모든 http method에 따라 훅이 다 만들어지게 될 것 같은데요.
useFolder 라는 훅 하나를 만들어 get all, create, update, delete 메소드를 제공해주도록 일원화 시키면 어떨까요?

function useGetFolderList(userId: string, folderId?: string) {
const [link, setLink] = useState<Folders>([]);
const [linkLoading, setLinkLoading] = useState(false);
function useGetFolderList() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sj0724

배열을 반환하는 경우라면 복수형을 사용해주세요!
useFolders

@1005hoon 1005hoon merged commit 452ee5e into codeit-bootcamp-frontend:part3-박상준 Jul 3, 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