From 82bd465f5bfe03cf7a95f1e5bf671e2a37b85430 Mon Sep 17 00:00:00 2001 From: Shagen Ogandzhanian Date: Sat, 29 Jan 2022 00:23:41 +0100 Subject: [PATCH] Deprecate DisposableRefEffect and DomSideEffect (#1714) --- .../jetbrains/compose/web/RenderComposable.kt | 6 +- .../compose/web/elements/ElementScope.kt | 21 ++++-- .../src/jsTest/kotlin/DomSideEffectTests.kt | 73 +++++++++++++++---- 3 files changed, 81 insertions(+), 19 deletions(-) diff --git a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/RenderComposable.kt b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/RenderComposable.kt index 978f7f7031..afed73f8c7 100644 --- a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/RenderComposable.kt +++ b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/RenderComposable.kt @@ -5,6 +5,7 @@ import androidx.compose.runtime.Composition import androidx.compose.runtime.ControlledComposition import androidx.compose.runtime.MonotonicFrameClock import androidx.compose.runtime.DefaultMonotonicFrameClock +import androidx.compose.runtime.DisposableEffectScope import androidx.compose.runtime.Recomposer import org.jetbrains.compose.web.dom.DOMScope import kotlinx.browser.document @@ -37,7 +38,10 @@ fun renderComposable( applier = DomApplier(DomNodeWrapper(root)), parent = recomposer ) - val scope = object : DOMScope {} + val scope = object : DOMScope { + override val DisposableEffectScope.scopeElement: TElement + get() = root + } composition.setContent @Composable { content(scope) } diff --git a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/ElementScope.kt b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/ElementScope.kt index cd47766054..2ec9149ca6 100644 --- a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/ElementScope.kt +++ b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/ElementScope.kt @@ -11,9 +11,10 @@ import androidx.compose.runtime.SideEffect import androidx.compose.runtime.currentComposer import androidx.compose.runtime.remember import org.w3c.dom.Element -import org.w3c.dom.HTMLElement -interface DOMScope +interface DOMScope { + val DisposableEffectScope.scopeElement: TElement +} /** * ElementScope allows adding effects to the Composable representing html element. @@ -35,9 +36,11 @@ interface ElementScope : DOMScope { * and must be reversed or cleaned up if [key] changes or if the DisposableRefEffect leaves the composition. * [effect] lambda provides a reference to a native element represented by Composable. * Adding [DisposableEffectScope.onDispose] to [effect] is mandatory. + * DisposableRefEffect is deprecated, use regular DisposableEffect instead and access element via [DOMScope.scopeElement] if needed */ @Composable @NonRestartableComposable + @Deprecated("DisposableRefEffect is deprecated, use regular DisposableEffect instead and access element via scopeElement() if needed") fun DisposableRefEffect( key: Any?, effect: DisposableEffectScope.(TElement) -> DisposableEffectResult @@ -48,9 +51,11 @@ interface ElementScope : DOMScope { * and must be reversed or cleaned up if element or the DisposableRefEffect leaves the composition. * [effect] lambda provides a reference to a native element represented by Composable. * Adding [DisposableEffectScope.onDispose] to [effect] is mandatory. + * DisposableRefEffect is deprecated, use regular DisposableEffect instead and access element via [DOMScope.scopeElement] if needed */ @Composable @NonRestartableComposable + @Deprecated("DisposableRefEffect is deprecated, use regular DisposableEffect instead and access element via scopeElement() if needed") fun DisposableRefEffect( effect: DisposableEffectScope.(TElement) -> DisposableEffectResult ) { @@ -61,25 +66,28 @@ interface ElementScope : DOMScope { * A side effect of composition that runs on every successful recomposition if [key] changes. * Also see [SideEffect]. * Same as other effects in [ElementScope], it provides a reference to a native element in [effect] lambda. + * DomSideEffect is deprecated, use [SideEffect] instead. If, for some reason, you need to access the scope element, use DisposableEffect and call [DOMScope.scopeElement] */ @Composable @NonRestartableComposable + @Deprecated("DomSideEffect is deprecated, use SideEffect instead. If, for some reason, you need to access the scope element, use DisposableEffect") fun DomSideEffect(key: Any?, effect: DomEffectScope.(TElement) -> Unit) /** * A side effect of composition that runs on every successful recomposition. * Also see [SideEffect]. * Same as other effects in [ElementScope], it provides a reference to a native element in [effect] lambda. + * DomSideEffect is deprecated, use [SideEffect] instead. If, for some reason, you need to access the scope element, use DisposableEffect and call [DOMScope.scopeElement] */ @Composable @NonRestartableComposable + @Deprecated("DomSideEffect is deprecated, use SideEffect instead. If, for some reason, you need to access the scope element, use DisposableEffect") fun DomSideEffect(effect: DomEffectScope.(TElement) -> Unit) } abstract class ElementScopeBase : ElementScope { - abstract val element: TElement - private var nextDisposableDomEffectKey = 0 + abstract val element: TElement @Composable @NonRestartableComposable @@ -114,7 +122,10 @@ abstract class ElementScopeBase : ElementScope } internal open class ElementScopeImpl : ElementScopeBase() { - public override lateinit var element: TElement + override lateinit var element: TElement + + override val DisposableEffectScope.scopeElement: TElement + get() = element } interface DomEffectScope { diff --git a/web/core/src/jsTest/kotlin/DomSideEffectTests.kt b/web/core/src/jsTest/kotlin/DomSideEffectTests.kt index 819c0fbb74..905f1954f4 100644 --- a/web/core/src/jsTest/kotlin/DomSideEffectTests.kt +++ b/web/core/src/jsTest/kotlin/DomSideEffectTests.kt @@ -1,11 +1,18 @@ package org.jetbrains.compose.web.core.tests -import androidx.compose.runtime.* -import org.jetbrains.compose.web.dom.Div +import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect +import androidx.compose.runtime.RecomposeScope +import androidx.compose.runtime.currentRecomposeScope +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue import kotlinx.browser.document import kotlinx.dom.clear -import org.jetbrains.compose.web.testutils.* +import org.jetbrains.compose.web.dom.Div +import org.jetbrains.compose.web.testutils.runTest import kotlin.test.Test +import kotlin.test.assertContentEquals import kotlin.test.assertEquals class DomSideEffectTests { @@ -106,7 +113,7 @@ class DomSideEffectTests { @Test fun sideEffectsOrder() = runTest { - var effectsList = mutableListOf() + val effectsList = mutableListOf() var key = 1 var recomposeScope: RecomposeScope? = null @@ -120,24 +127,64 @@ class DomSideEffectTests { } DisposableRefEffect(key) { effectsList.add("DisposableRefEffect") - onDispose { } + onDispose { + effectsList.add("DisposableRefEffectDisposed") + } + } + } + } + + assertContentEquals(effectsList, listOf("DisposableRefEffect", "DomSideEffect")) + + key = 2 + recomposeScope?.invalidate() + + waitForRecompositionComplete() + + assertContentEquals( + effectsList, + listOf("DisposableRefEffect", "DomSideEffect", "DisposableRefEffectDisposed", "DisposableRefEffect", "DomSideEffect") + ) + } + + @Test + fun domSideEffectWithDisposableEffectTest() = runTest { + val effectsList = mutableListOf() + + var key = 1 + var recomposeScope: RecomposeScope? = null + + composition { + recomposeScope = currentRecomposeScope + + Div { + DisposableEffect(Unit) { + effectsList.add("DisposableEffectOneTime") + onDispose { } + } + DomSideEffect(key) { + effectsList.add("DomSideEffect") + } + DisposableEffect(key) { + effectsList.add("DisposableEffect") + onDispose { + effectsList.add("DisposableEffectDisposed") + } } } } - assertEquals(2, effectsList.size) - assertEquals("DisposableRefEffect", effectsList[0]) - assertEquals("DomSideEffect", effectsList[1]) + assertContentEquals(effectsList, listOf("DisposableEffectOneTime", "DisposableEffect", "DomSideEffect")) key = 2 recomposeScope?.invalidate() waitForRecompositionComplete() - assertEquals(4, effectsList.size) - assertEquals("DisposableRefEffect", effectsList[0]) - assertEquals("DomSideEffect", effectsList[1]) - assertEquals("DisposableRefEffect", effectsList[2]) - assertEquals("DomSideEffect", effectsList[3]) + assertContentEquals( + effectsList, + listOf("DisposableEffectOneTime", "DisposableEffect", "DomSideEffect", "DisposableEffectDisposed", "DisposableEffect", "DomSideEffect") + ) } + }