-
Notifications
You must be signed in to change notification settings - Fork 138
[HACK week] Modularization of SelectedSite #15086
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
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
185bdcc
Move `SiteComponent` and site utilities to `commons` module
samiuelson 0a60e64
Move `ResourceProvider` to `libs/commons` along with its dependencies
samiuelson bec6c5a
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson 65b2bc7
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson 9fa84ad
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson 4c1580d
Clean up code
samiuelson 03b653b
Clean up code
samiuelson fd7cff9
Clean up code
samiuelson 6ca3357
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson d2cf0a5
Clean up code
samiuelson 4f73066
Satisfy detekt's complaints
samiuelson 3bd48ff
Add lint options and baseline for StateFlowValueCalledInComposition i…
samiuelson e766de2
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson d5891f3
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson d1a5c17
Use fluxC as non-transitive dependency
samiuelson 6e509bc
Merge remote-tracking branch 'origin/hack-modularize-selected-site' i…
samiuelson f242626
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson 4bd18c1
Enable Compose support in build configuration in commons
samiuelson 1406662
Merge remote-tracking branch 'origin/hack-modularize-selected-site' i…
samiuelson 68fade9
Merge branch 'trunk' into hack-modularize-selected-site
samiuelson d74b37d
Add Kotlin Compose plugin to build configuration
samiuelson 06d96f4
Add Compose UI dependency to test fixtures
samiuelson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
16 changes: 16 additions & 0 deletions
16
WooCommerce/src/main/kotlin/com/woocommerce/android/di/SiteComponentEntryPoint.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package com.woocommerce.android.di | ||
|
|
||
| import androidx.datastore.core.DataStore | ||
| import com.woocommerce.android.ui.mystore.data.DashboardDataModel | ||
| import dagger.hilt.EntryPoint | ||
| import dagger.hilt.InstallIn | ||
| import kotlinx.coroutines.CoroutineScope | ||
|
|
||
| @InstallIn(SiteComponent::class) | ||
| @EntryPoint | ||
| interface SiteComponentEntryPoint { | ||
| fun dashboardDataStore(): DataStore<DashboardDataModel> | ||
|
|
||
| @SiteCoroutineScope | ||
| fun siteCoroutineScope(): CoroutineScope | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,10 @@ | ||
| plugins { | ||
| alias(libs.plugins.android.library) | ||
| alias(libs.plugins.kotlin.android) | ||
| alias(libs.plugins.kotlin.compose) | ||
| alias(libs.plugins.kotlin.parcelize) | ||
| alias(libs.plugins.ksp) | ||
| alias(libs.plugins.google.dagger.hilt) | ||
| alias(libs.plugins.dependency.analysis) | ||
| } | ||
|
|
||
|
|
@@ -19,17 +22,41 @@ android { | |
| targetCompatibility = libs.versions.java.get() | ||
| } | ||
|
|
||
| buildFeatures { | ||
| buildConfig = true | ||
| compose = true | ||
| } | ||
|
|
||
| lintOptions { | ||
| // 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 I think we might want to create a linear task for this to ensure we don't forget about it. |
||
| disable "StateFlowValueCalledInComposition" | ||
|
|
||
| baseline file("lint-baseline.xml") | ||
| } | ||
|
|
||
| testFixtures.enable = true | ||
| } | ||
|
|
||
| dependencies { | ||
| implementation(libs.androidx.annotation) | ||
| implementation(libs.androidx.core.ktx) | ||
| implementation(libs.androidx.compose.ui.main) | ||
| implementation(libs.glassfish.javax.annotation) | ||
| implementation(libs.kotlinx.coroutines.core) | ||
| implementation(libs.automattic.tracks.crashlogging) | ||
| implementation(libs.androidx.lifecycle.process) | ||
| implementation(libs.androidx.preference.ktx) | ||
| implementation(libs.eventbus.android) | ||
| implementation(libs.google.dagger.hilt.android.main) | ||
| implementation(libs.wordpress.utils) | ||
| implementation(libs.apache.commons.text) | ||
| implementation(project(":libs:fluxc")) | ||
| implementation(project(":libs:fluxc-plugin")) | ||
|
|
||
| ksp(libs.google.dagger.hilt.compiler) | ||
|
|
||
| testImplementation(libs.kotlinx.coroutines.test) | ||
| testImplementation(libs.assertj.core) | ||
|
|
@@ -38,6 +65,7 @@ dependencies { | |
| testImplementation(libs.androidx.arch.core.testing) | ||
| testImplementation(libs.androidx.lifecycle.runtime.testing) | ||
|
|
||
| testFixturesImplementation(libs.androidx.compose.ui.main) | ||
| testFixturesCompileOnly(libs.kotlinx.coroutines.test) | ||
| testFixturesCompileOnly(libs.junit) | ||
| testFixturesCompileOnly(libs.mockito.kotlin) | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <issues format="6" by="lint 8.7.2" type="baseline" client="gradle" dependencies="false" name="AGP (8.7.2)" variant="all" version="8.7.2"> | ||
|
|
||
| <issue | ||
| id="StateFlowValueCalledInComposition" | ||
| message="Calling value on a StateFlow inside of a composable function will not observe changes to the StateFlow" | ||
| errorLine1=" state.value?.let { return it }" | ||
| errorLine2=" ~~~~~"> | ||
| <location | ||
| file="src/main/java/com/woocommerce/android/tools/SelectedSite.kt" | ||
| line="64" | ||
| column="15"/> | ||
| </issue> | ||
|
|
||
| </issues> |
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
File renamed without changes.
File renamed without changes.
File renamed without changes.
2 changes: 1 addition & 1 deletion
2
...merce/android/tools/SiteConnectionType.kt → ...merce/android/tools/SiteConnectionType.kt
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
File renamed without changes.
File renamed without changes.
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
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.
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.
💡 I'm looking for opinions on whether it's better to declare
fluxcas a transitive dependency inlibs/commonsand remove it from theWooCommercemodule (this is what I did here since think it's cleaner) or keep it as a regular dependency in both of them. cc @wziebaThere 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.
I'm leaning into declaring
fluxcas regular dependency. I can imagine a scenario in which we add a new module that will usecommonbut won't have to use anything fromfluxc. Not havingfluxcas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something I believe Fluxc will be part of
commoneither 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I understand the issue as @samiuelson described it is
Option A:
fluxc transitiveOption B:
fluxc non-transitiveMy opinion is that I think
Option Bis better, because in the future, if we introduce a new moduleNewModuleand we'll want to usecommonthere but we don't need to usefluxc, then with Option B we don't unnecessarily offerfluxc. This way we reduce the dependency chain.Uh oh!
There was an error while loading. Please reload this page.
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.
That's also an interesting point to discuss: it depends on what
commonis for us. I guess we might need both: acommonthat includesfluxcand acommonthat 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see - makes sense, thanks for elaborating on it! I agree that option B sounds better.