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

[FE] Refactor/#645: Home 페이지 API 로직 수정 및 React-Query 적용 #646

Merged
merged 6 commits into from
Feb 5, 2024

Conversation

jiwonh423
Copy link
Collaborator

작업 대상

  • Home페이지 tanstack-query 및 axios 적용

📄 작업 내용

  • 새로운 api 폴더를 생성해서 http.ts와 index.ts 파일을 만들었습니다
  • http.ts는 http메소드, index.ts는 지도들을 가져오는 getTopics함수가 있습니다
  • 홈 화면에서 각각 데이터를 fetch하는 곳은 TopicListContainer의 TopicCardContainer라서 useState와 useEffect를 useQuery로 대체했습니다
  • 이전에 자식 컴포넌트에 getTopicsFromServer로 넘겨주던 함수를 refetch로 대체했는데 문제 없는지???

🙋🏻 주의 사항

  • 인기 급상승 지도의 지도1을 모아보기 해제하면 새로울지도의 지도1에 반영 안되는 문제
  • getTopicsFromServer의 정확한 목적 헷갈리네여
  • refetch로 대체가능하면 error랑 로딩 처리 어떻게 할지 정해보죠

스크린샷

📎 관련 이슈

#645

#645

레퍼런스

https://github.com/ssi02014/react-query-tutorial

@jiwonh423 jiwonh423 added FE 프론트엔드 관련 이슈 refactor 리팩토링 관련 이슈 labels Jan 14, 2024
@jiwonh423 jiwonh423 self-assigned this Jan 14, 2024
Copy link
Collaborator

@semnil5202 semnil5202 left a comment

Choose a reason for hiding this comment

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

로직상 문제는 없어보여서 바로 어프로브로 드립니다. 고생많으셨어요~~

본문에 적어주신 refetch 부분을 정책적으로 고민해봐야할 것 같습니다. 결론만 말씀드리면 현재 겪고 계시는 상황이 원래 의도된 상황은 맞습니다. 이전에는 모아보기 버튼 누르면 인기급상승할지도, 새로울지도, 모두일지도 총 세 개 항목의 모든 지도를 불러왔었습니다. 근데 이게 거의 동일한 요청을 세 번이나 날리면서 비용이 많이 들어가기도 하고 지도가 많아지면 같은 지도가 다른 항목에서 보일 확률이 적어질 것이라고 판단해서 그렇게 바뀐 것이에요.

refetch 함수 관련해서 프롭스 드릴링을 개선하고 선언적으로 처리할 수 있는 방법을 좀 더 고민해봐야할 것 같습니다. 이 부분은 좀이따 회의 때 얘기해보시죠.

아무튼 고생많으셨습니다 👍


const axiosInstance = axios.create({
baseURL: BASE_URL,
headers: token ? { Authorization: `Bearer ${token}` } : {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

token 없으면 Authorization: undefined로 들어가서 분기처리 해줘야하는게 맞겠져?

import { getTopics } from '../../api';

const useTopicsQuery = (url: string) => {
const { data: topics, refetch: refetchTopics } = useQuery({
Copy link
Collaborator

Choose a reason for hiding this comment

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

refetch 메서드를 통해 수동으로 리페칭 하는게 지금 저희 상황에 딱 맞는 상황 같아보이긴합니다.

그런데 refetch 함수가 props drilling 되는 문제도 그렇고, 모아보기 또는 즐겨찾기 버튼을 누르면 queryKey 등을 통해서 해당 버튼 컴포넌트 내부에서 refetch 할 수 있으면 좋겠지만 그런 메서드가 없는 것 같더군요?? (제가 못찾은거일 수 도 있습니다.)

아무튼 자동 refetch 여기 블로그 말대로라면 상태를 넘겨서 선언적으로 갱신시켜라 이런말이 있습니다. 리액트 쿼리에서 강조한다는 내용인데 따로 참고 링크가 없어서 완전히 신뢰하진 않지만 한 번 확인해보는 것도 좋아보입니다.

하지만 결과적으로 봤을 때 본문에도 적어두겠지만 refetch 하는 이유가 유저가 즐겨찾기 하면서 인기있을지도의 순위 변동이 일어나고 그에 따른 최신 업데이트를 위한 것인데 이 과정이 비용이 엄청 들긴합니다. topics 호출을 세 번 하는 셈이니.. 이 부분은 정책적으로 좀 더 고민해봐야할 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

오호.. 세인이 말씀해주셔서 이 부분에 대해서 저는 물러나겠습니다~

@@ -21,11 +22,15 @@ if (process.env.REACT_APP_GOOGLE_ANALYTICS) {
ReactGA.initialize(process.env.REACT_APP_GOOGLE_ANALYTICS);
}

const queryClient = new QueryClient();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 옵션 회의 때 한 번 얘기해봅시다. 윈도우 포커스 될 때 자동 refetch 하는게 기본값 같던데, 그럴 필요까지는 없어보이니..

Copy link
Collaborator

@GC-Park GC-Park left a comment

Choose a reason for hiding this comment

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

저도 크게 수정할 부분이 안 보여서 바로 approve 드립니다!

처음부터 코드를 아주 깔끔하게 짜두었군요..! 이게 DH 클라스... 난 왜?..

import { getTopics } from '../../api';

const useTopicsQuery = (url: string) => {
const { data: topics, refetch: refetchTopics } = useQuery({
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 +30 to +32
getTopicsFromServer?: (
options?: RefetchOptions | undefined,
) => Promise<QueryObserverResult<TopicCardProps[], Error>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

QueryObserverResult에 대한 설명을 들을 수 있을까욧?!! 찾아봤는데 이해가 안 가서요.. 흑흑


const axiosInstance = axios.create({
baseURL: BASE_URL,
headers: token ? { Authorization: `Bearer ${token}` } : {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

오호.. 이 부분을 까먹었군요! 저도 변경 해야겠습니다! 굿 👍

@semnil5202 semnil5202 merged commit f67141a into develop-FE Feb 5, 2024
1 check passed
@semnil5202 semnil5202 deleted the refactor/home branch February 9, 2024 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 관련 이슈 refactor 리팩토링 관련 이슈
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants