[HACK week] Modularization of SelectedSite#15086
Conversation
- Move `SiteComponent`, `SiteScope`, and `SiteCoroutineScope` definitions to `libs/commons`. - Move `SelectedSite` and `SiteConnectionType` to `libs/commons`. - Rename `SiteComponent.kt` to `SiteComponentEntryPoint.kt` in the app module to reflect the removal of the component definition. - Configure `libs/commons` build to support Hilt and expose FluxC dependencies.
f605018 to
185bdcc
Compare
Project dependencies changeslist+ New Dependencies
androidx.preference:preference-ktx:1.2.1tree+\--- project :libs:pos
+ +--- project :libs:commons
+ | +--- androidx.core:core-ktx:1.17.0 (*)
+ | +--- androidx.compose.ui:ui -> 1.9.4 (*)
+ | +--- androidx.preference:preference-ktx:1.2.1
+ | | +--- androidx.preference:preference:1.2.1 (*)
+ | | +--- androidx.core:core-ktx:1.1.0 -> 1.17.0 (*)
+ | | +--- androidx.fragment:fragment-ktx:1.3.6 -> 1.8.9 (*)
+ | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.0 -> 2.2.21 (*)
+ | +--- org.greenrobot:eventbus:3.3.1 (*)
+ | +--- org.apache.commons:commons-text:1.15.0 (*)
+ | +--- project :libs:fluxc (*)
+ | \--- project :libs:fluxc-plugin (*)
+ +--- project :libs:fluxc (*)
+ \--- project :libs:fluxc-plugin (*) |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Pull request overview
This PR modularizes the SelectedSite class by moving it (along with supporting utilities and extensions) from the main WooCommerce module to the libs/commons module. This enables better code reuse and reduces coupling, allowing the commons module to be used independently or in other contexts.
- Moves
SelectedSiteclass and related components to the commons module - Consolidates string utility functions (
StringUtils,StringExt,NumberExt) into commons - Updates
ResourceProviderto have its own implementation ofgetQuantityString - Separates app-specific
SiteComponentEntryPointfrom the coreSiteComponent
Reviewed changes
Copilot reviewed 6 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| libs/commons/src/main/java/com/woocommerce/android/tools/SelectedSite.kt | New file containing the SelectedSite singleton class moved to commons module |
| libs/commons/src/main/java/com/woocommerce/android/tools/SiteConnectionType.kt | Updated BuildConfig import to reference commons module instead of main app |
| libs/commons/src/main/java/com/woocommerce/android/viewmodel/ResourceProvider.kt | Replaced dependency on StringUtils with inline implementation of getQuantityString |
| libs/commons/src/main/java/com/woocommerce/android/util/StringUtils.kt | New utility object with string manipulation functions including email validation, formatting, and HTML processing |
| libs/commons/src/main/java/com/woocommerce/android/extensions/StringExt.kt | New extension functions for String including HTML stripping, version comparison, and file size formatting |
| libs/commons/src/main/java/com/woocommerce/android/extensions/NumberExt.kt | New extension functions for numbers including formatting and percentage conversion |
| libs/commons/src/main/java/com/woocommerce/android/di/SiteComponent.kt | Removed app-specific entry point interface, keeping only the core component definition |
| WooCommerce/src/main/kotlin/com/woocommerce/android/di/SiteComponentEntryPoint.kt | New file containing app-specific SiteComponent entry point moved from commons |
| libs/commons/build.gradle | Added Hilt, Compose, EventBus, and other dependencies; exposed FluxC via api configuration |
| WooCommerce/build.gradle | Removed FluxC dependencies as they now come transitively through commons |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| implementation(libs.automattic.tracks.android) | ||
| implementation(libs.automattic.tracks.crashlogging) | ||
|
|
||
| implementation(project(":libs:fluxc")) |
There was a problem hiding this comment.
💡 I'm looking for opinions on whether it's better to declare fluxc as a transitive dependency in libs/commons and remove it from the WooCommerce module (this is what I did here since think it's cleaner) or keep it as a regular dependency in both of them. cc @wzieba
There was a problem hiding this comment.
I'm leaning into declaring fluxc as regular dependency. I can imagine a scenario in which we add a new module that will use common but won't have to use anything from fluxc. Not having fluxc as transitive dependency would be an advantage in such a case, to not share unnecessary classes, recuding dependencies etc.
There was a problem hiding this comment.
Unless I'm missing something I believe Fluxc will be part of common either case @wzieba. The difference is just in the WooCommerce module - whether it's declared explicitly there or its a transitive dependency.
I mean, ideally, I would also prefer not having fluxc in common, but I don't think that's what's being discussed here.
There was a problem hiding this comment.
Ok so I understand the issue as @samiuelson described it is
Option A: fluxc transitive
// libs/commonts/build.gradle
dependencies {
api(project(":libs:fluxc"))
}
// WooCommerce/build.gradle
dependencies {
// nothing
}Option B: fluxc non-transitive
// libs/commonts/build.gradle
dependencies {
implementation(project(":libs:fluxc"))
}
// WooCommerce/build.gradle
dependencies {
implementation(project(":libs:fluxc"))
}My opinion is that I think Option B is better, because in the future, if we introduce a new module NewModule and we'll want to use common there but we don't need to use fluxc , then with Option B we don't unnecessarily offer fluxc. This way we reduce the dependency chain.
There was a problem hiding this comment.
I mean, ideally, I would also prefer not having fluxc in common, but I don't think that's what's being discussed here.
That's also an interesting point to discuss: it depends on what common is for us. I guess we might need both: a common that includes fluxc and a common that doesn't have to and shares different types of things. But this split is still doable incrementally later, so I think I would proceed with this PR as it's a step in right direction.
There was a problem hiding this comment.
Ahh, I see - makes sense, thanks for elaborating on it! I agree that option B sounds better.
| // Workaround for kotlinx-metadata version incompatibility: | ||
| // Kotlin 2.2.21 generates metadata v2.2.0, but AGP 8.13.2's Compose lint detector | ||
| // only supports up to v2.0.0, causing crashes when analyzing Kotlin files. | ||
| // This can be removed when AGP updates its bundled kotlinx-metadata-jvm dependency. |
There was a problem hiding this comment.
💡 I think we might want to create a linear task for this to ensure we don't forget about it.
|
Version |
…nto hack-modularize-selected-site
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #15086 +/- ##
=========================================
Coverage 38.69% 38.69%
Complexity 10547 10547
=========================================
Files 2192 2192
Lines 124712 124711 -1
Branches 17244 17243 -1
=========================================
Hits 48255 48255
+ Misses 71577 71576 -1
Partials 4880 4880 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR modularizes the
SelectedSiteclass by moving it (along with supporting utilities and extensions) from the main WooCommerce module to the libs/commons module.SelectedSiteis one of the core classes used by a great number of classes, moving it tolibs/commonsenables better code reuse and reduces coupling, unlocking further modularization possibilities 👉 #15094Test Steps
Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.