Event ~key changing does not cause event handler closure to update

[IanConnolly]

I have an issue where event handler callbacks on a div that close over information do not get replaced even when the key of the event property changes.

I'm using a patched Tea_html.onWithOptions that looks like the following:

let onWithOptions ?(key="") eventName (options: Tea_html.options) decoder =
  Tea_html.onCB eventName key (fun event ->
    if options.stopPropagation then event##stopPropagation () |> ignore;
    if options.preventDefault then event##preventDefault () |> ignore;
    event
    |> Tea_json.Decoder.decodeEvent decoder
    |> Tea_result.result_to_option
  )

I have event handlers on my DOM node that make use of this like so:

(* implementation of this isn't important, other than for understanding the structure below *)
let eventNoPropagation ~(key: string) (event : string) (constructor : mouseEvent -> msg) :
    msg Vdom.property =
  Patched_tea_html.onWithOptions ~key event
    {stopPropagation= true; preventDefault= false}
    (Decoders.wrapDecoder (decodeClickEvent constructor))

(* ...... *)
  let events =
       (* note that 'identifier' is closed over by the callback, so I want these events replaced
        * if the identifier changed *)
        [ eventNoPropagation
            "click"
            ~key:("cc-" ^ identifier)
            (fun x -> ClickCallback (identifier, x)
        ; eventNoPropagation
            "dblclick"
            ~key:("dbcc-" ^ identifier)
            (fun x ->DoubleClickCallback (identifier, x))
        ; eventNoPropagation
            "mouseenter"
            ~key:("me-" ^ identifier)
            (fun x -> MouseEnterCallback (identifier, x))
        ; eventNoPropagation
            "mouseleave"
            ~key:("ml-" ^ identifier)
            (fun x ->MouseLeaveCallback (identifier, x))
        ]
  in 
  let attrs = otherAttrs :: events in
  Html.div
    attrs
    content

The code above doesn't work some (all?, this code is pretty deep into our view but I have it fully reproducible in a number of our integration tests) of the time, and results in the the callback messages being received by an update function/model that no longer has those identifiers (because the identifier has changed and the old one is no longer around).

The code does work when I give the div a unique parameter where it is unique on the identifier, because it executes replaceNode branch of _MutateNode

I understand why the fix works, but I don't understand why it requires the fix in the first place, if that makes sense? My understanding is that because the key on the Event is changing that the old event handler should be unregistered and the new one registered. Is this a bug in key on event properties, or is this a case where I semantically want unique on the node itself?

[OvermindDL1]

I have an issue where event handler callbacks on a div that close over information do not get replaced even when the key of the event property changes.

Hmm, I have an idea how that could happen but I'm not sure it would, do you have a minimal example of it?

I'm using a patched Tea_html.onWithOptions that looks like the following:

You don't need to patch it, you can just write your own version of that function in your own module, I left it all very open on purpose. :-)

The code does work when I give the div a unique parameter where it is unique on the identifier, because it executes replaceNode branch of _MutateNode

Yeah I figured that might be where it is, but I thought I had it pretty well tested on that

I understand why the fix works, but I don't understand why it requires the fix in the first place, if that makes sense? My understanding is that because the key on the Event is changing that the old event handler should be unregistered and the new one registered. Is this a bug in key on event properties, or is this a case where I semantically want unique on the node itself?

I concur, it should indeed be properly removed, that's what the whole comparison is for.

Overall though, if the 'shape' the attributes/properties changes for a given tree branch then you generally want to use unique (unique in general should be used for the top level of any major what-would-normally-be-called-a-component as it can in general be cheaper in the DOM to rebuild a reshaped tree then to try to mutate it across, mutations are cheaper when the overall 'shape' remains fairly similar).

But still, it should work...

If you can reduce it to a minimal example that would be awesome! I'll leave this open for now to see if I can come up with a test-case myself when I get time, but looking at the code right now I really have no idea where it would be faulting, it's pretty straight-forward and I've not experienced this at my job where I use bucklescript-tea for quite a long time now...

[OvermindDL1]

I am curious, do you use the same 'name' in other nodes in the tree? Maybe in another node that might get 'mutated' to this node or vice-versa? If so that would definitely prevent the function callback from updating as names should be unique across an entire application (think of the id attribute on DOM elements).

[IanConnolly]

Thanks for responding so fast!

Hmm, I have an idea how that could happen but I'm not sure it would, do you have a minimal example of it?

We're currently in the middle of our Elm->Bucklescript port so I don't currently have time to generate a minimal example of it, but now that I know it's a bug and not a gap in my understanding I'll definitely try to book in time to get one soon.

You don't need to patch it, you can just write your own version of that function in your own module, I left it all very open on purpose. :-)

Yep for sure, that's actually what I've done -- by 'patched' here I just meant amended/based on. Incidentally, is adding optional ?key parameters to the various functions in Tea_html that don't use them a patch you'd accept?

The lack of ?key params made it super hard to realise I needed them (which then lead to learning about unique) -- it was only through reading some the other issues here that I figured it out. I'd be happy to do the work here if it's something you'd accept.

Overall though, if the 'shape' the attributes/properties changes for a given tree branch then you generally want to use unique (unique in general should be used for the top level of any major what-would-normally-be-called-a-component as it can in general be cheaper in the DOM to rebuild a reshaped tree then to try to mutate it across, mutations are cheaper when the overall 'shape' remains fairly similar).

Could you possibly restate this in some other words? I feel like I'm like 80% there to understanding it but I'm struggling to re-formulate it in my own words which is showing me that I don't actually understand it 😄

I am curious, do you use the same 'name' in other nodes in the tree? Maybe in another node that might get 'mutated' to this node or vice-versa? If so that would definitely prevent the function callback from updating as names should be unique across an entire application (think of the id attribute on DOM elements).

Theoretically no, the identifiers should be unique -- but one symptom of this bug that's fixed by the addition of unique was the closure having the identifier of an object in a different scope (whose DOM node was firmly in a cousin tree far away), which should be impossible (esp. given the click handlers are actually keyed by "<msg-abbrev>-<scope-id>-<id>").

Both that symptom (incorrect identifier) and the initial symptom in the bug report (stale identifier) can be 'fixed' by deleting the event handler cache used in eventHandler_Mutate (ie. always execute the else case of the Some case of the match).

[alfredfriedrich]

Funny thing, I just happend to have the same issue.
I'm trying to build a calendar component/module where I have a previous/next button for the months and a list with days below. When I display the previous (or next) month and select a day, the selected date is actually the date from the day of that month that was initially shown (this sounds pretty complicated)
e.g. calendar is shown with this month (Nov 2018), all the days from 1 to 30, then I click on previous month, calendar view gets updated, shows the days 1 to 31.
When I click on, lets say the 15th on the October view, the update message gets passed the 13th of November which was on that position.
I tried with the key on the callback and on the HTML button, but then the msg gets fired twice (or more, depends how many months I showed).

Here is some code:

type model = {
    visibleDate : Js.Date.t;
}
type msg = 
    | Clicked of Js.Date.t
    | NextMonth
    | PreviousMonth
[@@bs.deriving accessors]

let update model = function
    | Clicked date ->
        let () = Js.log2 "Calendar got selected date" date in
        model,Cmd.none

    | NextMonth ->
        let nextMonth = addMonths model.visibleDate 1 in
        let day = min (getDaysInMonth nextMonth) (getDate model.visibleDate) in
        let visibleDate = setDate nextMonth day in
        {model with visibleDate = visibleDate},Cmd.none

    | PreviousMonth ->
        let prevMonth = 
            let prev = Js.Date.makeWithYM 
                ~year:(model.visibleDate |> Js.Date.getFullYear)
                ~month:(model.visibleDate |> Js.Date.getMonth) () in
            subMonths prev 1 |> lastDayOfMonth in
        let day = min (getDaysInMonth prevMonth) (getDate model.visibleDate) in
        let visibleDate = setDate prevMonth day in
        {model with visibleDate = visibleDate},Cmd.none


let calendarNav model = 
    let open Html in
    div [ class' "calendar-nav" ]
        [ div 
            [ class' "calendar-nav-prev-month" ]
            [ button 
                [ class' "button is-primary"; onClick PreviousMonth ] 
                [ span [ class' "icon"] 
                    [ i [ class' "fas fa-chevron-left" ] [] ] 
                ]
            ]
        ; div [ class' "calendar-month" ] [ text @@ format model.visibleDate "MMMM YYYY" ]
        ; div 
            [ class' "calendar-nav-next-month"; onClick NextMonth ]
            [ button 
                [ class' "button is-primary" ] 
                [ span [ class' "icon"] 
                    [ i [ class' "fas fa-chevron-right" ] [] ] 
                ]
            ]
        ]
let dateView date =
    let open Html in
    let dateString = Js.Date.toString date in
    let cb = Vdom.onCB "click" {j|$(dateString)-cb|j} (fun _ -> Clicked date|. Some ) in
    node "div" ~key:{j|$(dateString)-div|j} [ class' "calendar-date"; id {j|$(dateString)-div|j} ]
        [ node "button" ~key:{j|$(dateString)-button|j} 
            [ class' "date-item"; cb; id {j|$(dateString)-button|j} ] 
            [ date |> getDate |> string_of_int |> text ]
        ]

let calendarView model =
    let open Html in
    let dayLabels = 
        Belt.Array.make 7 @@ startOfWeek model.visibleDate
        |. Belt.Array.mapWithIndex (fun i d ->
            format (addDays d i) "ddd"
        ) in
    let startDay = 
        model.visibleDate |> startOfMonth |> startOfWeek in
    let endDay = 
        model.visibleDate |> endOfMonth |> endOfWeek in
    let days = 
        let size = 1 + differenceInDays endDay startDay in
        Belt.Array.make size startDay
        |. Belt.Array.mapWithIndex (fun i d ->
            addDays d i
        ) in
    div [ class' "calendar-container" ]
        [ div [ class' "calendar-header" ]
            (Belt.Array.map dayLabels (fun day -> 
                node "div" ~key:day [ class' "calendar-date" ] [ text day ]
            ) |> Belt.List.fromArray)
        ; div [ class' "calendar-body" ]
            (Belt.Array.map days (fun day ->
                dateView day
            ) |> Belt.List.fromArray)
        ]
   

let view model =
    let open Html in
    let navView = calendarNav model in
    div [ class' "calendar" ] 
        [ navView; (calendarView model) ]

Note, this is "ported" from https://gist.github.com/stevensacks/79c60d0f8b1f8bc06b475438f59d687e

I'm curious about that unique thing, but its already to late, will try to reread the issue and the others with proper open eys tomorrow :-)

[alfredfriedrich]

Ok, after rereading this and other issues, I understood that I also have to key the NextMonth/PreviousMonth onClick callbacks. Now the click on a date gets only triggered once and with the right date.

e.g.

let genKeyForMonth date =
    format date "YYYYMM"

let genKeyForDay date = 
    format date "YYYYMMDD"

let onMonthClick date msg =
    let key = genKeyForMonth date in
    Html.onCB "click" key (fun _ -> Some msg)

let onDayClick date msg =
    let key = genKeyForDay date in
    Html.onCB "click" key (fun _ -> msg date |. Some)

and in the view

let calendarNav model = 
(* ... *)
             button 
                [ class' "button is-primary"
                ; (model.visibleDate |. genPreviousMonth |. onMonthClick PreviousMonth)
                ] 
                [ span [ class' "icon"] 
                    [ i [ class' "fas fa-chevron-left" ] [] ] 
                ]
(* ... *)
let dateView date =
    let open Html in
    div [ class' "calendar-date" ]
        [ button 
            [ class' "date-item"
            ; (date |. onDayClick clicked)
            ] 
            [ date |> getDate |> string_of_int |> text ]
        ]

So the behaviour with the handlers not getting unregistered was due to not "inform" the VDOM about that the month changed, which is now done with proper keying.
One thing I do not fully understand is, where to apply the key, on the element/node that holds the onCB handler, one the handler itself, somewhere further up the tree?

[OvermindDL1]

Yep for sure, that's actually what I've done -- by 'patched' here I just meant amended/based on. Incidentally, is adding optional ?key parameters to the various functions in Tea_html that don't use them a patch you'd accept?

Absolutely. :-)

The original Elm API didn't have them hence the lack of their initial inclusion, but as they are optional then they are backwards compatible anyway so it definitely makes sense.

The lack of ?key params made it super hard to realise I needed them (which then lead to learning about unique) -- it was only through reading some the other issues here that I figured it out. I'd be happy to do the work here if it's something you'd accept.

Yeah the lack of Elm having them had some 'fun' as well (required use of lazy and other constructs to work around). But definitely would accept!

Could you possibly restate this in some other words? I feel like I'm like 80% there to understanding it but I'm struggling to re-formulate it in my own words which is showing me that I don't actually understand it smile

Heh, just in general you want to use unique when that changing will change the entire DOM at this point, I.E. it is conceptually a different 'thing'/'component'. Not using unique allows for mutation, which generally should only be used for component transformations of the same or similar 'type'. Like in the all-popular TODO App example you would have each 'todo' entry not be unique, because although they are different entries they are still all the same 'shape', but the container for them all would have a unique entry, as would the overall holder of the textfield and so forth. Although in this TODO App example they are less needed as you only have one 'page' of the given app, it's definitely more important in larger apps. :-)

Theoretically no, the identifiers should be unique -- but one symptom of this bug that's fixed by the addition of unique was the closure having the identifier of an object in a different scope (whose DOM node was firmly in a cousin tree far away), which should be impossible (esp. given the click handlers are actually keyed by "<msg-abbrev>-<scope-id>-<id>").

Both that symptom (incorrect identifier) and the initial symptom in the bug report (stale identifier) can be 'fixed' by deleting the event handler cache used in eventHandler_Mutate (ie. always execute the else case of the Some case of the match).

Well that's odd, considering the cache exists in part on the VDom node itself and on the actual DOM element itself, so being a scoping issue sounds exceedingly odd indeed... Especially as the key for the event callback should prevent the 'then' case from ever executing...

Ok, after rereading this and other issues, I understood that I also have to key the NextMonth/PreviousMonth onClick callbacks. Now the click on a date gets only triggered once and with the right date.

Yeah, in 'thought' every DOM Event Callback should have a globally unique ID. Elm does not do that (and it's had some "fun" issues related to it in the past) but trying to hold to it's API as close as I really could definitely enforces some mis-patterns. I'm wondering if it's time to dump the Elm API for the thought of actually fixing the interfaces, the compiler would definitely let people know what needs to be changed so...

One thing I do not fully understand is, where to apply the key, on the element/node that holds the onCB handler, one the handler itself, somewhere further up the tree?

Well the event callbacks themselves should have the ID as it allows the most fine-grained mutations. Putting a key on a higher thing is more like lazy for Elm, those matching means it skips all mutation within (I.E. you need to change the key on every child update), and unique means that if the string changes then dump the entire element and all children and rebuild from scratch, else if the same then perform mutation as normal.

[alfredfriedrich]

Well the event callbacks themselves should have the ID as it allows the most fine-grained mutations.

With ID you mean keyor unique? As far as I understand it, the difference wouldn't matter, as it doesn't have any children.

Putting a key on a higher thing is more like lazy for Elm, those matching means it skips all mutation within (I.E. you need to change the key on every child update), and unique means that if the string changes then dump the entire element and all children and rebuild from scratch, else if the same then perform mutation as normal.

Thanks for that explanation. Apart from that, I would probably be comfortable with dumping the ELM API.

[OvermindDL1]

With ID you mean keyor unique? As far as I understand it, the difference wouldn't matter, as it doesn't have any children.

I mean the literal id on the event callback variant, not the key or unique on the element, the element should generally not have either (maybe a unique depending on situation, but even then generally not). Thus on the https://github.com/OvermindDL1/bucklescript-tea/blob/master/src-ocaml/tea_html.ml#L262 line of let onCB eventName key cb = onCB eventName key cb the key on this thing, this is what identifies and allows for event mutation when the key changes (I should probably rename this to id... to make it more explicit, but Elm calls these things 'key'...). Every event should have a unique key in the general case.

[alfredfriedrich]

Thus on the https://github.com/OvermindDL1/bucklescript-tea/blob/master/src-ocaml/tea_html.ml#L262 line of let onCB eventName key cb = onCB eventName key cb the key on this thing

Got it, key is id, except when its key ;-)
nah.. just messin around :-)

Every event should have a unique key in the general case

This should be somewhere bold in the docs/source imho. For a beginner who learns from reading the code (yeah I'm looking at you, Vdom.ml) this is not easy to comprehend, especially as all ~key arguments are optional, as @IanConnolly mentioned above. But yes, it's also kind of fun, learned a lot from that :-)

[OvermindDL1]

Got it, key is id, except when its key ;-)
nah.. just messin around :-)

Yeah it bugged me writing it too but I was trying to follow the Elm naming conventions at the time (not sure I care so much anymore)... ^.^;

This should be somewhere bold in the docs/source imho. For a beginner who learns from reading the code (yeah I'm looking at you, Vdom.ml) this is not easy to comprehend, especially as all ~key arguments are optional, as @IanConnolly mentioned above. But yes, it's also kind of fun, learned a lot from that :-)

Well it's optional within it's own unique scope, but even then it's safer to just always have it (that is why I want to make it very not optional in a few cases).