fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 67 forks source link

How to use memo with Elmish dispatch? #137

Closed cmeeren closed 5 years ago

cmeeren commented 5 years ago

Currently experimenting with the new memo. As I understand it, it performs a reference equality check of the props. How should this be used with the Elmish dispatch function?

MangelMaxime commented 5 years ago

If you use Elmish 3 beta, the dispatch function should now be a stable reference. At least, from what I understood.

If you don't want to use Elmish 3 beta, you will have to stabilize the dispatch function using a mutable reference to stock the dispatch function once. And then only pass the stabilize version

cmeeren commented 5 years ago

Thanks. Using Elmish 3 beta is no problem, but as I explained, for child components (where one often passes ChildMsg >> dispatch in the parent component), the child dispatch will be created each time anyway, so one has to use mutable fields in the manner you describe for each child component (all the way down). Am I correct, or have I missed something?

cmeeren commented 5 years ago

If my above reasoning is correct, then it can at least be made more elegant with a helper class:

type StableDispatch() =
  let mutable dispatch = None
  member __.GetStable dispatchIfNone =
    match dispatch with
    | Some d -> d
    | None ->
        dispatch <- Some dispatchIfNone
        dispatchIfNone

Define "globally" (outside view) as many as needed:

let child1Dispatch = StableDispatch()
let child2Dispatch = StableDispatch()

Then in view, do e.g.

div [] [
  Child1.View model.Child1 (child1Dispatch.GetStable(Child1Msg >> dispatch))
  Child2.View model.Child2 (child2Dispatch.GetStable(Child2Msg >> dispatch))
]

Does this sound reasonable? Are there more elegant ways of optimizing React in an Elmish context?

alfonsogarciacaro commented 5 years ago

If I'm not mistaken, memo defaults to React swallow comparison. You can use memoWith to control the comparison. Although it's not the most elegant solution, I've personally implemented a swallow comparison that ignores functions and works fine:

open Fable.Core
open Fable.Core.DynamicExtensions
open Fable.Import

let inline eqByRef (x: 'a) (y: 'a) = obj.ReferenceEquals(x, y)

[<Emit("typeof $0 === 'function'")>]
let isFunction (x: obj): bool = jsNative

let memoEquals (x: 'a) (y: 'a) =
    if eqByRef x y then true
    else
        (true, JS.Object.keys (x))
        ||> Seq.fold (fun eq k -> eq && (isFunction x.[k] || eqByRef x.[k] y.[k]))

// Usage
let myComponent =
    memoBuilderWith "MyComponent" memoEquals <| fun props ->
        (* render *)
cmeeren commented 5 years ago

... React swallow comparison ... I've personally implemented a swallow comparison ...

Is that a new type of equality comparison only relevant for avian domains? 🐦🦅 😆

In any case, thanks, I can see that a custom equality comparison could work. Though that complicates using custom equality when needed for other things.


By the way, fold is rather heavyweight when there are simpler alternatives (cf. Don Dyme's talk "F# Code I Love", I think he mentions it there). IMHO memoEquals is clearer and more declarative (in the non-technical sense of the word) if you write it like this (haven't compile-checked, sorry for any typos):

let memoEquals (x: 'a) (y: 'a) =
    eqByRef x y
    || JS.Object.keys x |> Seq.forall (fun k -> isFunction x.[k] || eqByRef x.[k] y.[k])

(By the way, can indexer notation be used for arbitrary/generic types like this?)


Taking a step back: I think the Fable-Elmish-React combo would benefit from users not having to remember to implement or even use such workarounds. This would reduce friction when writing Elmish apps. It seems to me that this should be solvable in Fable.React. There may exist good solutions that will "fix" this automatically without the user needing to do much at all, but if nothing else, the library could implement helpers like memoEquals or the one I suggested (which for the record could also be written using a partially applied function instead of a class, e.g. createStableRef () = ...; fun x -> ...). I don't really care about the implementation, just the end result/API and how simple it is for users. Do you have any thoughts?

cmeeren commented 5 years ago

One concrete suggestion from me: Change memo so that it actually calls memoWith behind the scenes with a comparer similar to yours above, perhaps with the added requirement that only function members called dispatch or Dispatch are ignored. That way, Elmish users can include the dispatch function in the props and won't ever have to think about stabilizing the reference, or even know that this is necessary. This should work great for 99% of Elmish use cases without being disruptive other than in edge cases (and it would be documented, of course, that any dispatch function member gets special treatment). And if they need custom equality comparison, they can use memoWith themselves.

If you're reluctant to make this part of the default memo behavior, you can name it memoElmish or something like that, and let memo keep its current definition. Or you could define something like memoFn that ignores all functions.

Thoughts?

alfonsogarciacaro commented 5 years ago

Yes, I tend to use fold too much 😅 Thanks for the tip!

We are all in this journey together, we are also learning how to use React efficiently and we don't get it right every time. That's why we're not trying to solve everything here, to reduce the chances of breaking changes if we make mistakes. The guideline is to try to have a thin layer on top of React so devs can check React documentation if necessary. If we add some extra behaviour I prefer to change the name to avoid confusions from users (see this comment).

Also, conceptually this package is independent of Elmish (we already introduced the reactiveCom to be Elmish-like but I think not many people are using it) so I'd prefer not to introduce here solutions specific for Elmish. The proper place for that would probably the Fable.Elmish.React package.

cmeeren commented 5 years ago

Thanks for taking your time to reply coherently to my ramblings. :)

I completely understand the thin-layer approach. I think solid documentation can solve this adequately. A dispatch-ignoring memo wrapper isn't more than a few lines, so I think it's okay having users who need this define it themselves as long as it's clearly documented how to do it and when it's required (or in general, when dispatch needs to be stabilized, i.e., child components).

I'm currently on the final leg of changing fable-elmish-electron-demo to demo Material-UI and non-trivial UX patterns with Elmish, so I'll try to get it right there, at least. Hoping I can get both JSS and stable child dispatch to work at the same time. (JSS requires some workarounds for now; hooks instead of HOC might solve this but isn't ready yet).

cmeeren commented 5 years ago

@alfonsogarciacaro I tried your memoEquals function, but I get an error on x.[k] and y.[k] saying

Error FS0752 The operator 'expr.[idx]' has been used on an object of indeterminate type based on information prior to this program point. Consider adding further type constraints

Is indexer notation on arbitrary types are supposed to work? (I skimmed the docs but could not find anything relevant.)

alfonsogarciacaro commented 5 years ago

Sorry, I forgot to mention you need to open Fable.Core.DynamicExtensions.

MangelMaxime commented 5 years ago

For info, I just discovered that the solution proposed by @alfonsogarciacaro

let memoEquals (x: 'a) (y: 'a) =
    if eqByRef x y then true
    else
        (true, JS.Object.keys (x))
        ||> Seq.fold (fun eq k -> eq && (isFunction x.[k] || eqByRef x.[k] y.[k]))

Doesn't work if you pass a Msg in the props.

Executing eqByRef on a DUs returns false instead of true. The solution for me was to pass a callable function instead of Msg and Dispatch separately.

cmeeren commented 5 years ago

Which use-cases require passing a Msg in the props? Isn't the Msg only supposed to go from the UI to the update loop (using dispatch), and not back again to the UI?

MangelMaxime commented 5 years ago

Well it's just that I prefe to write:

OnChange (fun _ ->
    props.Dispatch ToggleSetting
)

than

OnChange (fun _ ->
    props.OnChange()
)

Because it makes the code look the same as in "standard" view. Also, the problem was not passing Msg but a DUs case so if you pass any DUs you will have a problem.

cmeeren commented 5 years ago

I still don't get it. props.Dispatch makes sense, I use that too. But that isn't related to DUs as far as I can see. Furthermore, I can't see why eqByRef should treat DUs differently than all other objects; you can have to references to the same DU value, and eqByRef should return true. So I'm still a bit in the dark what exactly the problem is in your specific case. Which DU are you passing using props?

MangelMaxime commented 5 years ago

It has nothing to do with having it called Msg I just mentioned Msg because it was the name of my types...

type Test =
    | CaseA
    | CaseB

type RenderDisplaySettingProp =
    { Text : string
      Test : Test }

let private renderDisplaySetting =
    memoBuilderWith
        "DisplaySetting"
        Memo.equalsIgnoreFunction
        (fun props ->
            div [ ]
                [ str props.Text ]
        )

let private settings (loadedModel : LoadedModel) (dispatch : Dispatch<Msg>) =
    renderDisplaySetting
            { Text = "Marqueurs"
              Test = CaseA }

For example here as you can see I am passing a string and a DUs from Test. If I highlight the changes made by react I get this:

2019-03-19 13 38 04

Each flicker (in blue) react to change somewhere in my program model. I can't show the whole application because it is my production app.

The problem is that each time the view is re-render we create a new instance of CaseA and so eqByRef returns false. If I remove Test : Test from RenderDisplaySettingProp then I don't have anymore blue flickers.

So the conclusion of all that is you can create a string on place to pass it to the props but you can't do that for a DUs.

And it was just me learning what a refernce is and how it works.

In my case, I had to pass the Msg via the props because it's doesn't always return the same Msg in the application.

MangelMaxime commented 5 years ago

So we can safely ignore my comments about DUs. It was just me not understanding and not using memoBuilder correctly in my specific case.

cmeeren commented 5 years ago

Oh, right. It's actually string that's special. Constant strings within the same assembly are interned, and will always point to the same memory reference. It won't work for any other types, though.

Object.ReferenceEquals("a", "a");;
val it : bool = true

Update: The above is true for .NET, but AFAIK most JS implementatinos also use string interning.

alfonsogarciacaro commented 5 years ago

I have another proposal so we can cache the dispatch argument and forget about it in the comparison :)

let memoElmish name (view: 'Model->('Msg->unit)->ReactElement) =
    let mutable cachedView = None
    fun model dispatch ->
        let view =
            match cachedView with
            | Some v -> v
            | None ->
                let v = memoBuilderWith name (=) (fun m -> view m dispatch)
                cachedView <- Some v
                v
        view model
cmeeren commented 5 years ago

Love the signature! Very low friction for Elmish users. Can't wrap my head around the implementation right now, but if it works without any drawbacks, that seems like a great option. And of course a memoElmishWith function that allows overriding the comparison.

alfonsogarciacaro commented 5 years ago

Latest release (5.0.0-beta-003) includes equalsButFunctions so you can build memos while ignoring all functions in the props object (first level):

let MyComponent =
    memoBuilderWith "MyComponent" equalsButFunctions (fun ->
       ...
   )
cmeeren commented 5 years ago

Why is that needed? Wouldn't it be better to use your memoElmish suggestion above?

alfonsogarciacaro commented 5 years ago

I was talking with @vbfox at FsharpX and he suggested to do this to cover the cases where you want to pass other functions besides dispatch (not very common in Elmish but it can happen if you want to pass a function to modify the state of a parent component). The helper is an improved version of the previous proposal.