Skip to content

Conversation

@jiyong1
Copy link
Member

@jiyong1 jiyong1 commented Dec 30, 2021

개요

새로 고침 시 로그인이 풀리는 버그를 수정하였습니다.
로직이 수정되어서 그에 따른 몇 가지 수정사항이 있습니다.

이슈 번호

변경사항

  • login을 진행할 경우 localStorage에 userId 를 저장합니다.
  • logout을 진행할 경우 userId 를 제거합니다.
  • App 컴포넌트가 마운트 될 때 autoLogin을 진행합니다. (localStorage에 userId 키의 값이 존재할 경우)
  • Signin page의 test 코드에 RecoilRoot 가 존재하지 않아 실패를 하여 RecoilRoot 를 추가하였습니다.

특이사항

  • autoLogin을 실패할 경우에 다시 로그인 해달라는 에러 핸들링을 진행하면 좋을 것 같습니다.

@jiyong1 jiyong1 added bug Something isn't working refactor Code refactoring labels Dec 30, 2021
@jiyong1 jiyong1 changed the title 새로 고침 시 로그인이 풀리는 버그 수정과 그에 따른 hook과 컴포넌트 로직 수정 새로 고침 시 로그인이 풀리는 버그 수정과 그에 따른 hook과 컴포넌트 로직 수정 (after #69 merged) Dec 30, 2021
Copy link
Member

@pumpkiinbell pumpkiinbell left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~

Comment on lines +22 to +26
/*
따로 에러 핸들링 해야할 것으로 보입니다.
refresh를 실패했다는 것은 곧 refresh token이 만료되었을 경우가 대부분일테니
이에 따라서 다시 로그인 해달라는 창을 보여주는 것이 어떨지.. toastify 처럼..
*/
Copy link
Member

@pumpkiinbell pumpkiinbell Dec 30, 2021

Choose a reason for hiding this comment

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

개인적으로 저는 refresh token 이 만료되었을 때 모달같은 알림을 띄우는 것을 선호하지 않습니다

그린카라는 앱을 이용하다가 언급해주신 경험을 했는데요,
앱에 오랜만에 들어갔을 때 아무 맥락없이 토큰이 만료되어 ... 같은 알림을 보면 썩 유쾌하진 않더라고요
유저가 비로그인 상태를 인지하면 자연스레 로그인을 시도하지 않을까... 하는 생각이 듭니다!

개인적인 생각이라 다른 방향으로 결정되어도 괜찮습니다!!
다만 굳이 로그인 창을 띄운다면 window.alert API 같이 확인 버튼을 눌러야 사라지는 모달보단 말씀하신 toastify 처럼 시간이 지나면 자연스레 사라지는 모달이 더 좋겠네요 => 저희 useSnackbar 사용하는 것도 좋아보입니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

그 훅이 뭐하는 건지.. 잘 몰라서요.. 어떤거죠 ?!

Copy link
Member

Choose a reason for hiding this comment

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

이 영상 5초 부근 인데... 훔 하단 중간 부분에 보이게 된다는 단점이 있긴 하네요...

Copy link
Member

Choose a reason for hiding this comment

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

로그인 필요 여부는 반드시 사용자에게 보여줘야 하는 부분이지 않을까 싶어요. refresh token이 만료되게 되면 잘 사용하다가 갑자기 새로고침을 했더니 로그아웃이 되는 상황이 발생할 수 있고 이에 대한 피드백이 반드시 사용자에게 필요하다고 생각합니다.

요거에 대해서는 useAutoLogin 외에도 일반 API 요청을 할 때도 처리를 해줘야 하는 부분이라 다른 이슈에서 작업하는 게 좋지 않을까 싶습니다.

Comment on lines 53 to 58
setUserState(null);
authApi.logout().catch(() => {
/* 로그아웃의 경우, 별도의 에러 처리를 하지 않음 */
});

localStorage.removeItem('userId');

authApi.logout();
};
Copy link
Member

Choose a reason for hiding this comment

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

authApi.logout() 뒤의 .catch()를 지우신 이유가 있으실까요?? api 요청의 경우, 실패했을 때 Error를 throw하기 때문에 try/catch가 필수적이지만 로그아웃 동작의 경우 api 실패 여부와 상관없이 성공해야 하므로 빈 catch 문을 적어줬던 거였습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아 그런 의도 였군요.. 허허 다시 수정해놓겠습니다 ㅎㅎ

Copy link
Member

@Seogeurim Seogeurim left a comment

Choose a reason for hiding this comment

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

훨씬 좋네요 !!!!

  • 새로고침 시 로그인 상태 잘 유지되네요!! 그런데 서비스 페이지에서 새로고침하면 홈페이지로 가던데, 이 부분 이유를 아시나용?
  • useAuth의 isLoading을 로그인페이지에서 안 쓰고 있는거죠? 이 부분 로딩중일 때 로더 하나 표시해주면 더 자연스러울 것 같은데 어떤가요? 제가 만들어놓은 Loader가... 앗 아직 merge가 안 됐으려나 ..!
  • 로그아웃했을 때도 스낵바 하나 띄워주면 훨씬 자연스러울 것 같은데 어떤가요?


setIsLoading(false);

throw e;
Copy link
Member

Choose a reason for hiding this comment

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

에러 throw를 할 필요가 있을까욥??

@hseoy
Copy link
Member

hseoy commented Jan 8, 2022

새로고침 시 로그인 상태 잘 유지되네요!! 그런데 서비스 페이지에서 새로고침하면 홈페이지로 가던데, 이 부분 이유를 아시나용?

요건,,,, PrivateRoute의 동작 때문인 것 같네요. 새로고침 시 당장은 사용자 상태가 없기 때문에 home 페이지로 이동을 하고 home 페이지 내에서 사용자 상태가 새로 세팅이 되는,,,, 그런 상태이지 않을까 싶습니다.

요건 제가 나중에 확인해서 수정해볼게요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working refactor Code refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants