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

(ENG)[#252] feature 모듈 내 main 과 다른 모듈에 대해 수평 구조화 #261

Conversation

SeongUgJung
Copy link

@SeongUgJung SeongUgJung commented Sep 6, 2023

Issue

Overview (Required)

  • feature:main 은 각 feature:xxx-api 만 참조
    • 구현체는 DI 를 통해 주입받음
    • 이를 통해 feature 내의 구현체 모듈은 의존성에서 수평적 관계로 정의됨
  • main-nav-graph 의 기능
    • DroidKnightsNavGraph : nav-builder 를 위한 추상화 추가
    • DroidKnightsNavController : nav-controller 를 위한 추상화 추가
    • DroidKnightsTab : tab 정보를 관리하기 위한 추상화 추가
  • DroidKnightsNavController DroidKnightsNavGraph
    • home-api session-api setting-api contributor-api bookmark-api 에서 DroidKnightsNavControllerDroidKnightsNavGraph 추상화 추가
    • main 모듈에서는 각 하위모듈의 xxx-api 모듈 의존성 추가

기존 코드들이 함수위주로 적용되어 main 모듈에서 하위 구현체 모듈에 대해 강한 커플링이 구성되어 있어 nav-graph 기능과 에러 처리등에서 이를 분리하기가 매우 난해하여 이를 해결하고자 LocalCompositionProvider 를 이용하였으나 이는 임시방편에 가까운 형태입니다.

MainActivity 에서는 각 모듈내의 DroidKnightsNavGraph DroidKnightsNavController DroidKnightsTab 의 구현체를 추상화 객체를 통해서 주입받고 이를 동작하도록 하였습니다.

의존성 변경사항

v1 v2
before after

차이점

순서 기존 신규
P0 App App
P1 main main,home, setting, contributor, session, bookmark, core-impl
P2 home, setting, contributor, session, bookmark, core-impl core-apis, feature-apis ,main-nav-graph ,design, ui, navigation
P3 core-apis, design, ui, navigation -

@SeongUgJung SeongUgJung changed the title (ENG)[#252] feature 모듈 내 main 과 다른 모듈에 대해 수평 구조화 WIP (ENG)[#252] feature 모듈 내 main 과 다른 모듈에 대해 수평 구조화 Sep 7, 2023
@SeongUgJung
Copy link
Author

좀 더 naive 한 접근방법이 생각나서 다시 작업해서 올리겠습니다.

@laco-dev
Copy link
Contributor

laco-dev commented Sep 7, 2023

기존 코드들이 함수위주로 적용되어 main 모듈에서 하위 구현체 모듈에 대해 강한 커플링이 구성되어 있어 nav-graph 기능과 에러 처리등에서 이를 분리하기가 매우 난해하여 이를 해결하고자 LocalCompositionProvider 를 이용하였으나 이는 임시방편에 가까운 형태입니다.

이 점이 Feature 베이스를 사용하는 레퍼런스에서 가장 어려운 고민 주제가 되는 것 같습니다.
대부분 단순한 형태의 네비게이션 형태이지만 사실 현업과 같은 환경에서는 복잡한 라우팅이 요구되는데 하나의 모듈에서 모든 라우팅을 담당하는부분에 어려움이 있었습니다.
작성해주신 내용처럼 이러한 문제를 해결하기 위한 뚜렷한 Practice가 아직은 참고할만한 자료가 많이 부족한 상태이기 때문에, 의견주신 내용이 개발자들의 고민 욕구를 한층 푸는데 도움이 될 것으로 보입니다. 👍🏻

@SeongUgJung SeongUgJung force-pushed the eng/252_restructure_part2 branch from d4b79ec to 09eac8b Compare September 7, 2023 16:21
@SeongUgJung
Copy link
Author

좀 더 간단하고 직관적인 형태로 변경하였습니다.

MainNavigator 에서 사용할 DroidKnightsNavController, NavGraph 빌드를 위한 DroidKnightsNavGraph, 탭 정보 관리를 위한 DroidKnightsTab 을 추가 하고 이를 주입받아 기존의 코드를 대체하는 형태로 해서 변경을 최소화 하도록 하였습니다.

@SeongUgJung SeongUgJung changed the title WIP (ENG)[#252] feature 모듈 내 main 과 다른 모듈에 대해 수평 구조화 (ENG)[#252] feature 모듈 내 main 과 다른 모듈에 대해 수평 구조화 Sep 7, 2023
@SeongUgJung SeongUgJung force-pushed the eng/252_restructure_part2 branch from 09eac8b to 7a5e1d7 Compare September 8, 2023 00:43
@SeongUgJung SeongUgJung force-pushed the eng/252_restructure_part2 branch from 7a5e1d7 to 7d0e5c5 Compare September 8, 2023 13:33
@SeongUgJung
Copy link
Author

리뷰해주시는대로 #262 작업 진행하도록 하겠습니다.

Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

감사합니다. 그리고 답변이 늦었습니다.

초기 UI 레이어 구조를 추상화 하는 방향으로 개발을 진행하였는데, 어떤 방식으로든 시원하게 해결되지 못하여 직접 의존하는 방식으로 만든 기억이 나네요.

구현적으로는 제시해주신 방향과 일치하기에 몇가지 의견 남겼습니다. 🙏🏻

}

android {
namespace = "com.droidknights.app2023.feature.main_nav_graph"
Copy link
Contributor

Choose a reason for hiding this comment

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

네임스페이스의 구분이 (underscore) 오타로 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

앗 👀

Copy link
Author

Choose a reason for hiding this comment

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

수정하였습니다.

Comment on lines +8 to +10
class MainTabs @Inject constructor(
private val tabs: Set<@JvmSuppressWildcards DroidKnightsTab>,
) {
Copy link
Contributor

@laco-dev laco-dev Sep 9, 2023

Choose a reason for hiding this comment

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

기존에는 메인 화면이 가지는 탭을 알고 있었는데 지금 구조에서는 추상화로 탭을 주입받고 있습니다.
하지만 어떤 탭을 이동할 때에는 명확하게 특정 화면의 api를 사용합니다.
*두개 모두 추상화가 이루어진다면 DroidKnightsTabNavController 라는 일반화가 이루어지고 Tab을 받아 이동할 것임

DroidKnightsTab.XXX --> ContributorNavController.navigate()
탭을 주입받는 이점은 탭에 대한 구현을 각각의 피쳐로 위임하는 것에 있는 것으로 보입니다.
그렇다면 탭을 그리기 위한 UI 관련 정보는 메인이 가져야 하는 것이 아닌 각 피쳐에서 구현해야 한다는 방향인가요?

Copy link
Author

@SeongUgJung SeongUgJung Sep 9, 2023

Choose a reason for hiding this comment

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

궁극적으로 상하위의 관계를 가졌다 하더라도 추상화를 통해서 구현체가 무엇인지 몰라도 동작이 될거라는 전제로 구현하였습니다.

따라서 각 탭의 정보도 하위에서 가지고 상위에서는 가져선 안된다고 봅니다.

이러한 구현방식은 탭이 수십, 수백개가 생겨도 feature:main 에 코드 추가 없이 동작을 할 수 있기 때문에 확장성을 고려한 설계라고 볼 수 있습니다.

이를 확장한 개념의 예시 중에 Server-Driven-UI 도 있습니다.

Comment on lines +5 to +8
plugins {
id("com.android.library")
id("droidknights.verify.detekt")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

의도를 하지 않았었는데 라이브러리 플러그인에 Hilt 의존성이 포함되게 변경이 되어있었네요...
*원래는 library, hilt 따로 선언했음

no-hilt 모듈 제공이 플러그인 컨벤션을 근본적으로 개선한 방법은 아닌 것으로 보이지만 현재 구현에서 해결하기 위한 방법으로 이해했습니다.

Copy link
Author

Choose a reason for hiding this comment

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

👍

Comment on lines +6 to +10
interface BookmarkNavController : DroidKnightsNavController<BookmarkNavControllerInfo>

data class BookmarkNavControllerInfo(
val navOptions: NavOptions,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

정말 이상적인 UI 레이어 추상화라면 구현체가 네비게이션을 사용하는지, 아닌지 관심이 없는 정도로 만들어진다고 생각합니다.

북마크를 실행하기 위해서 반드시 네비게이션을 사용한다는 제약이 걸린다는 의미로 볼 수도 있는데요.
이러한 관점에서 보면 네비게이션을 UI 인터페이스로 존재해도 상관 없다로 생각하시나요?
저는 개인적으로 네비게이션 구조를 모르도록 추상화 하는것에 실패하여 의견을 여쭙고 싶습니다.

Copy link
Author

@SeongUgJung SeongUgJung Sep 9, 2023

Choose a reason for hiding this comment

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

현재의 구조는 Compose Navigation 을 사용한다는 전제로 구현이 되었습니다.

저는 Compose Navigation 은 유연성이 극도로 단절된 컴포넌트로 규정하고 사용을 하지 말 것을 권고하고

Uber-RIBs Router 를 변형한 형태로 새로운 컴포넌트를 만들어서 사용합니다.

@laco-dev
Copy link
Contributor

@SeongUgJung
#261 (comment)
이 내용 놓치신 것 같아서 맨션 드립니다.

확인되면 병합하도록 하겠습니다. 🙏🏻

@SeongUgJung SeongUgJung force-pushed the eng/252_restructure_part2 branch from 7d0e5c5 to e4d3144 Compare September 10, 2023 03:54
Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

@laco-dev laco-dev merged commit 34e4b01 into droidknights:MVVM_Split_API_Module Sep 10, 2023
@SeongUgJung SeongUgJung deleted the eng/252_restructure_part2 branch September 21, 2023 04:46
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.

4 participants