Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
531 stars 77 forks source link

Weird dispatch issue with useElmish #569

Closed forki closed 1 year ago

forki commented 1 year ago
open Elmish
open Fable.React
open System
open Feliz.UseElmish

type Model = {
    Count : int
}

type Msg =
    | Refresh

let init initialValue =
    {
        Count = initialValue
    },
    Cmd.ofMsg Refresh

let update msg (model:Model) =
    match msg with
    | Refresh ->
        printfn "refreshing log history"
        { model with Count = model.Count + 1}, Cmd.none

open Feliz
open Fable.Core.JS

[<ReactComponent>]
let LogHistory
    (props:
        {|
            initialValue : int
        |})
    =
    let model, dispatch =
        React.useElmish (init props.initialValue, update, [||])

    let subscribeToTimer() =
        let subscriptionId =
            setInterval
                (fun _ ->
                    printfn "sending refresh"
                    dispatch Refresh)
                5000
        { new IDisposable with member __.Dispose() = clearTimeout(subscriptionId) }

    React.useEffect(subscribeToTimer, [| |])

    div [] [
        Html.button [
            prop.onClick (fun _ -> dispatch Refresh)
            prop.text "Refresh"
        ]

        str (string model.Count)
    ]

this code is expected to update the Count every 5s. It actually runs the timer functions and prints "sending refresh". BUT: since recently (Fable 4 update??) it doesn't dispatch the Refresh command anymore.

Surprisingly the dispatch works from within the button

@alfonsogarciacaro @tforkmann @Zaid-Ajaj

alfonsogarciacaro commented 1 year ago

Generated code looks right. By debugging I found that the dispatch reference captured by subscribeToTimer becomes invalid. Not sure if there's something funny going on or this how useSyncExternalStore is supposed to work (React has this oddities).

You have two options:

  1. Pass dispatch as a dependency to useEffect. This is actually what React recommends, any captured value must also be a dependency. React.useEffect(subscribeToTimer, [| box dispatch |])

  2. Use Elmish subscriptions

☝️ Forget about the 2nd option, I tested and in that case the subscription is registered twice. Not sure why.

forki commented 1 year ago

this actually makes a lot of sense and I wonder why it ever worked before.

anyways sorry about the noise and many thanks for the help to spot the obvious!

Zaid-Ajaj commented 1 year ago

this actually makes a lot of sense and I wonder why it ever worked before.

It could be that previous version gave you a stable dispatch (the view and the effect get the same dispatch, always)

Sometimes to avoid capturing, I use inline (i.e. let inline subscribeToTimer()) so that the body of the effect is inlined and doesn't capture but that might not work with effects as subscriptions. I used that often with React.useMemo

alfonsogarciacaro commented 1 year ago

@Zaid-Ajaj If I replace Feliz.UseElmish with Fable.React.UseElmish Elmish subscriptions work. Haven't checked what's the actual difference (maybe because Fable.React.UseElmish doesn't use the useSyncExternalStore polyfill?)

Test project: https://github.com/alfonsogarciacaro/app

Zaid-Ajaj commented 1 year ago

@alfonsogarciacaro I am not sure, will have a look! First thing I need to do is actually bring back tests to life to ensure expected behaviour, will try when time permits