Open
Conversation
로그인 후, 없는 route에 대해서 /로 리다이렉트 시킴
비밀번호 재설정 신청 후, 일정시간만 재설정 할 수 있는 페이지 이동 유효함
kimyoungyin
approved these changes
Sep 7, 2022
Comment on lines
+96
to
101
| dispatch(checkCurrentURL({ code, username })) | ||
| .unwrap() | ||
| .catch(() => { | ||
| history.push("/error"); | ||
| }); | ||
| }, []); |
Collaborator
There was a problem hiding this comment.
- thunk나 slice에서 라우트 이동이 불가능해서 이렇게 컴포넌트 내부에서 처리하도록 한 걸까요?(
unwrap())
-> 내부에서useEffect로 위 동작을 처리한 경우 컴포넌트가 마운트 된 후에 코드 유효성 검사를 하게 되는데, 이 때 코드가 유효하지 않을 때 의미 없는 렌더링이 발생할 것 같아요. 굳이 그럴 필요 없이 상위 컴포넌트에서 로딩 처리를 위임하여 재설정 코드 유효성이 통과된다면 하위 컴포넌트인ResetPasswordForm을 렌더링하도록 하는 건 어떨까요? - deps에
dispatch,code,username을 넣지 않은 이유가 있을까요? - thunk별 수행하는 동작이:
checkCurrentURL: 재설정 하기 위해 주어진 코드가 유효한지 확인,resetPassword: 비밀번호 재 설정맞을까요?
Member
Author
There was a problem hiding this comment.
하나씩 답변드릴게용!
deps에 dispatch, code, username을 넣지 않은 이유가 있을까요?
code, username은 백엔드가 넘겨준 url에서 추출해오기 때문에, 최초 렌더링 이후 변경될 가능성이 없기때문에 deps에 넣을 필요를 못 느꼈습니다.
dispatch는 다른 곳에서도 계속 안넣어왔던 거 같은데, 영인님은 dispatch를 넣는 이유가 있으신가요?
thunk별 수행하는 동작이: checkCurrentURL: 재설정 하기 위해 주어진 코드가 유효한지 확인, resetPassword: 비밀번호 재 설정 맞을까요?
넵 맞습니다!
thunk나 slice에서 라우트 이동이 불가능해서 이렇게 컴포넌트 내부에서 처리하도록 한 걸까요?(unwrap())
네 맞아요. 이 부분 꽤나 고민했었는데..! 꼼꼼히 봐주셔서 감사합니다!
내부에서 useEffect로 위 동작을 처리한 경우 컴포넌트가 마운트 된 후에 코드 유효성 검사를 하게 되는데, 이 때 코드가 유효하지 않을 때 의미 없는 렌더링이 발생할 것 같아요. 굳이 그럴 필요 없이 상위 컴포넌트에서 로딩 처리를 위임하여 재설정 코드 유효성이 통과된다면 하위 컴포넌트인 ResetPasswordForm을 렌더링하도록 하는 건 어떨까요?
- 아 그렇네요. 의미없는 렌더링..! useEffect가 렌더링 전에 실행되는 줄 알았는데, 렌더링 이후에 실행되는 거더라구요. 영인님 의견이 좋은 거 같습니다. 그렇게 수정해서 커밋해놓을 게요!
Collaborator
There was a problem hiding this comment.
- https://okky.kr/articles/957064: dispatch를 deps에 넣을지에 대한 내용인데, 전 불변성이 유지되긴 하더라도 넣어주는 편이었습니다!
- unwrap(): 확인해보니
authSlice에서 checkCurrentURL가 Return하는 promise에 대한 state 변화는 없네요! 저는 얼마 전까지도 몰랐던 건데,unwrap()처리를 하더라도addCase에 등록된 해당 thunk extraReducer는 호출되더라구요.. 참고하세요!
렌더링 관련 커밋 사항 발생하면 리뷰 하도록 하겠습니다!
Comment on lines
64
to
+73
| void, | ||
| { code: string; username: string } | ||
| >("auth/checkResetPassword", async (payload, ThunkOptions) => { | ||
| try { | ||
| const config = { | ||
| params: { | ||
| code: payload.code, | ||
| username: payload.username, | ||
| }, | ||
| }; | ||
| const { data } = await customAxios.get( | ||
| `/accounts/password/reset`, | ||
| config, | ||
| ); | ||
| console.log(data); | ||
| } catch (error) { | ||
| throw ThunkOptions.rejectWithValue(error); | ||
| } | ||
| >("auth/checkResetPassword", async (payload) => { | ||
| const config = { | ||
| params: { | ||
| code: payload.code, | ||
| username: payload.username, | ||
| }, | ||
| }; | ||
| await customAxios.get(`/accounts/password/reset`, config); |
Collaborator
There was a problem hiding this comment.
에러가 나지 않고 200번 대 response가 오더라도 "올바르지�않은 비밀번호 재설정 코드입니다." 인 경우에는 에러 처리를 해줘야 하지 않을까요?
Member
Author
There was a problem hiding this comment.
dispatch(checkCurrentURL({ code, username }))
.unwrap()
.catch(() => {
history.push("/error");
});
}, []);여기 catch문에서 처리될 거라고 생각했는데, 200번대였군요..! 이 부분도 에러처리 반영하겠습니다. 감사합니다
Member
Author
|
@kimyoungyin 리뷰 감사합니다! |
Collaborator
|
404 페이지 만들기 전까지는 home으로 redirect 시켜주는 것으로 유지하면 좋을 것 같아요 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🧊 관련이슈
#88
👨💻 작업사항
메일 > 비밀번호 재설정, 코드 유효하지 않을 때 에러페이지로 대응
메일 > 비밀번호 재설정 > 완료 후, url을 "/"로 이동
💬 참고해주세요
로그인 후, 없는 라우터에 대해 redirect / 처리
로그인 유무와 상관없이 정의하지 않은 라우터에 대해서 리다이렉트 루트로 처리했었는데,
로그인 후에는 리다이렉트 루트처리할 필요가 없을 거 같다고 해서, 해당 코드를 지웠습니다.
그런데, 없는 라우터에 대해 home으로 리다이렉트 할 경우가 생겨서 이번에 추가했는데, 어떻게 생각하시나요??
저희가 처리하지 않은 페이지에 대해서는 home으로 돌아가게 됩니다!