Deprecate DisposableRefEffect and DomSideEffect (#1714)

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 <TElement : Element> renderComposable(
         applier = DomApplier(DomNodeWrapper(root)),
         parent = recomposer
     )
-    val scope = object : DOMScope<TElement> {}
+    val scope = object : DOMScope<TElement> {
+        override val DisposableEffectScope.scopeElement: TElement
+            get() = root
+    }
     composition.setContent @Composable {
         content(scope)
     }

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<out TElement : Element>
+interface DOMScope<out TElement : Element> {
+    val DisposableEffectScope.scopeElement: TElement
+}
 
 /**
  * ElementScope allows adding effects to the Composable representing html element.
@@ -35,9 +36,11 @@ interface ElementScope<out TElement : Element> : DOMScope<TElement> {
      * 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<out TElement : Element> : DOMScope<TElement> {
      * 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<out TElement : Element> : DOMScope<TElement> {
      * 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<out TElement : Element> : ElementScope<TElement> {
-    abstract val element: TElement
-
     private var nextDisposableDomEffectKey = 0
+    abstract val element: TElement
 
     @Composable
     @NonRestartableComposable
@@ -114,7 +122,10 @@ abstract class ElementScopeBase<out TElement : Element> : ElementScope<TElement>
 }
 
 internal open class ElementScopeImpl<TElement : Element> : ElementScopeBase<TElement>() {
-    public override lateinit var element: TElement
+    override lateinit var element: TElement
+
+    override val DisposableEffectScope.scopeElement: TElement
+        get() = element
 }
 
 interface DomEffectScope {

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<String>()
+        val effectsList = mutableListOf<String>()
 
         var key = 1
         var recomposeScope: RecomposeScope? = null
@@ -120,24 +127,64 @@ class DomSideEffectTests {
                 }
                 DisposableRefEffect(key) {
                     effectsList.add("DisposableRefEffect")
-                    onDispose { }
+                    onDispose {
+                        effectsList.add("DisposableRefEffectDisposed")
+                    }
                 }
             }
         }
 
-        assertEquals(2, effectsList.size)
-        assertEquals("DisposableRefEffect", effectsList[0])
-        assertEquals("DomSideEffect", effectsList[1])
+        assertContentEquals(effectsList, listOf("DisposableRefEffect", "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("DisposableRefEffect", "DomSideEffect", "DisposableRefEffectDisposed", "DisposableRefEffect", "DomSideEffect")
+        )
     }
+
+    @Test
+    fun domSideEffectWithDisposableEffectTest() = runTest {
+        val effectsList = mutableListOf<String>()
+
+        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")
+                    }
+                }
+            }
+        }
+
+        assertContentEquals(effectsList, listOf("DisposableEffectOneTime", "DisposableEffect", "DomSideEffect"))
+
+        key = 2
+        recomposeScope?.invalidate()
+
+        waitForRecompositionComplete()
+
+        assertContentEquals(
+            effectsList,
+            listOf("DisposableEffectOneTime", "DisposableEffect", "DomSideEffect", "DisposableEffectDisposed", "DisposableEffect", "DomSideEffect")
+        )
+    }
+
 }