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

Wrapping composable functions in a class or object as extension (Improvement) #287

Open
ilyasdirin opened this issue Nov 8, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@ilyasdirin
Copy link

One of downsides of compose for me is that it pollutes intellisense so easily. For that I always wrap any theme properties such as colors, typography, shapes, other composable functions under wrapper objects and avoid making everything top level.

Example

@Immutable
class ThemeColors(val theme: Theme) {
 val utility0 = when(theme) {
        Theme.Light -> Color(0xFFFFFFFF)
        Theme.Dark -> Color(0xFF000000)
    }
  ...
}

I prefer to do the same for composable screens but compose-destinations expects screen names to be top level functions.

Wrap Login related screens under this object
object LoginNavHostScreens

Called wrapped screen from the object

@LoginNavGraph(start = true)
@Destination
@Composable
fun LoginNavHostScreens.SignUpOrLogInScreen() {
...
}

Wrapping screens as an extension of a class or object gives error since it is expected to be a top level function.

public object SignUpOrLogInScreenDestination : DirectionDestination {
         
    public operator fun invoke(): Direction = this
    
    @get:RestrictTo(RestrictTo.Scope.SUBCLASSES)
    override val baseRoute: String = "sign_up_or_log_in_screen"

    override val route: String = baseRoute
    
    @Composable
    override fun DestinationScope<Unit>.Content(
        dependenciesContainerBuilder: @Composable DependenciesContainerBuilder<Unit>.() -> Unit
    ) {
                 // This is expected to be a top level function, here gets the error
		**SignUpOrLogInScreen**(
			navigator = destinationsNavigator
		)
    }   
}

Can there be an improvement for wrapping screen composables and be able to produce required code from object or class values? Thanks in advance.

@Nek-12
Copy link

Nek-12 commented Nov 9, 2022

Composable functions should be top-level functions at all times, there is even a lint check for this.
Using non-top-level composables is an antipattern in my humble opinion (except for rare cases like DSLs)

Also, theme properties are expected to be wrapped in an object, not an injectable/creatable class. Compose has nothing against properties of objects for non-composables like Color or Typography, but I'd avoid creating multiple instances of a hardcoded object.

P.S. You solve visibility problems by modularizing your app, not "innerclassizing" it

@raamcosta
Copy link
Owner

I see two separate things here. I think @ilyasdirin's example shows just a global object is used as a receiver for the extension function that is the Composable Screen, but note that this Composable is still top-level, it just requires some object as the receiver.

If this is it, I don't think it's an antipattern, but I'm also not really prioritizing this 😛
What I can see myself doing is looking at receivers just as any other parameter to the function, and if that's the case, you could provide these objects as dependencies in the dependencyContainerBuilder parameter of the DestinationsNavHost and then the lib would be able to call the Composables 🙂
Seems fair @ilyasdirin ?

@raamcosta raamcosta added the enhancement New feature or request label Nov 9, 2022
@gandrewstone
Copy link

Composable functions should be top-level functions at all times, there is even a lint check for this. Using non-top-level composables is an antipattern in my humble opinion (except for rare cases like DSLs)

Suppose I have a composable function hierarchy that creates a dynamic screen. There's a bunch of state that is shared through all of these top level functions. So am I supposed to pass 10-20 of the same args into every single function to carry the shared state down to where its needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants