appKODE / detekt-rules-compose

A collection of Detekt rules for Jetpack Compose
MIT License
136 stars 8 forks source link

`TopLevelComposableFunctions` and `inteface`s #29

Open BraisGabin opened 1 year ago

BraisGabin commented 1 year ago

I like the idea of TopLevelComposableFunctions. The composable function shouldn't be declared inside an Activity or anything like that.

But some times I have a Module were I want to display a Component that can't be defined in my module. For example I have a ProductDetail and I want to show Offers. Offers is a different feature so what I do is create an interface like this:

interface OffersProvider {
  @Composable
  fun Offers(id: String, modifier: Modifier = Modifier)
}

Then with the magic of Dagger everything works. The problem is that TopLevelComposableFunctions complains about that @Composable inside an interface.

I see different ways to fix this:

interface OffersProvider {
  fun offers(): @Composable (id: String, modifier: Modifier = Modifier) -> Unit
}

Or don't use OffersProvider and just inject the lambda @Composable (id: String, modifier: Modifier = Modifier) -> Unit.

But I'm not 100% sure if any of those solutions are better than the original one.

What do you think? Should TopLevelComposableFunctions have a flag to allow interfaces? Am I missing other solution?

Sorry if this issue is too philosophical.

dimsuz commented 1 year ago

Hi! :wave:

I like the idea of TopLevelComposableFunctions.

Me too! And it even was you who suggested it :)

This way may have some issues:

 fun offers(): @Composable (id: String, modifier: Modifier = Modifier) -> Unit

The thing about this solution is that it would potentially cause unnecessary recompositions as this lambda would be re-created on each call. I got myself puzzled over this recently even asked on StackOverflow, but no one helped me, so I did a little debugging and proved that recompose happens more with this "lambda factory" variant. Still can't wrap my head over what actually happens in my examples.

I have checked my own code and in the current project I also use @Composable functions inside interfaces, but also inside objects (mainly to group them in some scope: I tend to define several slots for "common" components under such groups, e.g MyComponent(header = { MyComponent.Header(...) }, text = { MyComponent.Text(...) }).

So it seems that this tends to be a common thing and you're right about adding allowInterfaces flag and maybe turning it on by default. Maybe also a separate flag for objects (with default == false).

And classes should still be disallowed, because if you do want composables in them, that effectively means turning off this rule completely.

BraisGabin commented 1 year ago

Me too! And it even was you who suggested it :)

🤦🤦🤦 I'm getting old. I didn't recall that.

I agree, a configuration to allow interfaces sound good.