-
Notifications
You must be signed in to change notification settings - Fork 79
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
[이승현] sprint12 #738
The head ref may contain hidden characters: "React-\uC774\uC2B9\uD604-Sprint12"
[이승현] sprint12 #738
Conversation
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에는 하신 부분만큼 설명을 적어주세요.
코드가 미완성인것은 괜찮지만, PR까지 미완성일 필요는 없습니다.
언제나 리뷰어에게 최대한 많은 정보를 제공할 수 있도록 노력하시면 좋을 것 같습니다. -
PR을 올릴 때는 테스트를 위해 log를 찍어보신 코드들은 지워주세요
중요한 로직들 사이에 log가 있다면 리뷰어가 리뷰를 하는데 방해가 될 수 있습니다.
시간이 부족하여 마음이 급하셨던 것 같습니다.
주어진 시간이 끝났을 때 어떤것에 집중해야 할지 잘 선택하는 것도 추후 업무를 진행함에 있어서도 큰 도움이 됩니다.
이번 미션도 수고많으셨습니다 👍
export interface PostAuthRefreshToken { | ||
accessToken: string; | ||
} |
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.
해당 타입이 response type으로써 사용되는 것 같은데 다른 type처럼 Response라는걸 나타내주면 좋을 것 같습니다.
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.
감사합니다
} | ||
}; | ||
|
||
export const postAuthRefreshToken = async () => { |
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.
어떤 요청으로 보내는지는 함수를 사용하는 곳에서 알아야할 관심사가 아닙니다.
그저 어떤걸 수행하는지만 잘 나타내주면 될 것 같습니다.
이 경우에는 RefreshAuthToken 정도면 괜찮다고 생각합니다.
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.
감사합니다!
if ( | ||
error.response?.status === 401 && | ||
originalRequest && | ||
!originalRequest._retry | ||
) { | ||
originalRequest._retry = true; | ||
try { | ||
// 토큰 재발급 | ||
const response = await postAuthRefreshToken(); | ||
|
||
// 토큰 갱신 | ||
useUserStore.setState({ | ||
user: { accessToken: response.accessToken }, | ||
}); | ||
|
||
// 헤더에 토큰 추가 | ||
originalRequest.headers.Authorization = `Bearer ${response.accessToken}`; | ||
|
||
// 재시도 | ||
return instance(originalRequest); |
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.
재시도 처리를 굳이 request안에 flag값을 넣어서 다음 요청에 보내서 처리하지않고
한번의 interceptor내에서 다 처리후 내보낼 수 있을 것 같은데 그렇게 시도해보시면 좋을 것 같습니다.
지금 코드는 파악하기에 어려움이 꽤나 많은 것 같습니다
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를 로그인 과정에 합친다는 글을 보고 따라하다가 남겨진 코드입니다.. 변경 시도해보겠습니다..
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.
위의 글을 보고 참고했는데 여기에 한번 지식이 박혀서 수정이 조금 힘든 것 같아요 혹시 대략적으로 설명해주실 수 있으신가요? 코드잇에서 배운 코드에도 flag가 담긴 코드였어서 헷갈리네요
완성하고 PR을 적고 싶었는데 제가 부족해서 완성하지 못했습니다.. 죄송합니다. 남은 부분 더 열심히 해보겠습니다.. 아직도 안되는 부분이 있네요..ㅠㅠ... |
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게