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

Feature/MZ-17 core module setting, moudle integration #5

Merged
merged 10 commits into from
Dec 7, 2023

Conversation

jinukeu
Copy link
Member

@jinukeu jinukeu commented Dec 2, 2023

💡 Issue

🌱 Key changes

  • BaseViewModel, collectWithLifecycle 추가
  • runCatchingIgnoreCancelled 추가
  • Dispatcher di 세팅
  • 모듈 간소화

✅ To Reviewers

collectWithLifecycle

collectWithLifecycle는 컴포즈에서 생명주기에 맞게 flow를 collect할 수 있는 확장 함수입니다.

@Composable
inline fun <reified T> Flow<T>.collectWithLifecycle(
    lifecycleOwner: LifecycleOwner = LocalLifecycleOwner.current,
    minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
    noinline action: suspend (T) -> Unit,
) {
    LaunchedEffect(key1 = Unit) {
        lifecycleOwner.lifecycleScope.launch {
            flowWithLifecycle(lifecycleOwner.lifecycle, minActiveState).collect {
                action(it)
            }
        }
    }
}

Dispatcher Di

#3 에 첨부한 참고 링크에서는 좀 더 다양한 Dispatcher를 추가해놨지만 당장 Susu에서는 IO Dispatcher만 사용할 것으로 예상되어 제외했습니다. 또한 nowinandroid의 방식을 참고했습니다.

아래와 같이 새로운 Dispatcher를 구성하여 CoroutineScope를 만드는 방법도 있지만, 당장에 필요한 코드는 아니기에 일단 추가하지 않았습니다. 추후 필요하다면 그때 추가해서 사용하시면 될 것 같습니다.

@Retention(AnnotationRetention.RUNTIME)
@Qualifier
annotation class ApplicationScope

@Module
@InstallIn(SingletonComponent::class)
object CoroutineScopesModule {
    @Provides
    @Singleton
    @ApplicationScope
    fun providesCoroutineScope(
        @Dispatcher(Default) dispatcher: CoroutineDispatcher,
    ): CoroutineScope = CoroutineScope(SupervisorJob() + dispatcher)
}

테스트할 만큼 개발 환경이 구축되지 않아 아직 테스트는 해보지 못했습니다.

📸 스크린샷

스크린샷
파일첨부바람

@jinukeu jinukeu added the chore label Dec 2, 2023
@jinukeu jinukeu self-assigned this Dec 2, 2023
Comment on lines +5 to +10
@Qualifier
annotation class Dispatcher(val susuDispatcher: SusuDispatchers)

enum class SusuDispatchers {
IO,
}
Copy link
Member

Choose a reason for hiding this comment

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

IO Dispatcher만 사용될 것으로 예상된다면, 이 Enum class가 현시점에 꼭 필요할까? 싶긴 합니다

Comment on lines +13 to +15
@Provides
@Dispatcher(SusuDispatchers.IO)
fun providesIODispatcher(): CoroutineDispatcher = Dispatchers.IO
Copy link
Member

Choose a reason for hiding this comment

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

@Qualifier
annotation class IODispatcher
object DispatchersModule {
    @Provides
    @IODispatcher
    fun providesIODispatcher(): CoroutineDispatcher = Dispatchers.IO

프로젝트 진행하면서 Dispatcher 추가가 되지 않고 IO 디스패쳐만 사용하게 된다고 가정했을 때, 이렇게 어노테이션을 사용하면 문제가 발생할지 궁금합니당

Copy link
Member Author

Choose a reason for hiding this comment

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

Dispatcher가 추가되지 않는다면 문제가 없을거같아요.

하지만 Default Dispatcher가 추가된다면, 수진님이 작성해주신 코드는 2가지 방법으로 수정할 수 있을거같아요.

  1. 새로운 DefaultDispatcher Qualifier 추가
@Qualifier
annotation class DefaultDispatcher
  1. Qualifier를 Dispatcher 하나로 통합하고 enum class를 통해 관리
@Qualifier
annotation class Dispatcher(val susuDispatcher: SusuDispatchers)

enum class SusuDispatchers {
    IO,
    Default,
}

1번 방식보다는 2번 방식이 더 좋다고 생각합니다. 그 이유는

  1. enum class로 감싸게 되면 SusuDispatcher안에 어떤 Dispatcher가 들어가있는지만 알면 되고 또 ide 자동 완성의 도움도 받을 수 있습니다.
  2. 1번 방식에 비해 코드를 덜 작성할 수 있습니다.

IO Dispatcher만 사용될 것으로 예상된다면, 이 Enum class가 현시점에 꼭 필요할까? 싶긴 합니다

현시점에서는 저도 필요하지 않다고 생각합니다. 하지만 추후 Dispatcher가 추가되는 경우를 대비해서 미리 enum class로 만들어놓는게 좋다 생각해요.

Copy link
Member

Choose a reason for hiding this comment

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

ide 자동완성 도움도 받을 수 있군요 😮 저는 필요 없는건 쓰지 않는 취향이어서 코멘트 남겼는데, 디스패쳐 추가에 대비하는 것 외의 장점이 있다면 도입하지 않을 이유가 없네요 👍

Comment on lines +19 to +24
LaunchedEffect(key1 = Unit) {
lifecycleOwner.lifecycleScope.launch {
flowWithLifecycle(lifecycleOwner.lifecycle, minActiveState).collect {
action(it)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

이 확장함수 lifecycleScope.launch에 싸여있는거만 보다가 LaunchedEffect에 감싸진 모습은 처음보니 뭔가 새롭네요 😄

@jinukeu
Copy link
Member Author

jinukeu commented Dec 4, 2023

해당 PR, Branch에서 #4 이슈도 함께 진행하겠습니다!
작업 완료 후 코멘트 남길게요~

@jinukeu jinukeu changed the title Feature/MZ-17 core module setting Feature/MZ-17 core module setting, moudle integration Dec 4, 2023
@jinukeu
Copy link
Member Author

jinukeu commented Dec 4, 2023

모듈 통합 완료했습니다! 같이 코드 리뷰 부탁드려요~

Copy link
Member

@yangsooplus yangsooplus left a comment

Choose a reason for hiding this comment

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

모듈 다시 합치는 작업도 꽤나 번거로우셨을텐데 수고 많으셨습니다!

@jinukeu
Copy link
Member Author

jinukeu commented Dec 4, 2023

모듈 다시 합치는 작업도 꽤나 번거로우셨을텐데 수고 많으셨습니다!

저야말로 항상 꼼꼼한 코멘트 감사드립니다 ㅎㅎ (__)

@syb8200
Copy link
Contributor

syb8200 commented Dec 7, 2023

확인했습니다! 모듈화 다시 수정하시느라 수고 많으셨습니다 :)

@jinukeu jinukeu merged commit 8b0a3d4 into develop Dec 7, 2023
1 check passed
@jinukeu jinukeu deleted the feature/MZ-17-core-module-setting branch December 7, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chore] Module Integration [Chore] Core Module Setting
3 participants