-
Notifications
You must be signed in to change notification settings - Fork 0
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 : useMutation으로 api 호출 방식 통일 #299
base: develop
Are you sure you want to change the base?
Conversation
1932b02
to
b3e7f7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마참내......... 코멘트 쪼끔 남겨놨습니다 확인해주세용. 고생하셨습니다!! ^ㅅ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tanstack query를 적용하면서 apis/authVerification
파일 내에 api 함수와 useMutation이 같이 위치하게 됐는데요.
useMutation은 home/hooks
로 분리하는 게 폴더 구조상 적절해 보입니다!
아니면 같은 파일 내에 위치시키신 이유가 따로 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 다른 API 호출 코드들과 통일성을 위해 apis
폴더 아래에는 axios를 이용한 API 호출 코드만 남기고 useMutation()
을 반환하는 커스텀 훅은 home/hooks
아래로 위치시키는게 좋을 것 같습니다!
추가적으로 postAuthVerificationEmail()
, getAuthVerificationCheck()
함수도 별도의 파일로 분리하는게 좋을 것 같네요.
GetAuthVerificationCheckResponse, | ||
PostAuthVerificationEmailResponse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인해보니까 이 두 타입은 이제 사용하는 곳이 없던데, 한번 확인해보시고 이번 PR에서 멸종 ㄱㄱ 어때요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#300 과 작업 부분이 많이 겹치는 거 같은데 conflict 해결 가능하실까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다👏👏
추가적으로 #262 에서 언급한 postAuthSignUp()
부분도 tanstack query를 사용해서 API 호출 상태를 관리하면 좋을 것 같네요!
//이전과 같은 이메일일 경우 errorBoundary로 가지 않고 에러 처리 | ||
if (error.response?.status === 400) return false; | ||
// 나머지는 errorBoundary로 처리 | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//이전과 같은 이메일일 경우 errorBoundary로 가지 않고 에러 처리 | |
if (error.response?.status === 400) return false; | |
// 나머지는 errorBoundary로 처리 | |
return true; | |
return error.response?.status !== 400 |
으로 줄일 수도 있을 것 같네요!
setAuthed(false); | ||
setError('인증 메일 재전송에 실패했습니다.'); | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
onSettled: () => { | |
setEmailSending(false); | |
resetTimer(); | |
}, |
다른 콜백들과의 일관성을 위해 mutation이 성공하거나 실패했을 때 호출되는 onSettled()
콜백에서 setEmailSending()
, resetTime()
를 호출하면 어떨까요?
추가적으로 onSettled()
콜백을 사용하면 동기적으로 mutation 결과를 기다릴 필요가 없으니 mutateAsync()
도 mutate()
로 바꿀 수 있을 것 같네요
setEmailError(res.error?.response?.data.message || '이메일을 다시 확인해주세요.'); | ||
} | ||
|
||
await postAuthVerificationEmailMutation.mutateAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onSuccess()
, onError()
함수는 mutation 결과에 따라 tanstack query가 동기적으로 호출해주니 mutateAsync()
를 사용할 필요가 없을 것 같아요!
const { | ||
handleSubmit, | ||
formState: { isSubmitting }, | ||
} = useForm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-hook-form으로 form 제출 상태를 관리하는게 좋네요👍
email error도 react-hook-form으로 관리할 수 있을 것 같은데 나중에 해보는 걸루..
1️⃣ 어떤 작업을 했나요? (Summary)
기존 코드에 영향을 미치지 않는 변경사항
기존 코드에 영향을 미치는 변경사항
useMutation을 통해 api 호출 방식을 통일 했습니다.
juun이 얘기한 대로
try catch를 적용하면 promise가 reject되지 않기 때문에 errorBoundary를 랜더링 시킬 수 없습니다.
수정된 코드입니다.
throwOnError를 통해 처리 가능한 에러 status 이외의 에러는 상위 컴포넌트인 errorBoundary를 랜더링 시키도록 했습니다.
버그 픽스
2️⃣ 알아두시면 좋아요!
emailForm과 emailAuth에는 중복 제출 방지를 위한 버튼 disable 로직이 존재합니다.
void인 mutate를 적용하면 await에서 실행 흐름을 기다리지 않기 때문에
isSubmitting같이 제출 상태를 체크하는 부분이 제대로 작동하지 않습니다.
때문에 mutateAsync를 통해 promise를 반환하도록 했습니다.
근데 throwOnError에서 status 잘 쓴건지 모르겠네요
혹시 throw하면 안되는 혹은 throw 해야하는(errorBoundary 띄워야 하는) status가 있는지 한번 봐주시면 감사하겠습니다
3️⃣ 추후 작업
리뷰 환영합니다.
4️⃣ 체크리스트 (Checklist)
main
브랜치의 최신 코드를pull
받았나요?