elmish / browser

Routing and Navigation for browser apps
https://elmish.github.io/browser
Other
35 stars 20 forks source link

Clean old events handler when HMR is trigger #14

Closed MangelMaxime closed 5 years ago

MangelMaxime commented 5 years ago

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:

capture d ecran 2018-11-06 a 11 58 43
et1975 commented 5 years ago

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 commented 5 years ago

Here is a gif showing the problem:

2018-11-06 13 53 45

Source code (click to expand)

```fs 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> = 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 https://github.com/elmish/hmr/issues/8

MangelMaxime commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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