-
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
로그인 상태라면 인증 헤더 넣기 #152
로그인 상태라면 인증 헤더 넣기 #152
Conversation
class AuthRepositoryImpl @Inject constructor( | ||
private val userApi: UserApi, | ||
private val preferenceDataStore: DataStore<Preferences> | ||
private val tokenStore: TokenStore, | ||
private val dataStore: DataStore<Preferences> | ||
) : AuthRepository { |
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.
리팩터링 과정에서 data store 로직을 data source에 넘겼다가 다시 돌려놔서 이름이 바뀌었지만, 기본적으로 data store에 저장하고 가져오는 로직은 동일해
class TokenStore @Inject constructor() { | ||
|
||
var token: String = "" | ||
private set | ||
|
||
fun updateToken(newToken: String) { | ||
token = newToken | ||
} |
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.
interceptor에서 토큰을 넣어줘야 하는데, data store에서 직접 불러오려고 하면 suspend 함수라 runBlocking을 쓰는 것밖에는 답이 없는 거 같더라구. 그래서 이건 찾아보진 않았는데, 로직상으로 말이 되는 거 같아서 적용해 봤어.
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.
okhttp 는 메인스레드가 아니라서 runBlocking 써도 무방하다고 하는데 문서, 그래서 runBlocking 써도 괜찮은 것 같아용! 지금 로직은 처음 본사람이 코드 이해하기가 조금 어려울 것 같다는 생각이 들었어요 ! Auth Repository 에서 TokenStore랑 DataStore 를 둘다 주입 받는게 어색한 것 같아유
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.
저도 유라님과 비슷하게 runBlocking 쓰고, 저 좋은 방법이 있으면 그때 수정하면 될것 같습니다
class GetIdTokenUseCase @Inject constructor( | ||
class AutomaticallyLoginUseCase @Inject constructor( |
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.
이름 바꾼건 좋은것 같아용!! 근데 동사를 앞에 넣어서 LoginAutomaticallyUseCase 어떤가요?
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.
UseCase는 앞에 동사가 오는걸 원칙으로 하면 일관성 있지 않을까요???
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.
한 번만 봐줘 ㅠㅠㅠ
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.
봐줬다😏
@@ -41,6 +43,9 @@ class NicknameFragment : BaseFragment<FragmentNicknameBinding>(R.layout.fragment | |||
viewModel.events.collect { event -> | |||
when (event) { | |||
is NicknameEvent.NavigateToHome -> { | |||
val intent = Intent() | |||
intent.component = ComponentName("com.ohdodok.catchytape", "com.ohdodok.catchytape.MainActivity") |
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.
이것도 임시 코드
import javax.inject.Qualifier | ||
|
||
@Qualifier | ||
annotation class AuthInterceptor |
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.
파일명 AuthInterceptor 로 하는 것이 어떨까용
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.
음 원래는 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.
반영했어~
class GetIdTokenUseCase @Inject constructor( | ||
class AutomaticallyLoginUseCase @Inject constructor( |
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.
이름 바꾼건 좋은것 같아용!! 근데 동사를 앞에 넣어서 LoginAutomaticallyUseCase 어떤가요?
override suspend fun tryLoginAutomatically(): Boolean { | ||
val accessToken = | ||
dataStore.data.map { preferences -> preferences[accessTokenKey] ?: "" }.first() | ||
|
||
if (accessToken.isBlank()) return false | ||
|
||
return userApi.verify("Bearer $accessToken").isSuccessful | ||
} |
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.
나중에 수정되는건지 모르겠는데,, 지금 같은 상황에서는 accessToken이 이미 저장된 상태에서 앱을 실행하면 header 에 token을 담아보낼 수 없을 것 같아요! 지금 로직대로 하려면 여기서도 tokenStore.updateToken이 필요할 것 같다는 생각이 드네용
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.
고생했다리~~👻
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.
고생하셨습니다~
Issue
Overview