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

Refactor/#655: 토픽 단일 조회 페이지 레이어 분리 #656

Merged
merged 9 commits into from
May 4, 2024

Conversation

semnil5202
Copy link
Collaborator

@semnil5202 semnil5202 commented Feb 29, 2024

작업 대상

  • 단일 토픽 조회 (SelectedTopic) 페이지를 대상으로 합니다.

📄 작업 내용

  • React Query 적용
    • TopicDetail을 받아오는 부분을 React Query로 적용했습니다.
  • 비즈니스 레이어 (custom hook, service) 적용으로 레이어 분리
    • 서버로부터 클러스터링 정보를 받아오는 로직을 커스텀 훅으로 분리했습니다.
    • 사용자가 지도를 드래그, 줌 할 때 발생하는 이벤트 처리 로직을 커스텀 훅으로 분리했습니다.
  • 그외 자잘한 오류 수정
    • pin id 상태 세팅 및 쿼리 파라미터 조작 로직을 커스텀 훅으로 분리했습니다.
    • setTags 커스텀 훅 내부로 tags를 초기화 하는 로직을 위치 시켰습니다.

🙋🏻 주의 사항

1. 여전히 refetch 함수를 사용하고 있습니다.

refetch 함수 대신 queryClient.invalidateQueries를 통한 리페칭을 시도하고 싶었으나, 현재 구조상 다발적인 (여기 고치면, 저기 고치고, 또 저기 고치고..) 리팩토링이 예상되어 임시로 refetch 함수를 사용하였습니다.

2. 과도한 추상화 및 커스텀 훅 분리 같다면 꼭 의견 부탁드립니다.

개인적으로 컴포넌트는 성공 케이스에 대한 UI 로직만 가져가면 좋겠다고 생각하여 보통 비즈니스 로직을 커스텀 훅으로 뺍니다. (재사용이 가능할 땐 더더욱 커스텀 훅으로 분리합니다.) 이 경우 과도한 추상화 및 커스텀 훅 분리가 이뤄질 수 있으며, 컴포넌트를 볼 때 해당 컴포넌트의 기능 (비즈니스 로직)이 훅으로 분리되어있기에 오히려 리팩토링 전보다 가독성이 떨어질 수도 있습니다.

아래 캡쳐본만 보고 해당 컴포넌트가 무슨 역할을 하는 것 같은지 대략적으로 예상이 가나요?
image

3. 페이지 단위 코로케이션 디렉토리 구조를 가져가면 어떨지요.

이 부분은 사전에 공유드리지 않았는데, 페이지 단위로 사용하는 컴포넌트, 타입 파일, 훅 등을 위치시키는게 어떨까 제안 드립니다. 보통 페이지 컴포넌트를 폴더로 두고, 해당 페이지 내부에서 사용되는 컴포넌트, 훅 등을 위치시키게 됩니다. 저희가 리액트 쿼리를 도입하면서 페이지 별로 query 및 mutation 훅 로직이 많아질 예정인데 이 모든 훅이 글로벌 hooks 파일로 들어갈 경우, 꽤 복잡해질 것 같습니다.

4. 페이지 컴포넌트 컨벤션으로 000.page.tsx 어떻습니까.

페이지 컴포넌트와 일반 컴포넌트가 구분이 모호하다고 생각합니다. 물론 pages, components 라는 폴더로 구분 짓고 있지만 작업물이 많아지면 생각보다 헷갈리더군요. 이 부분에서도 자유롭게 의견 부탁드립니다.

스크린샷

📎 관련 이슈

close #655

레퍼런스

추후 모아보기 페이지에서 재사용할 예정이다.
훅의 인자, 반환값의 네이밍 길이가 길어짐에 따라 위와 같이 변경한다.
추후 모아보기 페이지에서 사용할 수 있도록 한다.
모든 페이지마다 tags를 초기화해줘야하는 로직이 불필요하게 반복된다. 또한 휴먼에러가 발생할 가능성이 높은 부분으로 판단되어 useTags의 props로 받아 초기화가 불필요한 페이지는 명시적으로 초기화 하지 않도록 한다.
@semnil5202 semnil5202 requested review from jiwonh423 and GC-Park and removed request for jiwonh423 February 29, 2024 12:03
@semnil5202 semnil5202 self-assigned this Feb 29, 2024
@semnil5202 semnil5202 added FE 프론트엔드 관련 이슈 refactor 리팩토링 관련 이슈 labels Feb 29, 2024
Copy link
Collaborator

@jiwonh423 jiwonh423 left a comment

Choose a reason for hiding this comment

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

SelectedTopic페이지를 예를 들어 직접 보여줘서
3. 페이지 단위 코로케이션 디렉토리 구조가 뭔지 알게 되었네요 🙇🙇
이번에 분리된 훅만 해도 4개씩이나 되는데 전역적으로 두면 헷갈릴것 같다는 부분 완전히 공감되네요
자연스럽게 4. 페이지 컴포넌트 컨벤션으로 000.page.tsx로 통일하는게 훨씬 좋네요 😄

궁금한 부분은 refetch부분을 invalidateQueries로 대체하고싶다 하셨는데 어떤 방식인지 살짝 추가 설명 가능할까여? 제가 써본 경험은 useMutation 안에서 onSuccess일때 사용했었는데 세인이 생각하는 방식도 이건가요?

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.

refetch부분을 캐시 데이터를 초기화할 수 있고 쿼리 무효화할 수 있는queryClient.invalidateQueries로 바꾸는거 좋습니다! 그런데 각종 에러가 난다면 흠... 허헣 천천히 적용해봐야 겠군용

저도 컴포넌트 UI와 비즈니스 로직을 분리하는 것을 선호합니다! custom hook으로 빼는 것은 좋은 것 같습니다!

페이지 단위 코로케이션 디렉토리 구조 << 를 처음 듣는데 세인이 설명해준 것을 읽어보니 지금 저희 구조에서 적용하면 가독성 측면에서 좋을 것 같아요! 그런데 페이지별로 공통으로 쓰는 컴포넌트는 어떻게 해야할지 모르겠네용. 이 부분은 어떻게 분리를 하면 될까요?!

이번 pr에서 예시(SelectedTopic)을 보여줘서 이해하기 쉬웠고 000.page.tsx 컨벤션 완전 찬성입니다!!

구우우우웃 👍

@semnil5202
Copy link
Collaborator Author

궁금한 부분은 refetch부분을 invalidateQueries로 대체하고싶다 하셨는데 어떤 방식인지 살짝 추가 설명 가능할까여? 제가 써본 경험은 useMutation 안에서 onSuccess일때 사용했었는데 세인이 생각하는 방식도 이건가요?

네네 맞습니다. 근데 현재 구조에서 invalidateQueries로 한 번에 변경할 경우 큰 변경점이 예상되서 점진적으로 변경해야할 것 같습니다.

페이지 단위 코로케이션 디렉토리 구조 << 를 처음 듣는데 세인이 설명해준 것을 읽어보니 지금 저희 구조에서 적용하면 가독성 측면에서 좋을 것 같아요! 그런데 페이지별로 공통으로 쓰는 컴포넌트는 어떻게 해야할지 모르겠네용. 이 부분은 어떻게 분리를 하면 될까요?!

공통 컴포넌트는 기존에 사용하던 components 디렉토리에 위치하면 될 것 같아요~ 에러바운더리 등과 같은 것이 되겠네요.

@semnil5202 semnil5202 merged commit 34c985b into develop-FE May 4, 2024
1 check failed
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