Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

2zerozu
Copy link
Contributor

@2zerozu 2zerozu commented Apr 9, 2023

관련 이슈

작업한 내용

  • 로그인 리팩토링
    • 라이브데이터 -> 플로우
    • 카카오로그인콜백 캡슐화
    • 불필요한 함수, 변수, 로컬에 토큰 저장하는 함수(InitToken) 제거

PR 포인트

  • 잘 한 걸까요..

@2zerozu 2zerozu requested a review from a team April 9, 2023 08:06
@2zerozu 2zerozu self-assigned this Apr 9, 2023
@2zerozu 2zerozu added 영주🐼 영주가 작업함! refactor 내부 로직은 변경 하지 않고 기존의 코드를 개선하는 리팩토링 시 Pull Request🔥 풀리퀘 날림! labels Apr 9, 2023
@2zerozu 2zerozu added this to the Hous 3차 릴리즈 milestone Apr 9, 2023
Copy link
Member

@KWY0218 KWY0218 left a 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 토큰을 불러오는 중인가요??

Comment on lines 83 to 84
binding.btnLoginKakao.setOnSingleClickListener {
kakaoLoginService.startKakaoLogin(loginViewModel.kakaoLoginCallback)
Copy link
Member

Choose a reason for hiding this comment

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

KakaoLoginService 를 필드에서 주입받아 사용 중인데
저희가 사용하는 구조랑 어색한 것 같습니다..!

Comment on lines +41 to +42
private val _isMultipleAccess = MutableStateFlow(false)
val isMiultipleAccess = _isMultipleAccess.asStateFlow()
Copy link
Member

Choose a reason for hiding this comment

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

단발성 이벤트인 것 같은데 stateflow를 사용하는 이유가 있을까요?

Copy link
Contributor Author

@2zerozu 2zerozu Apr 9, 2023

Choose a reason for hiding this comment

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

단발성 이벤트인 것 같은데 stateflow를 사용하는 이유가 있을까요?

다이얼로그에서 취소하고, 다시 강제로그인을 재요청할 수 있어서 상태 저장하려고 stateFlow 사용했숨다

Copy link
Member

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랑 비슷해보여서 바꾸는 것이 나을 것 같네요.

위에 상황 한번 해보고 후기 알려줘연~ 😄

Copy link
Contributor Author

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인 줄 알았구만요~ 오케이

Comment on lines +41 to +42
private val _isMultipleAccess = MutableStateFlow(false)
val isMiultipleAccess = _isMultipleAccess.asStateFlow()
Copy link
Member

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랑 비슷해보여서 바꾸는 것이 나을 것 같네요.

위에 상황 한번 해보고 후기 알려줘연~ 😄

Comment on lines 88 to 90
private fun startKakaoLogin() {
kakaoLoginService.startKakaoLogin(loginViewModel.kakaoLoginCallback)
}
Copy link
Member

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 모듈과 분리해보는 것이 어떤가 합니다..!

Copy link
Contributor Author

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 모듈과 분리해보는 것이 어떤가 합니다..!

아으~ 확인이요

Copy link
Contributor

Choose a reason for hiding this comment

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

@KWY0218 @2zerozu
제가 알기로는 KakaoLoginService에서 Context가 필요한걸로 알고있는데 모듈 분리하면 data모듈에서 context 사용이 어려울 것 같은데 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

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

@yjooooo

Kakao 로그인할 때 Activity Context가 필수적이니까 Kakao 로그인에 한에서 매개변수로 context를 전달하면 어떨까 합니다..!

Activity --(context)--> viewmodel --(context)--> domain --(context)--> data

context가 이런식으로 이동하겠네요..!

이렇게 생각한 이유는 나중에 feature 모듈로 분리를 하면 app 모듈과 떨어지게 되고, 해당 feature 모듈의 종속엔 data 모듈이 포함되지 않기 때문에 KakaoService를 주입 받을 때 문제가 생길 수 있다고 생각 들기 때문입니다..!

Copy link
Contributor

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 내부에 선언하거나 주입받는게 아니라면 메모리릭 문제에서는 벗어날 수도 있지 않을까 생각이 드네요...! (확인은 필요한 것 같아요!)

다른 분들의 의견도 궁금합니다!

Copy link
Member

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에서 응답이 없는 걸 보니
메모리 누수가 없는 것 같아 보이긴 하는데

이 상황이 근거가 될진 확신이 안섭니다.. 😂😂

Copy link
Contributor

@yjooooo yjooooo May 18, 2023

Choose a reason for hiding this comment

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

@KWY0218 카카오 로그인 이후 다음 화면으로 넘어갈 때도 릭카나리에서 응답이 없다면 메모리릭은 없는 것으로 봐도 되지 않을까요?
그러면 해당 구조로 가도 괜찮을 것 같아요!

@2zerozu 2zerozu marked this pull request as draft June 1, 2023 05:02
@2zerozu 2zerozu closed this Nov 23, 2023
@murjune murjune deleted the feature/#203-refact-login branch March 10, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request🔥 풀리퀘 날림! refactor 내부 로직은 변경 하지 않고 기존의 코드를 개선하는 리팩토링 시 영주🐼 영주가 작업함!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[refact] 카카오 로그인 flow로 리팩토링
3 participants