Merge pull request #17 from hotwired/bridge-delegate-lifecycle

Fix `BridgeDelegate` lifecycle issues
hotwired · Jul 13, 2023 · f998f0f · f998f0f
2 parents 609917b + 2efc15d
commit f998f0f
Show file tree

Hide file tree

Showing 7 changed files with 74 additions and 37 deletions.
diff --git a/strada/build.gradle b/strada/build.gradle
@@ -64,6 +64,12 @@ android {
     testOptions {
         unitTests.includeAndroidResources = true
         unitTests.returnDefaultValues = true
+
+        kotlinOptions {
+            freeCompilerArgs += [
+                '-Xopt-in=kotlinx.coroutines.ExperimentalCoroutinesApi'
+            ]
+        }
     }
 
     namespace 'dev.hotwire.strada'
@@ -79,6 +85,7 @@ dependencies {
     testImplementation 'junit:junit:4.13.2'
     testImplementation 'androidx.test:core:1.5.0'
     testImplementation 'androidx.lifecycle:lifecycle-runtime-testing:2.6.1'
+    testImplementation "org.jetbrains.kotlinx:kotlinx-coroutines-test:1.6.4"
     testImplementation 'org.robolectric:robolectric:4.9.2'
     testImplementation 'org.mockito:mockito-core:5.2.0'
     testImplementation 'com.nhaarman:mockito-kotlin:1.6.0'

diff --git a/strada/src/main/kotlin/dev/hotwire/strada/BridgeDelegate.kt b/strada/src/main/kotlin/dev/hotwire/strada/BridgeDelegate.kt
@@ -2,28 +2,20 @@ package dev.hotwire.strada
 
 import android.webkit.WebView
 import androidx.lifecycle.DefaultLifecycleObserver
-import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleOwner
 
 @Suppress("unused")
 class BridgeDelegate<D : BridgeDestination>(
+    val location: String,
     val destination: D,
     private val componentFactories: List<BridgeComponentFactory<D, BridgeComponent<D>>>
-) {
+) : DefaultLifecycleObserver {
     internal var bridge: Bridge? = null
+    private var destinationIsActive: Boolean = false
     private val initializedComponents = hashMapOf<String, BridgeComponent<D>>()
-    private val lifecycle get() = destination.bridgeDestinationLifecycleOwner().lifecycle
-    private val destinationIsActive get() = lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED)
-
-    private val allComponents: List<BridgeComponent<D>>
-        get() = initializedComponents.map { it.value }
 
     val activeComponents: List<BridgeComponent<D>>
-        get() = allComponents.takeIf { destinationIsActive } ?: emptyList()
-
-    init {
-        observeLifeCycle()
-    }
+        get() = initializedComponents.map { it.value }.takeIf { destinationIsActive }.orEmpty()
 
     fun onColdBootPageCompleted() {
         bridge?.load()
@@ -43,7 +35,7 @@ class BridgeDelegate<D : BridgeDestination>(
                 bridge?.load()
             }
         } else {
-            logEvent("bridgeNotInitializedForWebView", destination.bridgeDestinationLocation())
+            logEvent("bridgeNotInitializedForWebView", location)
         }
     }
 
@@ -57,7 +49,7 @@ class BridgeDelegate<D : BridgeDestination>(
     }
 
     internal fun bridgeDidReceiveMessage(message: Message): Boolean {
-        return if (destinationIsActive && destination.bridgeDestinationLocation() == message.metadata?.url) {
+        return if (destinationIsActive && location == message.metadata?.url) {
             logMessage("bridgeDidReceiveMessage", message)
             getOrCreateComponent(message.component)?.handle(message)
             true
@@ -73,20 +65,21 @@ class BridgeDelegate<D : BridgeDestination>(
 
     // Lifecycle events
 
-    private fun observeLifeCycle() {
-        destination.bridgeDestinationLifecycleOwner().lifecycle.addObserver(object :
-            DefaultLifecycleObserver {
-            override fun onStart(owner: LifecycleOwner) { onStart() }
-            override fun onStop(owner: LifecycleOwner) { onStop() }
-        })
+    override fun onStart(owner: LifecycleOwner) {
+        logEvent("bridgeDestinationDidStart", location)
+        destinationIsActive = true
+        activeComponents.forEach { it.onStart() }
     }
 
-    private fun onStart() {
-        activeComponents.forEach { it.onStart() }
+    override fun onStop(owner: LifecycleOwner) {
+        activeComponents.forEach { it.onStop() }
+        destinationIsActive = false
+        logEvent("bridgeDestinationDidStop", location)
     }
 
-    private fun onStop() {
-        allComponents.forEach { it.onStop() }
+    override fun onDestroy(owner: LifecycleOwner) {
+        destinationIsActive = false
+        logEvent("bridgeDestinationDidDestroy", location)
     }
 
     // Retrieve component(s) by type

diff --git a/strada/src/main/kotlin/dev/hotwire/strada/BridgeDestination.kt b/strada/src/main/kotlin/dev/hotwire/strada/BridgeDestination.kt
@@ -1,9 +1,5 @@
 package dev.hotwire.strada
 
-import androidx.lifecycle.LifecycleOwner
-
 interface BridgeDestination {
-    fun bridgeDestinationLocation(): String
-    fun bridgeDestinationLifecycleOwner(): LifecycleOwner
     fun bridgeWebViewIsReady(): Boolean
 }
diff --git a/strada/src/test/kotlin/dev/hotwire/strada/BridgeComponentFactoryTest.kt b/strada/src/test/kotlin/dev/hotwire/strada/BridgeComponentFactoryTest.kt
@@ -1,6 +1,5 @@
 package dev.hotwire.strada
 
-import androidx.lifecycle.testing.TestLifecycleOwner
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Test
@@ -14,6 +13,7 @@ class BridgeComponentFactoryTest {
         )
 
         val delegate = BridgeDelegate(
+            location = "https://37signals.com",
             destination = AppBridgeDestination(),
             componentFactories = factories
         )
@@ -28,8 +28,6 @@ class BridgeComponentFactoryTest {
     }
 
     class AppBridgeDestination : BridgeDestination {
-        override fun bridgeDestinationLocation() = "https://37signals.com"
-        override fun bridgeDestinationLifecycleOwner() = TestLifecycleOwner()
         override fun bridgeWebViewIsReady() = true
     }
 

diff --git a/strada/src/test/kotlin/dev/hotwire/strada/BridgeDelegateTest.kt b/strada/src/test/kotlin/dev/hotwire/strada/BridgeDelegateTest.kt
@@ -9,11 +9,13 @@ import com.nhaarman.mockito_kotlin.never
 import com.nhaarman.mockito_kotlin.whenever
 import org.junit.Assert.*
 import org.junit.Before
+import org.junit.Rule
 import org.junit.Test
 import org.mockito.Mockito.verify
 
 class BridgeDelegateTest {
     private lateinit var delegate: BridgeDelegate<AppBridgeDestination>
+    private lateinit var lifecycleOwner: TestLifecycleOwner
     private val bridge: Bridge = mock()
     private val webView: WebView = mock()
 
@@ -22,16 +24,24 @@ class BridgeDelegateTest {
         BridgeComponentFactory("two", ::TwoBridgeComponent)
     )
 
+    @Rule
+    @JvmField
+    var coroutinesTestRule = CoroutinesTestRule()
+
     @Before
     fun setup() {
         whenever(bridge.webView).thenReturn(webView)
         Bridge.initialize(bridge)
 
         delegate = BridgeDelegate(
+            location = "https://37signals.com",
             destination = AppBridgeDestination(),
             componentFactories = factories
         )
         delegate.bridge = bridge
+
+        lifecycleOwner = TestLifecycleOwner(Lifecycle.State.STARTED)
+        lifecycleOwner.lifecycle.addObserver(delegate)
     }
 
     @Test
@@ -112,9 +122,25 @@ class BridgeDelegateTest {
         assertNull(delegate.bridge)
     }
 
+    @Test
+    fun destinationIsInactive() {
+        val message = Message(
+            id = "1",
+            component = "one",
+            event = "connect",
+            metadata = Metadata("https://37signals.com"),
+            jsonData = """{"title":"Page-title","subtitle":"Page-subtitle"}"""
+        )
+
+        assertEquals(true, delegate.bridgeDidReceiveMessage(message))
+        assertNotNull(delegate.component<OneBridgeComponent>())
+
+        lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_STOP)
+        assertEquals(false, delegate.bridgeDidReceiveMessage(message))
+        assertNull(delegate.component<OneBridgeComponent>())
+    }
+
     class AppBridgeDestination : BridgeDestination {
-        override fun bridgeDestinationLocation() = "https://37signals.com"
-        override fun bridgeDestinationLifecycleOwner() = TestLifecycleOwner(Lifecycle.State.STARTED)
         override fun bridgeWebViewIsReady() = true
     }
 

diff --git a/strada/src/test/kotlin/dev/hotwire/strada/BridgeTest.kt b/strada/src/test/kotlin/dev/hotwire/strada/BridgeTest.kt
@@ -2,8 +2,6 @@ package dev.hotwire.strada
 
 import android.content.Context
 import android.webkit.WebView
-import androidx.lifecycle.LifecycleOwner
-import androidx.lifecycle.testing.TestLifecycleOwner
 import com.nhaarman.mockito_kotlin.any
 import com.nhaarman.mockito_kotlin.eq
 import com.nhaarman.mockito_kotlin.mock
@@ -137,8 +135,6 @@ class BridgeTest {
     }
 
     class AppBridgeDestination : BridgeDestination {
-        override fun bridgeDestinationLocation() = "https://37signals.com"
-        override fun bridgeDestinationLifecycleOwner() = TestLifecycleOwner()
         override fun bridgeWebViewIsReady() = true
     }
 }
diff --git a/strada/src/test/kotlin/dev/hotwire/strada/CoroutinesTestRule.kt b/strada/src/test/kotlin/dev/hotwire/strada/CoroutinesTestRule.kt
@@ -0,0 +1,21 @@
+package dev.hotwire.strada
+
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.test.*
+import org.junit.rules.TestWatcher
+import org.junit.runner.Description
+
+class CoroutinesTestRule(
+    private val testDispatcher: TestDispatcher = UnconfinedTestDispatcher(TestCoroutineScheduler())
+) : TestWatcher() {
+
+    override fun starting(description: Description) {
+        super.starting(description)
+        Dispatchers.setMain(testDispatcher)
+    }
+
+    override fun finished(description: Description) {
+        super.finished(description)
+        Dispatchers.resetMain()
+    }
+}