Make the optional `key` argument required?

[jackalcooper]

When I was creating a pagination with bucklescript-tea, I found that I could never get the "next" and "previous" button right. And I found a bug. It is the case if the msg for onClick is a if condition it will always go with one branch (and the value of first run, but in this case below it doesn't require a parameter for the Increment and Decrement msg). Here is the minimum counter example to reproduce it. The behavior should be that the counter will go down to 2 if it reach 3:

main_counter.ml:

open Tea.App
open Tea.Html

type msg =
  | Increment
  | Decrement
  | Reset
  | Set of int

let update model = function
  | Increment -> model + 1
  | Decrement -> model - 1
  | Reset -> 0
  | Set v -> v

let view_button title msg =
  button
    [ onClick msg
    ]
    [ text title
    ]

let view model =
  div
    []
    [ span
        [ style "text-weight" "bold" ]
        [ text (string_of_int model) ]
    ; br []
    ; view_button "Increment" (
      (*Here comes the bug*)
      if model >= 3 then
      Decrement
      else
      Increment
    )
    ; br []
    ; view_button "Decrement" Decrement
    ; br []
    ; view_button "Set to 42" (Set 42)
    ; br []
    ; if model <> 0 then view_button "Reset" Reset else noNode
    ]

let main =
  beginnerProgram {
    model = 0;
    update;
    view;
  }

I tried it with Elm and on Elm it works fine:

-- Read more about this program in the official Elm guide:
-- https://guide.elm-lang.org/architecture/user_input/buttons.html

import Html exposing (beginnerProgram, div, button, text)
import Html.Events exposing (onClick)

main =
  beginnerProgram { model = 0, view = view, update = update }

view model =
  div []
    [ button [ onClick Decrement ] [ text "-" ]
    , div [] [ text (toString model) ]
    , button [ onClick 
          (if model >= 3 then
            Decrement
          else
            Increment) 
        ] [ text "+" ]
    ]

type Msg = Increment | Decrement

update msg model =
  case msg of
    Increment ->
      model + 1

    Decrement ->
      model - 1

[OvermindDL1]

That in this case is entirely expected (I really need to write documentation...). ^.^

The callback is keyed based on the key that is passed to the onClick function, which by default the key is the empty string of "", thus it always matches. If you want to do the same event (click in this case) but with different callback functions based on the model then you need to pass it the optional key argument to the onClick method as well, so it knows 'when' to reset the callback, otherwise it would have to reset the callback on every-single-call to the onClick handler. Sadly in Javascript every-single-event-callback will not test as equal (since javascript does not have the ability to test what a callback 'does'). Event callbacks here have the ability to handle things like preventDefault() on an event based on model and event state (which Elm does not have that capability, it is either all or nothing), but to handle that we need to know when to replace the callback. Elm does it based on the Variant that is passed in, but instead here we have a function so that you have the ability to return different events based on the event itself as well as handle things like preventDefault() and other features that Elm does not have.

However, a work-around I can think of that would let your code work in this case (any case where the event gives the message no information, so a static Variant like onClick takes) would be to magically cast it to a string (which I can write code to do so safely, but that is... not quite right to do), or I could make a new Event Handler type in the VDom that can hold a variant so it can do direct comparisons instead (like elm does).

However, the proper way to change the message type on an event that will always work is to 'turn on or off the events' instead, so writing it like this:

let view model =
  div
    []
    [ span
        [ style "text-weight" "bold" ]
        [ text (string_of_int model) ]
    ; br []
    ; if model >= 3 then view_button "Increment" Decrement else noNode
    ; if model < 3 then view_button "Increment" Increment else noNode
    ; br []
    ; view_button "Decrement" Decrement
    ; br []
    ; view_button "Set to 42" (Set 42)
    ; br []
    ; if model <> 0 then view_button "Reset" Reset else noNode
    ]

Specifically because it holds places in the DOM for updating efficiency as well.

However your above case really should work, the internal details that it is holding a function pointer instead of a user type is really internal data, and there are multiple ways to work around it. I will probably do so by adding another type to the VDom itself that holds an arbitrary object to return or via string munging the variant in the handler itself to become a useful 'key'. Hmm, actually I might be able to alter the vdom to make the event 'key' a user-definable type instead of just a string, that might actually be the best way to do this... PR's accepted otherwise I'll likely get to it tomorrow or Monday. :-)

[OvermindDL1]

Well, it is 'kind of' documented at https://github.com/OvermindDL1/bucklescript-tea/blob/master/src/vdom.ml#L77 ^.^

[OvermindDL1]

@jackalcooper For note, Elm does have the same kind of issues, a few examples:

https://github.com/elm-lang/virtual-dom/issues/24 https://groups.google.com/forum/#!topic/elm-discuss/HdOGC6mpDMI

And a whole variety of other things ranging from various attributes, properties, events, and more, all solved via using Elm's Keyed attribute. I instead baked the 'key' concept into the nodes themselves since it ended up being so ubiquitous (defaulting to the empty string). Basically if anything can change that cannot be checked internally (callbacks and functions in other words) then they key is necessary both in Elm and here to work around. But as stated, I have ideas on how to minimize the 'front-end' requirements of using keys (unlike elm) so I think I can remove 95% of those issues (all but those where the front-end passes a callback directly instead of it being wrapped up in an interface), including every example issue you've had so far, which will definitely make it easier to use. :-)

[OvermindDL1]

I broke API and bumped version to 0.2.0 to allow a 'msg type as an event and changed the events that can use it to use it. I've not touched the optional ?key argument yet on the others, debating on making it not optional still, but your above case and other issue is now resolved anyway. :-)

I'm going to leave this open a bit longer until I decide to make the 'key' on the others forced non-optional or not, input is wanted. :-)