Clean old events handler when HMR is trigger

[MangelMaxime]

Description

When working on Thoth.Elmish.Toast I discovered that previous event handler are not cleaned HMR update. This is normal, because HMR can't know how to clean the old events handlers.

I managed to write a fix for this

// If HMR support is active, then we provide have a custom implementation.
// This is needed to avoid:
// - flickering (trigger several react renderer process)
// - attaching several event listener to the same event
if not (isNull HMR.``module``.hot) then
    if HMR.``module``.hot.status() <> HMR.Idle then
        Browser.window.removeEventListener(eventIdentifier, !!Browser.window?(eventIdentifier))

    Browser.window?(eventIdentifier) <- fun (ev : Browser.Event) ->
        let ev = ev :?> Browser.CustomEvent
        dispatch (Add (unbox ev.detail))

    Browser.window.addEventListener(eventIdentifier, !!Browser.window?(eventIdentifier))
else
    Browser.window.addEventListener(eventIdentifier, !^(fun ev ->
        let ev = ev :?> Browser.CustomEvent
        dispatch (Add (unbox ev.detail))
))

This is a known problem when using HMR, we need to clean the "global states" (event listener, global variables, etc) by ourself even in pure JavaScript.

At the time I was thinking that the Program.toNavigable was ok in current state because it's checking capturing when the newLocation is equal to lastLocation.

But it's not enough in fact, because when we have an HMR update I think we are creating a new Elmish instance. And so each instance is having its own handlers and lastLocation reference.

So for example, if in your code base when you have a page change you send a request to the server then you will send X request to your server. In the same way, I do have some concurrent race and each application try to render it's own view when a page change and it's causing some visual flickering.

In general, the renderered view is the newest program registered so this is why no one noticed this problem yet.

I will send a PR to remove old events listeners when using Program.toNavigable

---

On a side note, I will try to see if we can also completly kill the previous Elmish instance as they are still in running in memories.

Here I triggered 3 updates of the code and so have:

• Fleet Mapping aka instance n°1
• Instance n°2
• Instance n°3
• Instance n°4

[et1975]

I understand the concern, but let's solve this in HMR, I don't want this package to know about HMR.
I'm thinking HMR could shadow Program.toNavigable and implement HMR-specific logic to release event handlers. Makes sense?

[MangelMaxime]

Here is a gif showing the problem:

Source code (click to expand)

module App.View

open Elmish
open Fable.Helpers.React
open Fable.Helpers.React.Props
open Elmish.Browser.Navigation
open Fable.Import
open Fable.Core.JsInterop

let instanceID : int =
    Browser.window?HMR_Count <-
        if isNull Browser.window?HMR_Count then
            0
        else
            Browser.window?HMR_Count + 1
    Browser.window?HMR_Count

type Route =
    | Page1
    | Page2

    static member ToUrl (route : Route) =
        match route with
        | Page1 -> "#page1"
        | Page2 -> "#page2"

type Model =
    { CurrentRoute : Route }

type Msg =
    | GoTo of Route

let urlUpdate (result : Route option) model =
    Browser.console.log("Message from instance n°", instanceID)
    match result with
    | None ->
        model, Navigation.modifyUrl "#"

    | Some Page1 ->
        { model with CurrentRoute = Page1 }, Cmd.none

    | Some Page2 ->
        { model with CurrentRoute = Page2 }, Cmd.none

let init location =
    urlUpdate location { CurrentRoute = Page1 }

let private update msg model =
    match msg with
    | GoTo destination ->
        model, destination
                |> Route.ToUrl
                |> Navigation.newUrl

let private view model dispatch =
    let pageContent =
        match model.CurrentRoute with
        | Page1 -> str "Page 1"
        | Page2 -> str "Page 2"

    div [ ]
        [ span [ ]
            [ str "Current route force HMR several times: "
              strong [ ]
                [ pageContent ] ]
          br [ ]
          button [ OnClick (fun _ -> GoTo Page1 |> dispatch ) ]
            [ str "Go to Page1" ]
          button [ OnClick (fun _ -> GoTo Page2 |> dispatch ) ]
            [ str "Go to Page2" ] ]

open Elmish.React
open Elmish.Debug
open Elmish.HMR
open Elmish.Browser.UrlParser

let parser : Parser<Route -> Route, Route> =
    oneOf
        [ map Page1 (s "page1")
          map Page2 (s "page2")
          map Page1 top ]

Program.mkProgram init update view
|> Program.toNavigable (parseHash parser) urlUpdate
#if DEBUG
|> Program.withHMR
#endif
|> Program.withReactUnoptimized "elmish-app"
#if DEBUG
|> Program.withDebugger
#endif
|> Program.run

I don't want this package to know about HMR.

I understand the point and this is why some people in JavaScript world consider HMR a bad thing because you need to write your components/code to be compatible with how HMR works.

I'm thinking HMR could shadow Program.toNavigable and implement HMR-specific logic to release event handlers. Makes sense?

I indeed think we can benefit of F# compiler directive to shadow Program.toNavigable and any others function that's need to be aware of HMR to works. This is what we have been thinking for elmish/hmr#8

[MangelMaxime]

@et1975 Would you be ok to remove the internal from this line ?

Like that I can reference Elmish.Browser from Elmish.HMR and access the custom event name instead of writing it myself which would break if we change the name of the event.

[et1975]

Hmm, let's do some Information Hiding - wrap subscription into a function, add unsubscription here as well, that way HMR doesn't need to know details of the event(s), only that it needs to call unsubscribe.

[MangelMaxime]

Ok, I will work on adapting my fix to what you propose. Will create a PR so we can discuss the implementation details.