-
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
#203 [refact] 로그인 리팩토링 #204
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.
작업하시느라 고생하셨습니다 ~ ! 코멘트 확인해주세요~ 😄
만약 회원 정보가 없다면 userInput 뷰로 넘어갈 텐데,
이곳에서 사용자 정보를 넣고 body값을 보낼 때
fcm 토큰하고 kakao 토큰을 넣어서 보내는 것으로 압니다.
그래서 login 뷰에서 만약 회원 정보가 없으면 kakao 토큰하고 fcm 토큰을 잠시 저장해 뒀다가 user Input 뷰에서 사용하는 것으로 알고 있었는데,
user input 뷰에서 따로 kakao 토큰과 fcm 토큰을 불러오는 중인가요??
binding.btnLoginKakao.setOnSingleClickListener { | ||
kakaoLoginService.startKakaoLogin(loginViewModel.kakaoLoginCallback) |
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.
KakaoLoginService
를 필드에서 주입받아 사용 중인데
저희가 사용하는 구조랑 어색한 것 같습니다..!
private val _isMultipleAccess = MutableStateFlow(false) | ||
val isMiultipleAccess = _isMultipleAccess.asStateFlow() |
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.
단발성 이벤트인 것 같은데 stateflow를 사용하는 이유가 있을까요?
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.
단발성 이벤트인 것 같은데 stateflow를 사용하는 이유가 있을까요?
다이얼로그에서 취소하고, 다시 강제로그인을 재요청할 수 있어서 상태 저장하려고 stateFlow 사용했숨다
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.
오히려 위와 같은 상황이라면 더더욱 sharedFlow를 사용해야되지 않을까요?
[isMutipleAccess : false] -> 첫 강제 로그인 [isMutipleAccess : true] -> 취소 [isMutipleAccess : true] -> 다시 클릭 true 방출
stateFlow 는 같은 값은 수집 안하는 특성이 있어서 오히려 다이얼로그가 안뜰 것 같은 생각이 듭니다..!
그리고 사용 방식도 sharedFlow랑 비슷해보여서 바꾸는 것이 나을 것 같네요.
위에 상황 한번 해보고 후기 알려줘연~ 😄
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.
오히려 위와 같은 상황이라면 더더욱 sharedFlow를 사용해야되지 않을까요?
[isMutipleAccess : false] -> 첫 강제 로그인 [isMutipleAccess : true] -> 취소 [isMutipleAccess : true] -> 다시 클릭 true 방출
stateFlow 는 같은 값은 수집 안하는 특성이 있어서 오히려 다이얼로그가 안뜰 것 같은 생각이 듭니다..!
그리고 사용 방식도 sharedFlow랑 비슷해보여서 바꾸는 것이 나을 것 같네요.
위에 상황 한번 해보고 후기 알려줘연~ 😄
아 같은 값 수집 안 하는 게 sharedFlow인 줄 알았구만요~ 오케이
private val _isMultipleAccess = MutableStateFlow(false) | ||
val isMiultipleAccess = _isMultipleAccess.asStateFlow() |
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.
오히려 위와 같은 상황이라면 더더욱 sharedFlow를 사용해야되지 않을까요?
[isMutipleAccess : false] -> 첫 강제 로그인 [isMutipleAccess : true] -> 취소 [isMutipleAccess : true] -> 다시 클릭 true 방출
stateFlow 는 같은 값은 수집 안하는 특성이 있어서 오히려 다이얼로그가 안뜰 것 같은 생각이 듭니다..!
그리고 사용 방식도 sharedFlow랑 비슷해보여서 바꾸는 것이 나을 것 같네요.
위에 상황 한번 해보고 후기 알려줘연~ 😄
private fun startKakaoLogin() { | ||
kakaoLoginService.startKakaoLogin(loginViewModel.kakaoLoginCallback) | ||
} |
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.
음.. 이렇게 함수로 빼는 것이 아닌 지금 우리 프로젝트 구조를 보면
UI <--> Domain <--> Data
이런 구조로 유지 중입니다.
data 모듈에 있는 service를 ui 모듈에서 필드로 주입하는 것이
data 모듈을 ui 모듈과 분리해보는 것이 어떤가 합니다..!
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.
음.. 이렇게 함수로 빼는 것이 아닌 지금 우리 프로젝트 구조를 보면
UI <--> Domain <--> Data
이런 구조로 유지 중입니다.
data 모듈에 있는 service를 ui 모듈에서 필드로 주입하는 것이
data 모듈을 ui 모듈과 분리해보는 것이 어떤가 합니다..!
아으~ 확인이요
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.
Kakao 로그인할 때 Activity Context가 필수적이니까 Kakao 로그인에 한에서 매개변수로 context를 전달하면 어떨까 합니다..!
Activity --(context)--> viewmodel --(context)--> domain --(context)--> data
context가 이런식으로 이동하겠네요..!
이렇게 생각한 이유는 나중에 feature 모듈로 분리를 하면 app 모듈과 떨어지게 되고, 해당 feature 모듈의 종속엔 data 모듈이 포함되지 않기 때문에 KakaoService를 주입 받을 때 문제가 생길 수 있다고 생각 들기 때문입니다..!
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.
@KWY0218 @2zerozu @murjune
공식 문서 : [ViewModel은 뷰, Lifecycle 또는 활동 컨텍스트 참조를 포함하는 클래스를 참조해서는 안 됩니다.]
만약 분리를 하게 된다면 위의 말에 따라서 ViewModel 내부에 context가 존재하게는 하지말고, 메소드의 매개변수로 넘겨야겠네요..
제가 망설였던 부분은 data모듈에 저희가 넘기는 변수들은 뷰의 정보를 담고 있지는 않은데(보통 데이터 그 자체의 값이니까, ex. 이름, 생일, 룸코드 등등), context는 뷰의 정보들이 담겨있는 부분인데 넘기는 게 맞나? 하는 생각이 들었어요.
혹시 이 부분에 대해서는 어떻게 생각하시나요?
더불어 context는 메모리릭이 발생할 수 있는 부분이기도 하니까 더 조심스러워지는 것 같아요...
ViewModel 내부에 선언하거나 주입받는게 아니라면 메모리릭 문제에서는 벗어날 수도 있지 않을까 생각이 드네요...! (확인은 필요한 것 같아요!)
다른 분들의 의견도 궁금합니다!
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.
KakaoTalk_20230516_134740933.mp4
현재 제가 하고 있는 졸작에서
Activity --(context)--> viewmodel --(context)--> domain --(context)--> data
이런 형식으로 카카오 로그인을 구현한 상태입니다.
로그인을 누르고 카카오 토큰을 받기 전 배경화면으로 갔을 때 leak canary에서 응답이 없는 걸 보니
메모리 누수가 없는 것 같아 보이긴 하는데
이 상황이 근거가 될진 확신이 안섭니다.. 😂😂
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.
@KWY0218 카카오 로그인 이후 다음 화면으로 넘어갈 때도 릭카나리에서 응답이 없다면 메모리릭은 없는 것으로 봐도 되지 않을까요?
그러면 해당 구조로 가도 괜찮을 것 같아요!
관련 이슈
작업한 내용
PR 포인트