From 9ff38f8c5f22503c805870c8c32fcacd9e5672db Mon Sep 17 00:00:00 2001 From: Fyodor Soikin Date: Wed, 17 Jul 2024 09:20:41 -0400 Subject: [PATCH] Avoid capturing onValue closure in useSubscription forever --- CHANGELOG.md | 8 ++++ packages.dhall | 1 + spago.dhall | 4 +- src/Elmish/Hooks/UseEffect.purs | 6 +-- src/Elmish/Hooks/UseSubscription.purs | 13 ++++-- test.dhall | 8 +++- test/Main.purs | 5 ++ test/UseEffect.purs | 60 ++++++++++++++++++++++++ test/UseSubscription.purs | 66 +++++++++++++++++++++++++++ 9 files changed, 160 insertions(+), 11 deletions(-) create mode 100644 test/UseEffect.purs create mode 100644 test/UseSubscription.purs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ed4f7f..d9fcb32 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages.dhall b/packages.dhall index 60638b7..4e43f21 100644 --- a/packages.dhall +++ b/packages.dhall @@ -3,3 +3,4 @@ let upstream = sha256:81881d9e15484551b4293ab0a2639355f38d0cab1dfa49a077b5f1af374c292a in upstream + with elmish.version = "v0.12.0" diff --git a/spago.dhall b/spago.dhall index 128b3f1..c659486 100644 --- a/spago.dhall +++ b/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 diff --git a/src/Elmish/Hooks/UseEffect.purs b/src/Elmish/Hooks/UseEffect.purs index c305b1f..6dde0a4 100644 --- a/src/Elmish/Hooks/UseEffect.purs +++ b/src/Elmish/Hooks/UseEffect.purs @@ -81,12 +81,12 @@ useEffect_ name f deps runEffect = pure newDeps , view: \_ dispatch -> useEffectLifeCycles - { componentDidUpdate: dispatch - if Opaque.unwrap prevDeps /= deps then + { componentDidUpdate: dispatch + if Opaque.unwrap @"deps" prevDeps /= deps then Just deps else Nothing - , deps: (Opaque.wrap deps :: _ "deps" _) + , deps: Opaque.wrap @"deps" deps } $ render unit } diff --git a/src/Elmish/Hooks/UseSubscription.purs b/src/Elmish/Hooks/UseSubscription.purs index 2423fb5..9705158 100644 --- a/src/Elmish/Hooks/UseSubscription.purs +++ b/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 } diff --git a/test.dhall b/test.dhall index b6d3a29..b24341a 100644 --- a/test.dhall +++ b/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" ] } diff --git a/test/Main.purs b/test/Main.purs index 959625b..9b55f84 100644 --- a/test/Main.purs +++ b/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 diff --git a/test/UseEffect.purs b/test/UseEffect.purs new file mode 100644 index 0000000..49bef06 --- /dev/null +++ b/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 } diff --git a/test/UseSubscription.purs b/test/UseSubscription.purs new file mode 100644 index 0000000..dc1bdda --- /dev/null +++ b/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"]