elmish / hmr

Hot Module Replacement for Elmish apps
https://elmish.github.io/hmr
Other
28 stars 9 forks source link

Shadow Program.run #11

Closed MangelMaxime closed 5 years ago

MangelMaxime commented 5 years ago

@et1975 Here is another shadowing thing to fix Elmish when using HMR. This time we are shadowing Program.run.

Context

The features of the application created to illustrate the HMR problem are:

On the next GIF I am running the application, and triggering several HMR change.

Old version 2018-11-08 15 53 59

Easy way to spot the problem

If you look at the developer console, you can see a lot of message:

Message from instance n°  0
Message from instance n°  1
Message from instance n°  2
Message from instance n°  3
Message from instance n°  0
Message from instance n°  1
Message from instance n°  2
Message from instance n°  3
Message from instance n°  0
Message from instance n°  1
Message from instance n°  2
Message from instance n°  3
...

This is because I am logging each time an application received a Msg:

let private update msg model =
    Browser.console.log("Message from instance n° ", instanceID)
    match msg with
    // ...

As you can see all the application are working in parallel here.

What does it mean ?

Each time we trigger an HMR change, we are recreating a new Elmish application but the previous application is still running.

This means:

When an Elmish application received a message, it's running the update function and then updating the view. This is why you see the Instance rank: switching between 0, 1, 2, 3, etc.

It's the same things that make the input value flicker because it's not always the same Elmish application that is being rendered (they all try to render using React at the same node).

So when I write Maxime comment tu vas ? we have something like that happening:

View#1 means view of the first Elmish app

Running when using shadowing with Elmish.HMR.Program.run

New version 2018-11-08 15 56 40

Has you can see now the view doesn't flicker and stay stable with Instance rank: 4. Also the input is now working properly.

Implementation detail

Because we can't cancel a MailboxProcessor I needed, to wrap the Model to detect if it's Active or Inactive. Even if we cancel a MailboxProcessor I think we would have some errors coming from the subscription if they are sort of a timer like here (they have a local reference to dispatch).

I think we should be able to remove Program.withHMR and directly implements the state restoration from Elmish.HMR.Program.run do you think it's a good idea. The pros being that we are making it simpler to use HMR just open Elmish.HMR after other Elmish module and you will have HMR working.

I have been thinking about how we implements the shadowing things and wonder if we shouldn't do something like that:

#if DEBUG
/// Start the dispatch loop with `unit` for the init() function.
let inline run (program: Program<unit, 'model, 'msg, 'view>) =
    runWith () program
#else    
let inline run (program: Program<unit, 'model, 'msg, 'view>) =
    Elmish.Program.run program
#endif

The idea being if we are in DEBUG then use the custom implementation if not we sure inline the call to the standard API. I am thinking about that because if users do not use DEBUG mode then we are realying on him to have open the correct modules/packages etc. And I don't think it's a good idea.

et1975 commented 5 years ago

We're in uncharted territory here - Fable is kinda unique in F# world because compiling the dependency's sources makes behavior adjustments like this possible. I like it in this specific case. I also like the fact that we're reducing our ordering problem (quick, what should go where when augmenting a Program with navigation, React and HMR?).

MangelMaxime commented 5 years ago

we're reducing our ordering problem (quick, what should go where when augmenting a Program with navigation, React and HMR?).

Here is the code used, it's still in the same order as before.

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

If we manage the HMR state in the shadow Program.run we can have something like that which is better IMO and better reflect what we are trying to do by shadowing things.

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

We're in uncharted territory here - Fable is kinda unique in F# world because compiling the dependency's sources makes behavior adjustments like this possible. I like it in this specific case.

Do you mean you like this proposition:

I have been thinking about how we implements the shadowing things and wonder if we shouldn't do something like that:

#if DEBUG
/// Start the dispatch loop with `unit` for the init() function.
let inline run (program: Program<unit, 'model, 'msg, 'view>) =
    runWith () program
#else    
let inline run (program: Program<unit, 'model, 'msg, 'view>) =
    Elmish.Program.run program
#endif

The idea being if we are in DEBUG then use the custom implementation if not we sure inline the call to the standard API. I am thinking about that because if users do not use DEBUG mode then we are realying on him to have open the correct modules/packages etc. And I don't think it's a good idea.

Or like the idea of just having

#if DEBUG
/// Start the dispatch loop with `unit` for the init() function.
let inline run (program: Program<unit, 'model, 'msg, 'view>) =
    runWith () program
#endif
et1975 commented 5 years ago

I know which order they are supposed to go in :) I was just pointing out that we have a problem with a user potentially not knowing what the correct order is. I have a solution in mind , but for now reducing the uncertainty to 3 (from current 4) items to order is a small win.

I think we need both branches of #if DEBUG

MangelMaxime commented 5 years ago

I know which order they are supposed to go in :)

Ahah sorry I was thinking that you were thinking the order could have changed ^^

Ok, I will try to come up with something in the Elmish package to avoid re-implementing the loop in Elmish.HMR

MangelMaxime commented 5 years ago

@et1975 I rewrote the PR commits and publish a new version to shadow Program.run the good news is that we don't need to do any modification to Fable.Elmish.

I used "program composition" to wire the HMR logic. This has a benefit to hide the extra layer added to the Model for managing the HMR state. So it's not visible from debugger, console trace etc.

I also needed to shadow all the function from Fable.Elmish.React first in order to fix #8 and also because of what I explained here.

I need to do some more tests before merging (starting to be late here ^^) and would like your review on how I am shadowing all the Fable.Elmish.React functions.

The theory is that now if you open Elmish.HMR as the last statement then it will go on HMR mode if available.

Same if you open Elmish.HMR after Elmish.React then all the lazyView are disabled in dev mode.

et1975 commented 5 years ago

Since these are going to be breaking changes we should probably bump the major in semver fashion.

MangelMaxime commented 5 years ago

Since these are going to be breaking changes we should probably bump the major in semver fashion.

For sure, and will need to update the docs site to include explanation for the transition and how to use the shadow features.

et1975 commented 5 years ago

I don't think just throwing out laziness is a good idea. Maybe we can curry the HMR state in? Also, we can implement react's withReactHydrate,withReact,withReactUnoptimized by introducing "Using" version that takes the root element construction function (like we do with debounce function in debugger), which in Elmish.React will take its own lazyView2With but in HMR will use yours. That way you won't have to reimplement the logic of these functions.

MangelMaxime commented 5 years ago

I don't think just throwing out laziness is a good idea.

Ah yes, I was just thinking about getting HMR to work but indeed now we never have laziness at the root level which is a bad idea.

In the past, I was able to fix this problem by including an incremental value in the model. Like that each time I had an HMR triggered I was incrementing the HMRCount by 1 and lazyView was re-rendering the view.

IMO same goes with current implementation of the lazyView. We are disabling the lazyView all the time instead of only forcing a re-render when triggered by HMR. I have an idea to keep lazyView doing their job but being able to force a render if we are updating after an HMR call.

Also, we can implement react's withReactHydrate,withReact,withReactUnoptimized by introducing "Using"

Ok, will try create to it and send a PR to the right repo for it.

MangelMaxime commented 5 years ago

@et1975

So in theory we can do something like that:

type LazyView<'model>(props) =
    inherit Component<LazyProps<'model>,int>(props)

    let hmrCount =
        if isNull Browser.window?HMR_Count then
            0
        else
            unbox<int> Browser.window?HMR_Count

    do base.setInitState(hmrCount)

    override this.shouldComponentUpdate(nextProps, _nextState) =
        if isNull Browser.window?HMR_Count then
            not <| this.props.equal this.props.model nextProps.model
        else
            let currentHmrCount : int = Browser.window?HMR_Count
            if currentHmrCount > this.state then
                this.setState(fun _prevState _props ->
                    currentHmrCount
                )
                // An HMR has been triggered between two frames we force a rendering
                true
            else
                not <| this.props.equal this.props.model nextProps.model

    override this.render () =
        this.props.render ()

The idea is to re-implements the LazyView components from Elmish.React.

And because HMR make available a global variable window.HMR_Count which contains the current HMR rank then we capture the value when initializing the component and then on each "update" we check if the value changed or not. If it changed, then we force the view to reload.

[<AutoOpen>]
module Common =
    open Fable.Import.React

    type LazyProps<'model> = {
        model:'model
        render:unit->ReactElement
        equal:'model->'model->bool
    }

    module Components =
        type LazyView<'model>(props) =
            inherit Component<LazyProps<'model>,int>(props)

            let hmrCount =
                if isNull Browser.window?HMR_Count then
                    0
                else
                    unbox<int> Browser.window?HMR_Count

            do base.setInitState(hmrCount)

            override this.shouldComponentUpdate(nextProps, _nextState) =
                if isNull Browser.window?HMR_Count then
                    not <| this.props.equal this.props.model nextProps.model
                else
                    let currentHmrCount : int = Browser.window?HMR_Count
                    if currentHmrCount > this.state then
                        this.setState(fun _prevState _props ->
                            currentHmrCount
                        )
                        // An HMR has been triggered between two frames we force a rendering
                        true
                    else
                        not <| this.props.equal this.props.model nextProps.model

            override this.render () =
                this.props.render ()

    #if DEBUG
    /// Avoid rendering the view unless the model has changed.
    /// equal: function to compare the previous and the new states
    /// view: function to render the model
    /// state: new state to render
    let lazyViewWith (equal:'model->'model->bool)
                     (view:'model->ReactElement)
                     (state:'model) =
        ofType<Components.LazyView<_>,_,_>
            { render = fun () -> view state
              equal = equal
              model = state }
            []
    #else
    /// Avoid rendering the view unless the model has changed.
    /// equal: function to compare the previous and the new states
    /// view: function to render the model
    /// state: new state to render
    let inline lazyViewWith (equal:'model->'model->bool)
                     (view:'model->ReactElement)
                     (state:'model) =
        Elmish.React.Common.lazyViewWith
    #endif

    // Here we have all the  `lazyView` functions shadowed
    // ..

    // This function should be inside `Elmish.React.Internal.withReactUnoptimizedUsing`
    let withReactUnoptimizedUsing lazyView placeholderId (program:Elmish.Program<_,_,_,_>) =
        let setState model dispatch =
            Fable.Import.ReactDom.render(
                lazyView (fun x y -> obj.ReferenceEquals(x,y)) program.view model dispatch,
                document.getElementById(placeholderId)
            )

        { program with setState = setState }

    // And here we shadow `Elmish.React.withReactUnoptimized`
    #if DEBUG
    let withReactUnoptimized placeholderId (program:Elmish.Program<_,_,_,_>) =
        withReactUnoptimizedUsing lazyView2With placeholderId program
    #else
    let inline withReactUnoptimized placeholderId (program:Elmish.Program<_,_,_,_>) =
        Elmish.React.Program.withReactUnoptimized placeholderId program
    #endif    

So if we go this way, we keep lazyness working all the time which is really important because you want to have it working in order to implements it/test it using React dev tools & co.

And also, we implements the Program.view logic only once inside Elmish.React

IMHO this is promising, I will keep testing the shadowed LazyView components to make sure it's really working as expected. For now, I only tested it as the root level of the app I want to test it deeper into the view.

MangelMaxime commented 5 years ago

Ok @et1975 so my previous LazyView components was a bit buggy because I was not using an object to store the state but an int.

type LazyView<'model>(props) =
    inherit Component<LazyProps<'model>,LazyState>(props)

    let hmrCount =
        if isNull Browser.window?HMR_Count then
            0
        else
            unbox<int> Browser.window?HMR_Count

    do base.setInitState({ HMRCount = hmrCount})

    override this.shouldComponentUpdate(nextProps, _nextState) =
        if isNull Browser.window?HMR_Count then
            not <| this.props.equal this.props.model nextProps.model
        else
            let currentHmrCount : int = Browser.window?HMR_Count
            if currentHmrCount > this.state.HMRCount then
                this.setState(fun _prevState _props ->
                    { HMRCount = currentHmrCount }
                )
                // An HMR has been triggered between two frames we force a rendering
                true
            else
                not <| this.props.equal this.props.model nextProps.model

    override this.render () =
        this.props.render ()

Demo

See how the line just after Elmish.React.Common.lazyView2 change only when I am actually modify the current page value.

When the line after Elmish.HMR.Common.lazyView2 always get updated to the correct DOM when HMR has been applied.

2018-11-09 21 40 44