Remove LayoutViewFactory + refactor FeatureView to make it compose-first.#511
Remove LayoutViewFactory + refactor FeatureView to make it compose-first.#511alexanderbezverhni wants to merge 6 commits into
LayoutViewFactory + refactor FeatureView to make it compose-first.#511Conversation
JaCoCo Code Coverage 93.37% ✅
Generated by 🚫 Danger |
![olive-agent[bot]](https://avatars.githubusercontent.com/in/1216019?s=60&v=4){kind=small}
| return featureView.view | ||
| val state = mutableStateOf(featureView.initialModel) | ||
| this.outputState = state | ||
| return ComposeView(requireContext()).apply { |
There was a problem hiding this comment.
This changes the framework's error-handling contract for render failures. routeDelegate.setOutput(...) is still wrapped in try/catch, but the actual UI work now happens later inside ComposeView.setContent { ... }, so exceptions thrown during composition/recomposition will bypass RouteEnvironment.onScreenError. The updated test suite explicitly expects that behavior, which means apps that currently rely on onScreenError for screen-level crash reporting or recovery lose it silently. Was that intended? If not, we need a way to preserve the old reporting path for render-time failures before landing this refactor.
There was a problem hiding this comment.
This actually changes nothing for the upstream app, given it was not using LayoutViewFactory. RouteEnvironment.onScreenError was meaningful in LayoutViewFactory since synchronous and imperative android.view.View setup could throw:
- null findViewById
- NPE on a missing field
- adapter blowing up
ComposeViewFactory, on other hand, both before and after the changes, is just changing a MutableState variable, while the actual rendering happens later, inside Compose recomposition, outside
try/catch.
So onScreenError no longer catches the kind of error it was originally designed for. We should think of an alternative approach here, which is out of scope for this PR.
| * } | ||
| * ``` | ||
| */ | ||
| abstract class ComposeViewFactory<RenderModel : Any> : ViewFactory<RenderModel> { |
There was a problem hiding this comment.
Moving ComposeViewFactory into formula-android while deleting the old formula-android-compose module creates an immediate source/binary compatibility break for downstream consumers. Existing imports of com.instacart.formula.android.compose.ComposeViewFactory stop compiling, and the old artifact disappears entirely. If this is meant to ship as a normal library release rather than a major-version break, we should keep a deprecated forwarding type/module for at least one release to provide a migration path.
📊 Benchmark Comparison ReportSummary
No significant changes (25 benchmarks)
Regressions: ±10% with non-overlapping confidence intervals. Improvements: ±10% change only. Generated by 🚫 Danger |
|
Should bump to 0.9.0 with this one |
| import com.instacart.formula.android.FeatureView | ||
| import com.instacart.formula.android.ViewFactory | ||
|
|
||
| abstract class ComposeViewFactory<RenderModel : Any> : ViewFactory<RenderModel> { |
There was a problem hiding this comment.
Now, when Compose is first class citizen in formula-android, no need to keep a separate formula-android-compose dependency with a single ComposeViewFactory class. Folding it into formula-android.
| val view: View, | ||
| val setOutput: (RenderModel) -> Unit, | ||
| val content: @Composable (RenderModel) -> Unit, | ||
| val initialModel: RenderModel? = null, |
There was a problem hiding this comment.
Removing unneeded android.view.View field from FeatureView.
| private var featureView: FeatureView<Any>? = null | ||
| private var outputState: MutableState<Any?>? = null |
There was a problem hiding this comment.
No need to keep FeatureView around anymore, for setting new output.
| // Based-on: https://developer.android.com/develop/ui/compose/migrate/interoperability-apis/compose-in-views#compose-in-fragments | ||
| setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) | ||
| setContent { | ||
| state.value?.let { featureView.content(it) } | ||
| } |
There was a problem hiding this comment.
Moving androidx.compose.ui.platform.ComposeView creation from ComposeViewFactory directly to FormulaFragment.
Alternative navigation host (Compose Nav 3) doesn't need ComposeView to render the content.
| return flow<Unit> { delay(5.seconds) }.flatMapLatest { localStore } | ||
| return flow { | ||
| delay(5.seconds) | ||
| emit(Unit) | ||
| }.flatMapLatest { localStore } |
There was a problem hiding this comment.
Fixing unrelated bug that was introduced here: #430
This flow was never emitting, resulting in Tasks app never showing tasks:
| import com.examples.todoapp.R | ||
| import com.instacart.formula.invoke | ||
|
|
||
| private val TodoColors = lightColors( |
There was a problem hiding this comment.
Migrating Tasks demo app from LayoutViewFactory to ComposeViewFactory.
There was a problem hiding this comment.
Few screenshots AFTER:
| 1 | 2 | 3 |
|---|---|---|
 |
 |
 |
alexanderbezverhni
changed the title
LayoutViewFactory + refactor FeatureView to make it compose-first.
FrancoisBlavoet
left a comment
FrancoisBlavoet
left a comment
There was a problem hiding this comment.
nice
the announcement of Android being Compose first is good timing for formula to remove its view integration 🙌
4 participants
Remove
LayoutViewFactory+ refactorFeatureViewto make it compose-first.Also, as a side effect of
LayoutViewFactoryremoval:formula-android-composemodule intoformula-androidWhy do we stop supporting Android Views?
Android is Compose-first