-
Notifications
You must be signed in to change notification settings - Fork 9
[FEAT/#1457] 앱잼탬프 미션 상세 api 연결 #1465
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
[FEAT/#1457] 앱잼탬프 미션 상세 api 연결 #1465
Conversation
…amp-mission-detail-api
1971123-seongmin
left a comment
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.
너무 잘 해주셨네요. 정말 고생하셨습니다 🤗🤗
| } | ||
|
|
||
| LaunchedEffect(viewModel.sideEffect, lifecycleOwner) { | ||
| viewModel.sideEffect.flowWithLifecycle(lifecycle = lifecycleOwner.lifecycle) |
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.
여기에서 repeatOnLifecycle 말고 flowWithLifecycle를 사용하신 이유를 설명해주실 수 있으신가요?? (사이드 이펙트가 한 개라서??)
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.
sideEffect가 하나밖에 없고 실행되면 바로 화면 전환이 이루어져서 큰 고민을 하고 작성한 코드는 아닌데요😅 지금 다시 생각해보자면 flowWithLifecycle도 현재 상황에서는 큰 문제가 없겠지만 확장성과 안정성을 고려하면 언급해주신 repeatOnLifecycle을 사용하는게 더 나은 방법일 것 같다는 생각이 드네요! 요건 repeatOnLifecycle로 수정하도록 하겠습니다 말씀해주셔서 감사해요😊
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.
제가 알기로는 flowWithLifecycle의 내부 구현 코드에
@OptIn(ExperimentalCoroutinesApi::class)
public fun <T> Flow<T>.flowWithLifecycle(
lifecycle: Lifecycle,
minActiveState: Lifecycle.State = Lifecycle.State.STARTED
): Flow<T> = callbackFlow {
lifecycle.repeatOnLifecycle(minActiveState) { this@flowWithLifecycle.collect { send(it) } }
close()
}이미 repeatOnLifecycle를 사용하고 있어서 동작 방식은 동일하지 않나요?
오히려 단일 스트림이라 체이닝 형식이 더 낫지않을까요?
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.
위에서 말해주신 것 처럼 내부적으로 repeatOnLifecycle 되어 있기 때문에 사이드 이펙트가 한개만 있다면 flowWithLifecycle을 그냥 쓰는게 좋을 것 같네요
https://developer.android.com/topic/libraries/architecture/coroutines?hl=ko
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.
헐 그러네!! 롤백해야징ㅋㅋ
sonms
left a comment
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.
고생하셨습니다! 짱짱 효빈님
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
UI 구현할 때 제대로 안했더니 수정사항이 엄청 많아졌네요😅
뭔가 더 잘 구현할 수 있을 것 같은데... 생각나면 수정할게요잉 하하
뷰모델 내부 코드는 기존 솝탬프 코드 가져다 쓰면서 약간씩 수정한 부분도 있어요! 저 혼자서 테스트를 할 수가 없어서 리뷰 하실 때 + 나중에 QA 하실 때 꼼꼼하게 확인해주시면 감사하겠습니다😊