Let each feature declare its own dependency injection module - #87
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every Koin definition in the app lived in
:app/di, including each feature's ViewModels. That meant:apphad to name concrete classes from every feature module —HubViewModel,EditorViewModel,GitAuthSettingsViewModeland the rest — so it could register them.This is the specific mistake both reference architectures make. Novix binds every
XxxFeatureApito itsXxxFeatureApiImplinapp/di/ApiModule.kt, and MENA does the same incomposeApp/di/apiModule.kt. The consequence in both is identical: the app module keeps a hard compile dependency on every implementation, and no implementation class can ever beinternal.It also blocks what comes next here. Giving each feature an api module only decouples anything if
:appstops needing the implementation — and while:appregisters the ViewModels, it needs the implementation regardless of how many contracts exist.What changed
Each feature now declares what it contributes to the graph:
onboardingModule— its two ViewModels and theOnboardingRepositorybindingsettingsModule— five settings ViewModelsprojectsModule— hub, open, folder-picker and create-projectterminalFeatureModule— the terminal ViewModeleditorModule— the editor and AI-chat ViewModels, both parameterised by project id:app/di/KoinModules.ktdrops from naming fourteen ViewModels to aggregating a list. What remains in it is genuinely app-scoped:NetworkMonitor.The
OnboardingRepositorybinding moved to the onboarding feature, where its implementation already lived.Behavior
No behavior change. The same definitions, declared by the module that owns them.
Scope
The
:data:*bindings stay in:appfor now. They bind:domaininterfaces to:dataimplementations, which is legitimately the composition root's job — a data module should not have to know it is being bound.This does not yet remove
:app's dependency on the feature implementations;:appstill calls each feature's nav graph. Removing that is the point of the api modules that follow, and this change is what makes them able to.Tests
KoinModuleGraphTestis the real gate here: it runs Koin'sverify()across the assembled graph, so a definition lost in the move fails at test time rather than on launch. It caught exactly this class of mistake earlier in the migration whenGradleProjectInspectorhad no binding.Verification
./gradlew test detekt verifyModuleBoundaries :app:assembleDebug— BUILD SUCCESSFUL. Boundary report: 0 baseline entries, 0 violations.Verified on an emulator, resolving a ViewModel from four different feature modules in one session:
HubViewModel)SettingsRootViewModel)GitAuthSettingsViewModel)CreateProjectViewModel)No
NoBeanDefFoundException,InstanceCreationExceptionor crash in logcat.