Refactor(client): 타임테이블 추가 기능 개선 #307
Conversation
|
🏴☠️ Storybook 확인: 🔗 https://677bd6bc4909f2f48f4e0f42-iyutlkupah.chromatic.com/ |
|
|
||
| if (onSuccessCallback) { | ||
| onSuccessCallback(); | ||
| } | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
네트워크 요청 중복현상을 해결하기 위해 onSuccess 를 이용해서 post 요청이 성공되었을 때에 callback 함수를 처리할 수 있도록 개선하였습니다.
onSuccess 스코프 안에서 navigate를 이용하는 것보다 useAddTimeTableFestival 훅은 데이터 처리에만 책임을 질 수 있고, onSuccessCallback 을 넘김으로 써, 결합도를 낮춘다고 판단했어요
| @@ -2,3 +2,4 @@ export const HALF_HOUR_TO_MINUTES = 30; | |||
| export const TIME_SLOT_HEIGHT_5_MIN = 0.75; | |||
| export const ONE_HOUR_TO_MINUTES = 60; | |||
| export const END_HOUR = 24; | |||
| export const MAX_SELECTIONS = 3; | |||
There was a problem hiding this comment.
MAX_SELECTIONS 의 값을 constants 에 선언하여 import 하여 사용한 이유는 2가지입니다.
- 중복되어 사용되고 있었음
useFestivalSelection,AddFestival에서 중복사용되고 있었습니다. - 추가적으로 이번 2차스프린트에서
MAX_SELECTIONS의 값이 조정되는 걸로 알고 있어요 앞으로 추가적으로 변화가 생길 때 constants 에서 관리하면 사용되는 모든 부분의 코드를 일괄적으로 관리할 수 있다고 판단했어요
| const TOTAL_SELECTIONS = selectedFestivals.length + addedFestivals.length; | ||
|
|
||
| const handleAddClick = () => { | ||
| if (selectedFestivals.length + existingFestivals.length > MAX_SELECTIONS) { | ||
| if (TOTAL_SELECTIONS > MAX_SELECTIONS) { |
There was a problem hiding this comment.
기존의 조건문이 단순히 길이가 긴 부분도 있고, 직관적으로 어떤 필터링이 되는지 모호하다는 생각이 있어서
TOTAL_SELECTIONS 이라는 변수로 + 된 값들을 관리하여 if 문 조건부를 수정했어요
| const { mutate: addFestival } = useAddTimeTableFestival(() => { | ||
| navigate(routePath.TIME_TABLE_OUTLET); | ||
| }); | ||
| const TOTAL_SELECTIONS = selectedFestivals.length + addedFestivals.length; | ||
|
|
||
| const handleAddClick = () => { |
There was a problem hiding this comment.
개인적으로 page 단위에 UI 구성외에 함수가 선언되어있는 것을 선호하진 않는데, handleAddClick 의 관리가 어떻게 되었으면 하시는지 궁금합니다
There was a problem hiding this comment.
저는 직관적으로 함수가 선언되어있고, 해당 함수가 선언적으로 처리되고 있다면 괜찮다고 생각해요.
지욱님의 말도 일리가 있으니 커스텀훅으로 분리해도 상관없지만, 분리시에 handleAddClick함수가 useAddTimeTableFestival, useFestivalSelection 이 두 커스텀훅에 지나치게 의존되어 결합도가 높아질 것 같다는 우려가 생깁니다.
There was a problem hiding this comment.
저도 이 부분이 많이 고민되었어요 이미 handleAddClick 함수 자체가 다른 두 훅에 의존되어있기 때문에 분리하는 것에 대해 고민이 있었는데 현재상태를 유지해도 좋을 것 같아요
| onSuccess: async () => { | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: [...FESTIVAL_BUTTON_QUERY_KEY.ALL], | ||
| }); | ||
| if (onSuccessCallback) { | ||
| onSuccessCallback(); | ||
| } |
There was a problem hiding this comment.
기존 문제점 쿼리키 초기화 부문이 비동기적으로 실행되다 보니 navigate 가 너무 빨리 실행되면서 새로고침 없이 데이터가 업데이트 되지 않는 문제가 발생했어요.
그래서 async await 을 이용해서 festival 데이터를 초기화 한 후에 onSuccessCallback 이 실행되도록 로직을 수정하였어요
| onSuccess: async () => { | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: [...FESTIVAL_BUTTON_QUERY_KEY.ALL], | ||
| }); |
There was a problem hiding this comment.
| onSuccess: async () => { | |
| await queryClient.invalidateQueries({ | |
| queryKey: [...FESTIVAL_BUTTON_QUERY_KEY.ALL], | |
| }); | |
| onSuccess: () => { | |
| queryClient.removeQueries({ | |
| queryKey: [...FESTIVAL_BUTTON_QUERY_KEY.ALL], | |
| }); |
UI에 타임테이블이 즉각적으로 반영되지 않는 이유는 비동기처리에 관한 문제가 아니라 쿼리키와 캐싱된 데이터가 제대로 초기화 되지 않기 때문이라고 생각해요.
불필요한 async-await 대신에 removeQueries를 사용하면 기존 쿼리키&캐시를 완전히 제거하고 최신화된 데이터와 UI를 유지할 수 있을 것 같아요.
There was a problem hiding this comment.
두 가지 코드 모두 UI가 즉시 반영되지 않던 문제는 해결되는 것 같아요
하지만 removeQueries 를 사용해서 기존 쿼리 키와 캐시를 삭제하는 로직이
festival 을 추가 할 때 마다 이루어지는 것이 더 적절한지는 모르겠어요
async await + invalidateQueries 로 해결이 가능하다면 이 방법이 더 적절하지 않을까요..? 어렵네요
| const { mutate: addFestival } = useAddTimeTableFestival(() => { | ||
| navigate(routePath.TIME_TABLE_OUTLET); | ||
| }); | ||
| const TOTAL_SELECTIONS = selectedFestivals.length + addedFestivals.length; | ||
|
|
||
| const handleAddClick = () => { |
There was a problem hiding this comment.
저는 직관적으로 함수가 선언되어있고, 해당 함수가 선언적으로 처리되고 있다면 괜찮다고 생각해요.
지욱님의 말도 일리가 있으니 커스텀훅으로 분리해도 상관없지만, 분리시에 handleAddClick함수가 useAddTimeTableFestival, useFestivalSelection 이 두 커스텀훅에 지나치게 의존되어 결합도가 높아질 것 같다는 우려가 생깁니다.
| const TOTAL_SELECTIONS = selectedFestivals.length + addedFestivals.length; | ||
|
|
||
| const handleAddClick = () => { | ||
| if (selectedFestivals.length + existingFestivals.length > MAX_SELECTIONS) { | ||
| if (TOTAL_SELECTIONS > MAX_SELECTIONS) { |
📌 Summary
📚 Tasks
👀 To Reviewer
기존 문제점에 대한 사항은 연결 된 이슈에 작성해두었습니다 ! 확인해주세요 !
❗ 변경사항에 관련 된 설명을 리뷰로 달아두었습니다 확인부탁드려요!
📸 Screenshot
before
2025-02-18.035407.mp4
after
2025-02-18.035537.mp4
개선 내용 주요 내용