Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UseElmish with Fast Refresh #481

Closed
Dzoukr opened this issue May 26, 2022 · 21 comments · Fixed by #515
Closed

UseElmish with Fast Refresh #481

Dzoukr opened this issue May 26, 2022 · 21 comments · Fixed by #515

Comments

@Dzoukr
Copy link
Contributor

Dzoukr commented May 26, 2022

Hi @Zaid-Ajaj,

this is related to WIP PR #423. I usually cannot make UseElmish to work with Fast Refresh, even when following all the rules like PascalCase + making init + update functions private. However, I tried to use @alfonsogarciacaro version and got some minor improvement:

useElmish

As you can see, Alfonso's version survives the first reload, and on the second change, it resets to the initial state again.

Do you have any idea what could be done on the top of Alfonso's PR to survive every refresh (a.k.a. make it working with Fast Refresh)? Thanks for any help! 🙏

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 26, 2022

Just FYI, I also tried #401 by @olivercoad and it even survives every refresh, however it stops triggering commands, so it's more less at the same state as Alfonso's PR (pretty close, but not fully functional). I assume this issue is quite hard to tackle. 😄

@MangelMaxime
Copy link
Contributor

Hello @Dzoukr,
in one of my project I made a custom/fixed version of UseElmish

I never had the chance to really test it but we are using it successfully in our project since 3+ months. And I didn't have the time to send a PR.

If you want to use it, you can copy/paste the code in your project as a replacement of Feliz.UseElmish deps.

Here is the code of it:

module Feliz.UseElmish

open Feliz
open Elmish

module Resumable =

    module Program =

        type Msg<'UserMsg, 'UserModel> =
            | UserMsg of 'UserMsg
            | SetState of 'UserModel

        let toResumable
            (initialState : 'model option)
            (program : Program<'a, 'model, 'msg, 'view>) =

            let map (model, cmd) =
                model
                , Cmd.map UserMsg cmd

            let mapInit userInit args =
                match initialState with
                | Some model ->
                    model, Cmd.none
                | None ->
                    userInit args |> map

            let mapUpdate userUpdate msg model =
                match msg with
                | UserMsg userMsg ->
                    userUpdate userMsg model
                | SetState newModel ->
                    newModel
                    , Cmd.none
                |> map

            let subs userSubscribe model =
                Cmd.batch [
                    userSubscribe model |> Cmd.map UserMsg
                ]

            let mapSetState userSetState model dispatch =
                userSetState model (UserMsg >> dispatch)

            let mapView userView model dispatch =
                userView model (UserMsg >> dispatch)

            program
            |> Program.map mapInit mapUpdate mapView mapSetState subs

type ElmishObservable<'Model, 'Msg>() =
    let mutable hasDisposedOnce = false
    let mutable state: 'Model option = None
    let mutable listener: ('Model -> unit) option = None
    let mutable dispatcher: ('Msg -> unit) option = None

    member _.Value = state
    member _.HasDisposedOnce = hasDisposedOnce

    member _.SetState (model: 'Model) (dispatch: 'Msg -> unit) =
        state <- Some model
        dispatcher <- Some dispatch
        match listener with
        | None -> ()
        | Some listener -> listener model

    member _.Dispatch(msg) =
        match dispatcher with
        | None -> () // Error?
        | Some dispatch -> dispatch msg

    member _.Subscribe(f) =
        match listener with
        | Some _ -> ()
        | None -> listener <- Some f

    /// Disposes state (and dispatcher) but keeps subscription
    member _.DisposeState() =
        match state with
        | Some state ->
            match box state with
            | :? System.IDisposable as disp -> disp.Dispose()
            | _ -> ()
        | _ -> ()
        dispatcher <- None
        state <- None
        hasDisposedOnce <- true

let inline runProgram
    (program: unit -> Program<'Arg, 'Model, 'Msg, unit>)
    (arg: 'Arg)
    (obs: ElmishObservable<'Model, 'Msg>)
    (resumedState: 'Model option)
    () =

    program()
    |> Program.withSetState obs.SetState
    |> Resumable.Program.toResumable resumedState
    |> Program.runWith arg

    match obs.Value with
    | None -> failwith "Elmish program has not initialized"
    | Some v -> v

let inline disposeState (state: obj) =
    match box state with
    | :? System.IDisposable as disp -> disp.Dispose()
    | _ -> ()

type React with
    [<Hook>]
    static member inline useElmish(program: unit -> Program<'Arg, 'Model, 'Msg, unit>, arg: 'Arg, ?dependencies: obj array) =
        // Don't use useMemo here because React doesn't guarantee it won't recreate it again
        let obs, _ = React.useState(fun () -> ElmishObservable<'Model, 'Msg>())

        // Initialize the program without previous state
        let state, setState = React.useState(runProgram program arg obs None)

        React.useEffect((fun () ->
            // If the component has been disposed, re-build it
            // use the last known state.
            // This make HMR support works by maintaining the state
            // We need to rebuild a new program, in order to get access
            // to the new dispatch instance
            if obs.HasDisposedOnce then
                runProgram program arg obs (Some state) () |> setState
            React.createDisposable(obs.DisposeState)
        ), defaultArg dependencies [||])

        obs.Subscribe(setState)
        state, obs.Dispatch

    [<Hook>]
    static member inline useElmish(program: unit -> Program<unit, 'Model, 'Msg, unit>, ?dependencies: obj array) =
        React.useElmish(program, (), ?dependencies=dependencies)

    [<Hook>]
    static member inline useElmish(init: 'Arg -> 'Model * Cmd<'Msg>, update: 'Msg -> 'Model -> 'Model * Cmd<'Msg>, arg: 'Arg, ?dependencies: obj array) =
        React.useElmish((fun () -> Program.mkProgram init update (fun _ _ -> ())), arg, ?dependencies=dependencies)

    [<Hook>]
    static member inline useElmish(init: unit -> 'Model * Cmd<'Msg>, update: 'Msg -> 'Model -> 'Model * Cmd<'Msg>, ?dependencies: obj array) =
        React.useElmish((fun () -> Program.mkProgram init update (fun _ _ -> ())), ?dependencies=dependencies)

    [<Hook>]
    static member inline useElmish(init: 'Model * Cmd<'Msg>, update: 'Msg -> 'Model -> 'Model * Cmd<'Msg>, ?dependencies: obj array) =
        React.useElmish((fun () -> Program.mkProgram (fun () -> init) update (fun _ _ -> ())), ?dependencies=dependencies)

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 26, 2022

OMG man, it works like a CHARM!!! 👏🔥💓

@MangelMaxime
Copy link
Contributor

Also just as a reminder for React refresh to work you need to only export ReactComponent from the module.

So mark init, update, or any other helpers function/values as private.

For vite.js, their is also the need to add this line

// Workaround to have React-refresh working
// I need to open an issue on react-refresh to see if they can improve the detection
emitJsStatement () "import React from \"react\""

Without this line vite.js doesn't seems to apply React Refresh

Example of a module:

using the bad example of counter 🙈

module Antidote.Components.Counter

// Stupid counter component
// This component is used to test integration with React and ViteJS
// In the future, this component will be removed from the project
// but I keep it in case I need to test/fix more stuff from Feliz

open Feliz
open Feliz.Bulma
open Elmish
open Fable.Core
open Feliz.UseElmish
open Fable.Core.JsInterop

// Workaround to have React-refresh working
// I need to open an issue on react-refresh to see if they can improve the detection
emitJsStatement () "import React from \"react\""

type private Classes =

    [<Emit("$0[\"counter-value\"]")>]
    abstract counterValue : string

let private classes : Classes = import "default" "./Counter.module.scss"

type private Model =
    {
        Value : int
    }

type private Msg =
    | Increment
    | Decrement

let private init () =
    {
        Value = 0
    }
    , Cmd.none

let private update (msg : Msg) (model : Model) =
    match msg with
    | Increment ->
        { model with
            Value = model.Value + 1
        }
        , Cmd.none

    | Decrement ->
        { model with
            Value = model.Value - 1
        }
        , Cmd.none

[<ReactComponent>]
let Counter () =
    let model, dispatch = React.useElmish(init, update, [||])

    Html.div [
        Html.div [
            prop.className classes.counterValue
            prop.text model.Value
        ]

        Bulma.button.button [
            prop.onClick (fun _ ->
                dispatch (Increment)
            )

            prop.text "Increment"
        ]

        Bulma.button.button [
            prop.onClick (fun _ ->
                dispatch (Decrement)
            )

            prop.text "Decrement"
        ]
    ]

@olivercoad
Copy link
Contributor

Sounds promising, will have to check it out!

@landy
Copy link
Contributor

landy commented May 26, 2022

Just a quick question here. It is a confusing topic for me. Do we need to use the @pmmmwh/react-refresh-webpack-plugin when building Feliz apps with Webpack or is the react-refresh enough?

@MangelMaxime
Copy link
Contributor

@landy I think that when using Webpack you need to @pmmmwh/react-refresh-webpack-plugin because Webpack doesn't have ReactRefresh out of the box.

It only have HMR (Hot module replacement) which can be seen as the ancestor of react-refresh for React code.

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 27, 2022

One observation here. I found that in this @MangelMaxime version of UseElmish, when I change the dependencies, the full render is not triggered (for original UseElmish it works). Will try to dig deeper and will let you know.

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 27, 2022

Ok, I may find it... I have to store also the state of dependencies and if it's not the same like previous ones, I need to do program re-init.

type React with
    [<Hook>]
    static member inline useElmish(program: unit -> Program<'Arg, 'Model, 'Msg, unit>, arg: 'Arg, ?dependencies: obj array) =
        // Don't use useMemo here because React doesn't guarantee it won't recreate it again
        let obs, _ = React.useState(fun () -> ElmishObservable<'Model, 'Msg>())

        // Initialize the program without previous state
        let state, setState = React.useState(runProgram program arg obs None)

        // Store initial dependencies
        let deps, setDeps = React.useState(dependencies)

        React.useEffect((fun () ->
            // Update for dependencies is the state valid
            dependencies |> setDeps

            // If the component has been disposed, re-build it
            // use the last known state.
            // This make HMR support works by maintaining the state
            // We need to rebuild a new program, in order to get access
            // to the new dispatch instance
            if obs.HasDisposedOnce then
                // Use only state that is valid for current dependencies
                let state = if deps <> dependencies then None else Some state
                runProgram program arg obs state () |> setState
            React.createDisposable(obs.DisposeState)
        ), defaultArg dependencies [||])

        obs.Subscribe(setState)
        state, obs.Dispatch

@MangelMaxime, please would you find time to look at this piece to tell me if you see any problem in it? 🙏

@Zaid-Ajaj
Copy link
Owner

Zaid-Ajaj commented May 29, 2022

Hi @Dzoukr and @MangelMaxime, thanks a lot for investigating these issues and coming up with a better solution 🙏 if you are satisfied with it, please consider making a PR. These days I am low on time to debug issues and add big features but if can definitely merge and publish new packages 😄 This one would be great so that we can implement what @olivercoad started with in #401

@MangelMaxime
Copy link
Contributor

One observation here. I found that in this @MangelMaxime version of UseElmish, when I change the dependencies, the full render is not triggered (for original UseElmish it works). Will try to dig deeper and will let you know.

@Dzoukr What do you mean by :

when I change the dependencies

Is it when the dependency change via runtime execution?

Or when your code changes and so trigger a refresh?

// Old code
let model, dispatch = React.useElmish(init, update, [||])

// New code
let model, dispatch = React.useElmish(init, update, [| myDeps |])

@Zaid-Ajaj Will send a PR yes.

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 31, 2022

Is it when the dependency change via runtime execution?

Yup. Exactly. I have a ReactComponent, that has a string parameter and this parameter is used in useElmish like:

let state, dispatch = React.useElmish(init, update, [| box myDependencyFromUpperLevel |])

When this dependency changes, it still behaves as if it did not.

So I did update in my file of your code to:

type React with
    [<Hook>]
    static member inline useElmish(program: unit -> Program<'Arg, 'Model, 'Msg, unit>, arg: 'Arg, ?dependencies: obj array) =
        // Don't use useMemo here because React doesn't guarantee it won't recreate it again
        let obs, _ = React.useState(fun () -> ElmishObservable<'Model, 'Msg>())

        // Initialize the program without previous state
        let state, setState = React.useState(runProgram program arg obs None)

        // Store initial dependencies
        let deps, setDeps = React.useState(dependencies)

        React.useEffect((fun () ->

            // If the component has been disposed, re-build it
            // use the last known state.
            // This make HMR support works by maintaining the state
            // We need to rebuild a new program, in order to get access
            // to the new dispatch instance
            if obs.HasDisposedOnce then
                // Use only state that is valid for current dependencies
                let state = if deps <> dependencies then None else Some state
                runProgram program arg obs state () |> setState

            // Update for dependencies is the state valid
            dependencies |> setDeps

            React.createDisposable(obs.DisposeState)
        ), defaultArg dependencies [||])

        obs.Subscribe(setState)
        state, obs.Dispatch

@MangelMaxime
Copy link
Contributor

@Dzoukr
If the dependencies changed in this case we expect the hook to be recreated from scratch?

As if the useElmish was invoked for the first time right?

Asking for confirmation and for doing my tests before sending the PR.

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 31, 2022

Yup. I would say so. It's still hook so if the deps in array changes, it should behave like it's the first time IMHO.

@Zaid-Ajaj
Copy link
Owner

If the dependencies changed in this case we expect the hook to be recreated from scratch?

@MangelMaxime yes, imagine if your Elmish program depends on userId in the URL (i.e. /user/{userId}) then

React.useElmish(init, update, [| box userId |])

Should re-initialize the Elmish program when the URL changes (userId changes)

@Dzoukr
Copy link
Contributor Author

Dzoukr commented May 31, 2022

imagine if your Elmish program depends on userId in the URL

Hey, stop looking at my private repo! That's exactly how I found it!!! 😁

@alfonsogarciacaro
Copy link
Contributor

@MangelMaxime In my previous PR I added a test for useElmish with dependencies, so if your code passes it hopefully it should be fine 😸

Feliz/tests/Tests.fs

Lines 1152 to 1177 in 2951622

testReactAsync "useElmish works with dependencies" <| async {
let render = RTL.render(UseElmish.wrapper())
Expect.equal (render.getByTestId("count").innerText) "0" "Should be initial state"
render.getByTestId("increment").click()
do!
RTL.waitFor <| fun () ->
Expect.equal (render.getByTestId("count").innerText) "1" "Should have been incremented"
|> Async.AwaitPromise
render.getByTestId("increment-wrapper").click()
do!
RTL.waitFor <| fun () ->
Expect.equal (render.getByTestId("count").innerText) "1" "State should be same because dependency hasn't changed"
|> Async.AwaitPromise
render.getByTestId("increment-wrapper").click()
do!
RTL.waitFor <| fun () ->
Expect.equal (render.getByTestId("count").innerText) "0" "State should have been reset because dependency has changed"
|> Async.AwaitPromise
}

@MangelMaxime
Copy link
Contributor

After watching some talks about React, using React.useEffect is not actually the right way of implementing the Elmish hooks.
It "works" but it can lead to some problem/un-expected behaviour especially with React 18.

I will revisit my implementation to fixes this issues/behaviour.

@alfonsogarciacaro
Copy link
Contributor

@MangelMaxime I haven't been following the #useEffectGate. What's the problem with React 18? useEffect with an empty dependency array may retrigger even if the component is not dismounted?

@MangelMaxime
Copy link
Contributor

Sorry for the delay, I had a lot of stuff on the plate.

The simplest explanation that I found is:

This means that each component is mounted, then unmounted, and then remounted and that a useEffect call with no dependencies will be run double-time when it is used in React in version 18.

They do that because they want to prepare for future feature 🤷‍♂️

This only occur during development and not on production but that's still something bad according to the community etc.

They introduce a new hooks useSyncExternalStore which allows library that manage state like Elmish, Redux, XState, etc. to be implemented "correctly" and in a future proof way.

My plan is for the next days to dive into useSyncExternalStore and see how to use it for the Elmish hooks like that we can have the correct implementation right away.

@Zaid-Ajaj
Copy link
Owner

@MangelMaxime Just to let you know before diving into specific React v18.x hooks, the current Feliz version still requires React >= 17.0.0 && < 18.0.0 so if you have an implementation that works in for React < 18.x then I think it is very much worth adding to useElmish, then users could start using it today and we could have a suit of unit tests for the expected behaviour which will help implement the upgrade later when we bump the React version requirements to > 18.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants