See if we can move `key` prop from a view we're wrapping into the element produced by `lazyView`

[et1975]

@alfonsogarciacaro brings up a good point that lazyView wrapped elements don't expose/inherit the key prop (not their own, nor from the view they are embedding), resulting in a dicey diff job for React when dealing with lists. I think it would be nice to see if there's key prop in the view we're wrapping and move the prop from the embedded element up into the one produced by the lazyView family of functions.

[theimowski]

I'd like to tackle this one. Any implementation ideas?

[et1975]

Need to figure out where exactly in React lifecycle we can plug this in and go from there: by the time lazy gets access to the view it's wrapping we're not in F# anymore, but the view hasn't been rendered yet. The id is somewhere in the props, but I have no idea how we can get to it. Another thing to figure out: can we copy the id as-is or do we have to generate another one based on it or do we "move" it.

[alfonsogarciacaro]

I think the main issue is we're passing a normal function to lazyView. If we used something like a React component we could pass the elements as children and then it's easier to get the key from the root one. But we may have eager evaluation issues in that case.

Anyways, it's probably simpler to move to memo components that provide the lazy functionality by default (although we could provide an equality function more adapted to Elmish as discussed here).

[et1975]

The whole point of this function is to skip recreating the children, so even if something else is easier I don't want to change that specific aspect.

[theimowski]

Was trying to find React JS examples where one would refer to a Higher-Order Component children in this way, but no success so far. Thought about accessing this.props.children in componentDidMount but it seems it doesn't contain any props for those children there

[et1975]

I think there are 2 stages that lazy needs to be aware of and handle differently:

• child view is created in VDOM, but not rendered into DOM yet
• child view is already in DOM

~In either case I don't think React gives us an API to access the props, but the key must exist in some form - either as an attribute somewhere in the VDOM hierarchy or as DOM attribute. I figure it's ? all the way to reach and read it.~ Maybe read it from VDOM and just preserve for the next round of rendering.

[theimowski]

Following works when key is present on rendered child:

type LazyProps<'model> = {
    model:'model
    render:unit->ReactElement
    equal:'model->'model->bool
    key:string  // added key prop
}

let getKey (view:'model->ReactElement) (state:'model) : string =
    let render = view state
    !!render?key

let lazyViewWith (equal:'model->'model->bool)
                 (view:'model->ReactElement)
                 (state:'model) =
    ofType<LazyView<_>,_,_>
        { render = fun () -> view state
          equal = equal
          model = state
          key = getKey view state }
        []

Downsides are:

• you call render once more
• doesn't work when key is missing on child

[et1975]

• doesn't work when key is missing on child

Don't think it needs to (or even could, in any meaningful way) - functionally nothing should depend on it being there on the lazy element, the child view should be in control.

• you call render once more

Even doing it once when we shouldn't would defeat the purpose. I can see the problem though, I think - the key is just an eager value from React perspective and I think it won't let you change it from inside the component either... Need to dig deeper and really think outside the React box.

[theimowski]

What I meant when I said it doesn't work with missing key on child was it sets the key prop to null resulting in React warnings about non-unique keys within a list.

How about always setting the key prop for lazy based on the hashCode of model? I'm not sure though it can work for all object in JS?

[MangelMaxime]

In general, a Key property should always have the same value otherwise, React will always consider it need to re-render the view under the Key graph if the key change between the frame. Indeed, if the key was key-1 and now it's key-2 React consider it's 2 separates "components".

This is indeed kind of what we want, but I don't know if React will re-use the existing DOM elements or always destroy all the elements in this case even when patching would be more efficient.

[theimowski]

Alternatively we could add one more parameter to lazyViewWith so that in addition to passing equal function one would need to provide also getHashCode function or the hash itself. Even then the question remains what should be the default hashing logic for lazyView

[theimowski]

This works if we allow to specify getHashCode as function of 'model -> int option :

[<Emit("delete $0['$1']")>]
let deleteProp x (prop : string) = jsNative

let lazyViewWith (equal:'model->'model->bool)
                 (hash:'model->int option) 
                 (view:'model->ReactElement)
                 (state:'model) =
    let key =
        match hash state with
        | Some hashCode -> string hashCode
        | None -> null

    let props =
        { render = fun () -> view state
          equal = equal
          model = state
          key = key }

    if isNull key then deleteProp props "key" // do not add key if `None`

    ofType<LazyView<_>,_,_>
        props
        []

let lazyView (view:'model->ReactElement) =
  lazyViewWith (=) (fun _ -> None) view

[et1975]

@theimowski I like where this is heading! ~Since our equals works on the model, I think it would be fine to use GetHashCode() of the model as well. We should assume the possibility of sibling lazy views working off the same model though, so maybe combine the hash of the model with the hash of the view function ((view :> obj).GetHashCode())?~ Maybe just use the view hash as the key? Semantically it makes more sense than using the model hash anyway. Also, I want this to work by default and since we are not surfacing the (optional) child's key, I'd get rid of option and always produce the key.

[theimowski]

I don't think we can use view hash - given the following:

let lazyViewWith (equal:'model->'model->bool)
                 (view:'model->ReactElement)
                 (state:'model) =
    let key = (view :> obj).GetHashCode() |> string

    let props =
        { render = fun () -> view state
          equal = equal
          model = state
          key = key }

    ofType<LazyView<_>,_,_>
        props
        []

let lazyView (view:'model->ReactElement) =
  lazyViewWith (=) view

let menuItem label page currentPage =
    lazyView (fun (page, active, label) ->
    li
      [ ]
      [ a
          [ classList [ "is-active", active ]
            Href (toHash page) ]
          [ str label ] ]
          ) (page, page = currentPage, label)

let menu (model: Model) =
  aside
    [ ClassName "menu" ]
    [ p
        [ ClassName "menu-label" ]
        [ str "General" ]

      ul
        [ ClassName "menu-list" ]
        [ ofList
            (model.Pages
              |> List.map (fun (l,p) -> menuItem l p model.CurrentPage )) ] ]

the key of each item changes every time I add a new item to model

Basically I don't think we can assume the view function or state to be a stable reference, hence I suggested adding that as option

[et1975]

Hmm, I'd never write it like that, I always apply lazy in the outside/parent view which seems to offer consistent hash.

[theimowski]

Can you post an example? Anyway the thing is if we give such API there's a chance one might misuse it - as I did apparently :)

[et1975]

Frankly I don't understand why it gives consistent hash (even if you partially apply it OR across recompiles, according to the REPL), but your way constructs a new function, mine passes a stable reference and it remains stable. Here's how I'd write this:

let menuItem (page, active, label) =
    li
      [ ]
      [ a
          [ classList [ "is-active", active ]
            Href (toHash page) ]
          [ str label ] ]

let menu (model: Model) =
  aside
    [ ClassName "menu" ]
    [ p
        [ ClassName "menu-label" ]
        [ str "General" ]

      ul
        [ ClassName "menu-list" ]
        [ ofList
            (model.Pages
              |> List.map (fun (l,p) -> lazyView menuItem (p,model.CurrentPage,l))) ] ]

[theimowski]

Even if I do it your way, (view :> obj).GetHashCode() returns different result on each model change for me. Is there something I'm missing about the consistent hash for view ? Otherwise, additional getHashCode function for lazyViewWith of 'model -> int option is the only solution I found reasonable

[et1975]

Hmm, that doesn't make sense, even in CLR world that should have been stable, unless that cast is doing something strange. Anyway, I'm convinced now that the model hash is a wrong choice - potentially it would make reconciliation harder by introducing identity that changes when it shouldn't. And perhaps hash of the view is just as wrong from that perspective. ~~I'm ready to concede the defeat and mark the lazy functions as obsolete, suggesting to use memo instead.~~