cashapp / molecule

Build a StateFlow stream using Jetpack Compose
https://cashapp.github.io/molecule/docs/1.x/
Apache License 2.0
1.79k stars 77 forks source link

Add APIs with exposed emitter #285

Closed dzmpr closed 8 months ago

dzmpr commented 11 months ago

Problem:

We are use existing infrastructure of rememberSaveable to persist values in presenter, it requires to provide SaveableStateRegistry via CompositionLocalProvider. Current molecule API only takes composable body which return state. This API is not convenient for our use case. CompositionLocalProvider can't be used because it do not return value from it's content lambda.

body = {
    CompositionLocalProvider(
        LocalSaveableStateRegistry provides saveableStateRegistry,
    ) {
        presenterComposable() // <- CLP do not return this value
    }
}

Now we use own implementation of CLP that can return value from content lambda:

@Composable
@OptIn(InternalComposeApi::class)
private inline fun withPresenterSaveableStateRegistry(
    content: @Composable () -> State,
): State {
    currentComposer.startProviders(
        values = arrayOf(LocalSaveableStateRegistry provides saveableStateRegistry),
    )
    val state = content()
    currentComposer.endProviders()
    return state
}

But this approach has at least two significant disadvantages:

  1. We use internal compose API, that can change at every moment.
  2. Our CLP recomposes at every composition.

Proposal:

Molecule need alternative API with exposed emitter to be able to handle such cases:

body = { emitter ->
    CompositionLocalProvider(
        LocalSaveableStateRegistry provides saveableStateRegistry,
    ) {
        emitter(presenterComposable())
    }
}

Dunno is it possible to leave trailing lambda syntax for both bodies (with and w/o emitter). This head-on implementation has declaration clash when using trailing lambda. It requires to specify body parameter name to give a hint for compiler which lambda you want to use. Maybe it's possible to do this better, so this is not final code to approve.

JakeWharton commented 11 months ago

We use internal compose API, that can change at every moment.

These APIs cannot ever change without breaking every app in existence. They are internal because normal code should not be using them, but they are used inside inline functions and are thus part of the public API surface of Compose.

JakeWharton commented 11 months ago

Our CLP recomposes at every composition.

What's the cause? Is it due to the array creation inside the composable? Can you hoist that outside so its reference is stable and see if that helps?

Or is it simply due to the fact that Compose doesn't skip functions that return values?

dzmpr commented 11 months ago

Or is it simply due to the fact that Compose doesn't skip functions that return values?

Yes. To emit new state presenterComposable should be called, but due to it's inside 'our' CLP compose every time run provider code. Hoisting array is not an option.

JakeWharton commented 11 months ago

Hoisting array is not an option.

Why not? What about remembering then?

dzmpr commented 11 months ago

Why not? What about remembering then?

I mean recomposition of whole provider still happening when array hoisted.