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

Improving Development Support For Android Jetpack Compose Previews #1763

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

YanneckReiss
Copy link

Problem description this pull request aims to resolve

When we have a single composable in Android Jetpack Compose that depends on injecting some definitions, we can easily use the KoinApplication composable, like currently suggested in the Koin documentation for Compose Previews.

However, it's often the case that we want to define multiple compose previews with different use cases states of our composable. Unfortunately, when we have two or more compose previews in a single file, using the KoinApplication composable in both of them leads to a org.koin.core.error.KoinAppAlreadyStartedException because the first started KoinApplication also lives in the second compose preview.

The problem here is that we often want to provide a different set of definitions (a different list of modules), or simply can't guarantee which of the compose previews will build first, and therefore don't know if we need to start another KoinApplication or just can use the KoinContext composable to provide a KoinContext. Another solution would be to manually create an isolated context for each of the compose previews and supply it to a KoinApplication , which would introduce a lot of boilerplate code.

To showcase this problem at a practical example I created a small sample repository.

Description of my proposal

My solution for this problem is contained in this pull request. It should make the life easier for all developers who want to create compose previews for composables that rely on Koin.

If we come back to the mentioned example repository and take a look at the ExampleView instead of the following which throws the org.koin.core.error.KoinAppAlreadyStartedException:

@Preview
@Composable
fun Preview_ExampleView_enabled() {
    KoinApplication(application = { modules(appModule) }) {
        AppTheme {
            ExampleView(
                isEnabled = true
            )
        }
    }
}

@Preview
@Composable
fun Preview_ExampleView_disabled() {
    KoinApplication(application = { modules(appModule) }) {
        AppTheme {
            ExampleView(
                isEnabled = false
            )
        }
    }
}

we can simply replace the code with the following and don't need to worry about managing a compose preview (file) wide Koin context:

@Preview
@Composable
fun Preview_ExampleView_enabled() {
    KoinPreviewApplication(modules = { listOf(appModule) }) {
        AppTheme {
            ExampleView(
                isEnabled = true
            )
        }
    }
}

@Preview
@Composable
fun Preview_ExampleView_disabled() {
    KoinPreviewApplication(modules = { listOf(appModule) }) {
        AppTheme {
            ExampleView(
                isEnabled = false
            )
        }
    }

@arnaudgiuliani arnaudgiuliani self-requested a review January 25, 2024 10:29
@arnaudgiuliani arnaudgiuliani added this to the compose-3.6.0 milestone Jan 25, 2024
@arnaudgiuliani arnaudgiuliani added the status:checking currently in analysis - discussion or need more detailed specs label Jan 25, 2024
@YanneckReiss
Copy link
Author

I just realized a drawback with this approach. It only works for constructor injected parameters. When one uses the KoinComponent for field injection and the respective class needs access to that dependency in the context of that preview (for example at class initialization) the isolated context we defined in the preview wouldn't apply because in the KoinComponent we rely on the Koin GlobalContext .. therefore using field injection with this approach leads to another exception:

java.lang.IllegalStateException: KoinApplication has not been started at org.koin.core.context.GlobalContext.get(GlobalContext.kt:36)

To visualize it, the following example with constructor injection works:

class TestClass(
    private val myInjectedClass: MyInjectedClass
)

While this example won't work because we eagerly inject the MyInjectedClass and therefore access the GlobalContext under the hood:

class TestClass: KoinComponent {

    private val myInjectedClass: MyInjectedClass = get()
}

The Koin documentation suggests to define a custom KoinComponent but that wouldn't work for this case because we dynamically create a custom context for each preview. @arnaudgiuliani therefore maybe this pull request is obsolete or maybe you have an idea how to workaround this problem?

@arnaudgiuliani
Copy link
Member

we may need to investigate more to help cover the general case, and see the edge cases around 🤔
Let's keep it as a base of suggestion. You can also restart a PR if you want

@arnaudgiuliani
Copy link
Member

can be moved into koin-compose-viewmodel

@arnaudgiuliani
Copy link
Member

Otherwise idea sounds good. We need to tag it Experimental to get feedback

@arnaudgiuliani arnaudgiuliani added status:accepted accepted to be developed compose status:wait_feedback Need more details or follow-up and removed status:checking currently in analysis - discussion or need more detailed specs labels Aug 29, 2024
@arnaudgiuliani arnaudgiuliani modified the milestones: 4.0-RC2, 4.0-RC3 Aug 30, 2024
@arnaudgiuliani arnaudgiuliani modified the milestones: 4.0-RC3, 4.1 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compose status:accepted accepted to be developed status:wait_feedback Need more details or follow-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants