appKODE / detekt-rules-compose

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

New rule: Compose functions should be top-level functions #9

Closed BraisGabin closed 2 years ago

BraisGabin commented 2 years ago

Non compilant

class A : Activity() {

  @Component
  fun Asdf() {
    // ...
  }
}

Compilant

class A : Activity() {
  // ...
}

@Component
fun Asdf() {
  // ...
}

Context

A composable function shouldn't have access to mutables states that a Activity or Fragment store. For that reason those functions should be declared as top-level functions

dimsuz commented 2 years ago

When we were transitioning from using Conductor library (alternative to Fragment), we could have:

// base class
class ComposeController : Controller /* a-la Fragment */ {
   val state = mutableStateOf<ViewState>()

   init {
      presenter.stateFlow.onEach { state.value = it }.launchIn(scope)
      composeView.setContent { Content(state) }
   }

   @Composable
   abstract fun Content(state: ViewState)
}

// individual screen
class ScreenController : ComoseController<ViewState>() {
   private val someOtherState = mutableStateOf<MyState>()

   @Composable
   override fun Content(state: ViewState) { renderState(state); renderSometing(someOtherState) }
}

So theoretically private compose functions can be useful when used in inheritance-based frameworks. They could be still factored out as top-level, but all state which came from surrounding class would need to be passed explicitly as an argument.

Of course having this state implicit like this also relies on proper lifecycle management by the base class so that composable doesn't leak, but this could be done.

Anyway, after we've migrated completely away from legacy Fragments/Controllers we now have all "screen" composables as a top-level functions, so this rule makes sense!

And for someone who doesn't wish to follow: it can always be disabled :)

BraisGabin commented 2 years ago

Yes, I agree that this one could be a bit controversial but good on general cases. Maybe this one could be disabled by default. And let users to enable it.