Avoid capturing onValue closure in useSubscription forever

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## 0.10.3
+
+### Changed
+
+- fixed a bug in `useSubscription` that kept the first `onValue` closure
+  forever, potentially resulting in using stale state values captured by the
+  closure.
+
 ## 0.10.2
 
 ### Changed

packages.dhall

@@ -3,3 +3,4 @@ let upstream =
         sha256:81881d9e15484551b4293ab0a2639355f38d0cab1dfa49a077b5f1af374c292a
 
 in  upstream
+  with elmish.version = "v0.12.0"

spago.dhall

@@ -1,12 +1,10 @@
 { name = "elmish-hooks"
 , dependencies =
-  [ "aff"
-  , "debug"
+  [ "debug"
   , "effect"
   , "elmish"
   , "maybe"
   , "prelude"
-  , "tuples"
   , "undefined-is-not-a-problem"
   ]
 , packages = ./packages.dhall

src/Elmish/Hooks/UseEffect.purs

@@ -81,12 +81,12 @@ useEffect_ name f deps runEffect =
         pure newDeps
     , view: \_ dispatch ->
         useEffectLifeCycles
-          { componentDidUpdate: dispatch <?| \(prevDeps :: Opaque.Opaque "deps" a) ->
-              if Opaque.unwrap prevDeps /= deps then
+          { componentDidUpdate: dispatch <?| \prevDeps ->
+              if Opaque.unwrap @"deps" prevDeps /= deps then
                 Just deps
               else
                 Nothing
-          , deps: (Opaque.wrap deps :: _ "deps" _)
+          , deps: Opaque.wrap @"deps" deps
           } $
           render unit
     }

src/Elmish/Hooks/UseSubscription.purs

@@ -6,7 +6,7 @@ module Elmish.Hooks.UseSubscription
 import Prelude
 
 import Effect.Aff (Aff)
-import Elmish.Component (ComponentName(..))
+import Elmish.Component (ComponentName(..), forkVoid)
 import Elmish.Hooks.Type (Hook, HookType, mkHook)
 import Elmish.Subscription (Subscription)
 import Elmish.Subscription as Sub
@@ -32,7 +32,12 @@ foreign import data UseSubscription :: Type -> HookType
 useSubscription :: ∀ a. Subscription Aff a -> (a -> Aff Unit) -> Hook (UseSubscription a) Unit
 useSubscription subscription onValue =
   mkHook (ComponentName "UseSubscription") \render ->
-    { init: subscription # Sub.quasiBind onValue # Sub.subscribe identity
-    , update: \_ _ -> pure unit
-    , view: \_ _ -> render unit
+    { init: do
+        Sub.subscribe identity subscription
+        pure unit
+    , update: \_ value -> do
+        forkVoid $ onValue value
+        pure unit
+    , view: \_ _ ->
+        render unit
     }

test.dhall

@@ -3,13 +3,19 @@ let conf = ./spago.dhall
 in conf // {
   sources = conf.sources # [ "test/**/*.purs" ],
   dependencies = conf.dependencies #
-    [ "arrays"
+    [ "aff"
+    , "arrays"
+    , "avar"
     , "control"
+    , "datetime"
     , "effect"
     , "elmish-html"
+    , "elmish-testing-library"
     , "foldable-traversable"
     , "foreign"
     , "nullable"
     , "spec"
+    , "tailrec"
+    , "tuples"
     ]
 }

test/Main.purs

@@ -21,12 +21,17 @@ import Test.Spec.Assertions (shouldEqual, shouldNotEqual)
 import Test.Spec.Assertions.String (shouldContain)
 import Test.Spec.Reporter.Spec (specReporter)
 import Test.Spec.Runner (runSpec)
+import Test.UseEffect as UseEffect
+import Test.UseSubscription as UseSubscription
 
 main :: Effect Unit
 main = launchAff_ $ runSpec [specReporter] spec
 
 spec :: Spec Unit
 spec = do
+  UseEffect.spec
+  UseSubscription.spec
+
   describe "naming" do
     describe "component" do
       let

test/UseEffect.purs

@@ -0,0 +1,60 @@
+module Test.UseEffect where
+
+import Prelude
+
+import Data.Time.Duration (Milliseconds(..))
+import Data.Tuple.Nested ((/\))
+import Effect.Aff (delay)
+import Effect.Aff.Class (liftAff)
+import Effect.Class (liftEffect)
+import Elmish ((<|))
+import Elmish.HTML.Styled as H
+import Elmish.Hooks as Hk
+import Elmish.Test (click, find, testElement, text, (>>))
+import Test.Spec (Spec, describe, it)
+import Test.Spec.Assertions (shouldEqual)
+
+spec :: Spec Unit
+spec = do
+  describe "useEffect" do
+    it "calls the most current closure" do
+      let component = Hk.component Hk.do
+            effectRuns /\ setEffectRuns <- Hk.useState 0
+            clicks /\ setClicks <- Hk.useState 0
+
+            -- This here is the tricky part. The second argument of `useEffect'`
+            -- is a closure that captures `effectRuns`. If the same closure was
+            -- called every time, the value of `effectRuns` would always be
+            -- zero, so it would always be calling `setEffectRuns 1`. But if the
+            -- the close from the most recent run is called, the value would be
+            -- up to date.
+            --
+            -- To test this we use the second effect (two lines below) that
+            -- writes the values of `clicks` and `effectRuns` to the `output`
+            -- mutable cell, and then we check that, after a few button clicks,
+            -- the cell has the right values.
+            Hk.useEffect' clicks \_ -> liftEffect $
+              setEffectRuns $ effectRuns + 1
+
+            Hk.pure $ H.fragment
+              [ H.div "clicks" $ show clicks
+              , H.div "effectRuns" $ show effectRuns
+              , H.button_ "" { onClick: setClicks <| clicks + 1 } ""
+              ]
+
+          assertOutput expected = do
+            liftAff $ delay $ Milliseconds 10.0
+            find ".clicks" >> text >>= (_ `shouldEqual` show expected.clicks)
+            find ".effectRuns" >> text >>= (_ `shouldEqual` show expected.effectRuns)
+
+      testElement component do
+        find "button" >> click
+        assertOutput { clicks: 1, effectRuns: 1 }
+
+        find "button" >> click
+        assertOutput { clicks: 2, effectRuns: 2 }
+
+        find "button" >> click
+        liftAff $ delay $ Milliseconds 1.0
+        find "button" >> click
+        assertOutput { clicks: 4, effectRuns: 4 }

test/UseSubscription.purs

@@ -0,0 +1,66 @@
+module Test.UseSubscription where
+
+import Prelude
+
+import Control.Monad.Rec.Class (forever)
+import Data.Array ((:))
+import Data.Time.Duration (Milliseconds(..))
+import Data.Tuple.Nested ((/\))
+import Effect.Aff (delay)
+import Effect.Aff.AVar as AVar
+import Effect.Aff.Class (liftAff)
+import Effect.Class (liftEffect)
+import Elmish.HTML.Styled as H
+import Elmish.Hooks as Hk
+import Elmish.Subscription (Subscription(..))
+import Elmish.Test (find, testElement, text, (>>))
+import Test.Spec (Spec, describe, it)
+import Test.Spec.Assertions (shouldEqual)
+
+spec :: Spec Unit
+spec = do
+  describe "useSubscription" do
+    it "calls the most current closure" do
+      source <- liftAff AVar.empty
+
+      let subscription = Subscription \dispatch -> do
+            void $ forever do
+              value <- AVar.take source
+              liftEffect $ dispatch value
+            pure $ pure unit
+
+      let component = Hk.component Hk.do
+            receivedValues /\ setReceivedValues <- Hk.useState []
+
+            -- This here is the tricky part. The second argument of `useEffect'`
+            -- is a closure that captures `effectRuns`. If the same closure was
+            -- called every time, the value of `effectRuns` would always be
+            -- zero, so it would always be calling `setEffectRuns 1`. But if the
+            -- the close from the most recent run is called, the value would be
+            -- up to date.
+            --
+            -- To test this we use the second effect (two lines below) that
+            -- writes the values of `clicks` and `effectRuns` to the `output`
+            -- mutable cell, and then we check that, after a few button clicks,
+            -- the cell has the right values.
+            Hk.useSubscription subscription \value -> liftEffect $
+              setReceivedValues $ value : receivedValues
+
+            Hk.pure $
+              H.div "" $ show receivedValues
+
+          assertReceivedValues expected = do
+            liftAff $ delay $ Milliseconds 10.0
+            find "div" >> text >>= (_ `shouldEqual` show expected)
+
+      testElement component do
+        liftAff $ AVar.put "one" source
+        assertReceivedValues ["one"]
+
+        liftAff $ AVar.put "two" source
+        assertReceivedValues ["two", "one"]
+
+        liftAff $ AVar.put "third" source
+        liftAff $ delay $ Milliseconds 1.0
+        liftAff $ AVar.put "cuatro" source
+        assertReceivedValues ["cuatro", "third", "two", "one"]