-
Notifications
You must be signed in to change notification settings - Fork 46
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
[아크] 4단계 자동 DI 미션 제출합니다 #54
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.
안녕하세요 아크! 🤖
4단계를 잘 구현해주셨습니다. 지금 시점에서 더 이상 구조에 대한 피드백은 드리지 않아도 될 것 같아요.
몇 가지 개선 사항에 대한 피드백을 남겼지만, 변경사항을 만들지 않아도 괜찮습니다.
compileOptions { | ||
sourceCompatibility = JavaVersion.VERSION_1_8 | ||
targetCompatibility = JavaVersion.VERSION_1_8 | ||
} | ||
kotlinOptions { | ||
jvmTarget = "1.8" |
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.
프로젝트 내에서 1.8과 11이 혼용되고 있습니다.
|
||
@Config(application = FakeApplication::class) | ||
@RunWith(RobolectricTestRunner::class) | ||
class ArkActivityTest { |
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.
검증이 필요한 로직 테스트를 꼼꼼하게 남겨주셨네요. 다른 개발자들도 이해할 수 있도록 문서화의 역할에도 신경을 쓰신게 보입니다.
추가로 다른 개발자에게 내가 작성한 코드의 동작 원리를 이해할 수 있도록 설명하는 예시를 참고해보시면 좋을 것 같아 Dagger 라이브러리의 링크를 남깁니다. 때로는 아주 구체적인 단위의 테스트가 아니라 작성한 기능에 대한 시나리오를 명시적으로 확인할 수 있는 테스트가 한눈에 더 잘 이해할 수 있게 만들어주기도 합니다.
- https://github.com/google/dagger/blob/master/javatests/dagger/android/AndroidInjectionTest.java
- https://github.com/google/dagger/tree/master/examples/bazel/java/example/hilt
참고:
#29 (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.
다음을 참고하여 Activity 등 컴포넌트 LifeCycle에 따라 달라지는 상태를 검증하는 테스트 시나리오를 더 명시적으로 바꿔볼 수 있을까요? 어떤 시나리오를 더 추가할 수 있을까요?
제가 기대했던 예시로는 다음과 같은 시나리오가 있었습니다.
// Activity가 Created, Started, Resumed된 상태일 때 의존성 주입 필요 마킹된 필드에 인스턴스가 존재한다.
// Activity가 Destroyed된 상태일 때 의존성 주입된 필드 인스턴스가 제거된다.
@Test | ||
fun `ArkActivity 클래스는 @ArkInject 어노테이션이 있는 필드에는 주입이 된다`() { | ||
// when | ||
`액티비티를 생성했을 때`().apply { |
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.
(nitpicking) apply 가 이 상황에서 적절한 scope function일까요?
@Test | ||
fun `ArkViewModel 클래스는 @ArkInject 어노테이션이 있는 필드에는 주입이 된다`() { | ||
// when | ||
`뷰모델을 생성했을 때`().apply { | ||
// then | ||
`@ArkInject 어노테이션이 있는 필드에는 주입이 된다`() | ||
} | ||
} |
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.
신선하네요..👍
@@ -45,6 +45,7 @@ android { | |||
dependencies { |
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.
커스텀 어노테이션을 정의해서 사용하는 것도 좋지만, JSR-330 표준 어노테이션을 사용하는 것도 고려해볼 수 있겠네요.
JSR 330(자바 의존성 주입)은 자바 플랫폼을 위한 의존성 주입 어노테이션을 표준화해서 스프링의
@Autowired
및@Qualifier
어노테이션과 비슷한@Inject
및@Named
어노테이션을 정의하고 있다.
- https://docs.spring.io/spring-framework/reference/core/beans/standard-annotations.html
- https://dagger.dev/dev-guide
By building on standard javax.inject annotations (JSR 330), each class is easy to test. You don’t need a bunch of boilerplate just to swap the RpcCreditCardService out for a FakeCreditCardService.
@Singleton | ||
fun provideCartProductDao( | ||
shoppingDatabase: ShoppingDatabase, | ||
): CartProductDao = shoppingDatabase.cartProductDao() | ||
|
||
@Singleton | ||
@StorageType(DATABASE) | ||
fun provideCartInDiskRepository( | ||
cartProductDao: CartProductDao, | ||
): CartRepository = CartInDiskRepository(cartProductDao) |
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.
CartProductDao 인스턴스를 생성하는 로직과 CartRepository 인스턴스를 생성하는 로직은 어떤 차이가 있을까요? 참고로 Hilt에서는 이런 차이를 Binds, Provides로 구분합니다.
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.
고생했습니다 아크 구조가 깔끔하네요!
구현에 있어서 의견드릴게 안보여서, 패키지 제안 의견 조금 남겼습니다!
아크가 판단해 보시고 적용해보면 좋을 것 같습니다:) 고생했어요~!
{ parentModule, _ -> ArkModule(parentModule) } | ||
var viewModelModule: (ArkModule) -> ArkModule = | ||
{ parentModule -> ArkModule(parentModule) } | ||
var serviceModule: (ArkModule) -> ArkModule = |
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.
Service같은 컴포넌트도 분리하신 이유가 있을까요?
@@ -45,6 +45,7 @@ android { | |||
dependencies { | |||
implementation(project(":domain")) | |||
implementation(project(":ark-di")) | |||
implementation(project(":ark-di:arik-di-android")) |
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.
ark-di에 같이 넣지 않은 이유가 무엇일까요?! 두개의 임포트를 하면 불편하진 않을까요?!
|
||
dependencies { | ||
implementation(project(":ark-di")) | ||
implementation(project(":domain")) |
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.
라이브러리에서 domain 의존성이 필요할까요?!
@Test | ||
fun `ArkViewModel 클래스는 @ArkInject 어노테이션이 있는 필드에는 주입이 된다`() { | ||
// when | ||
`뷰모델을 생성했을 때`().apply { | ||
// then | ||
`@ArkInject 어노테이션이 있는 필드에는 주입이 된다`() | ||
} | ||
} |
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 com.re4rk.arkdi.ArkModule | ||
|
||
class ArkModulesBuilder { | ||
var applicationModule: (Context) -> ArkModule = |
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.
applicationModule도 변경 가능성이 있을까요?!
안녕하세요 코비! 이번 4단계 미션도 잘 부탁드립니다!
패키지 구조 변화
안드로이드 의존성을 분리하면서 모듈 분리를 위해서
dagger와 dagger-hilt의 관계처럼
ark-di와 ark-di-android로 분리하게 되었습니다!
이처럼의 분리를 통해서 app에서는 Module만 구성해서 주입하면 되게 만들었습니다!
생명주기 관리
이번 미션에서 여러가지 생명주기를 관리하게 되었는데 저는 총 5개로 분리를 하였습니다!
구성은 화살표 방향으로 의존하고 있다는 것으로 화살표 방향으로만 의존성 탐색을 할 수 있습니다!
클래스명 통일
앞선 PR들에서는 DI-를 prefix로 했지만 4단계에서는 Ark-로 변경하였습니다!
미션 요구사항 체크리스트
다음 문제점을 해결한다.
선택 요구 사항