Bug in ~key?

[utkarshkukreti]

Hi!

Is ~key:_ supposed to work the same as key in React/Value and keyed in Elm? If yes, I have created a test case where some nodes' values are not updated even if the model changes. Here's the code:

open Tea.App
open Tea.Html

type counter = {id: int; value: int}

type model = counter list

let init () = [{id= 0; value= 0}; {id= 1; value= 1}; {id= 2; value= 2}]

type msg = Increment of int

let update model = function
  | Increment id ->
      Belt.List.map model (fun counter ->
          if counter.id = id then {counter with value= counter.value + 1}
          else counter )


let view model =
  let () =
    Belt.List.map model (fun counter -> counter.value) |> Belt.List.toArray
    |> Js.log
  in
  div []
    (Belt.List.map model (fun counter ->
         div ~key:(string_of_int counter.id) []
           [ span [] [text (" " ^ string_of_int counter.value ^ " ")]
           ; button [onClick (Increment counter.id)] [text "+"] ] ))


let main = beginnerProgram {model= init (); update; view}

and repo: https://github.com/utkarshkukreti/bs-tea-key-bug (just run yarn install && yarn start and then open the link webpack prints).

If you press "+", you'll see the correctly incremented value logged to the console but the text display of the counter does not update! If I remove ~key it updates fine.

[OvermindDL1]

Is ~key:_ supposed to work the same as key in React/Value and keyed in Elm?

Nope. The two things of key and uniq kind of work like keyed in Elm (unsure how it works in React/Value), but Elm's style had issues with a level of useful control that splitting up the two concerns fixed.

In general, key just means that the element will not change at all, regardless of it's data, until the key value changes too (great for skipping lots of checks!), it doesn't mess with any other checks (including uniq), and uniq means to cause an element swap when the uniq value changes.

(just run yarn install && yarn start and then open the link webpack prints).

╰─➤  yarn install
The program 'yarn' is currently not installed. You can install it by typing:
sudo apt install cmdtest
zsh: command not found: yarn

╰─➤  apt show cmdtest                                                                                                           1 ↵
Package: cmdtest
Version: 0.22-1
Priority: optional
Section: universe/python
Origin: Ubuntu
Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
Original-Maintainer: Lars Wirzenius <liw@liw.fi>
Bugs: https://bugs.launchpad.net/ubuntu/+filebug
Installed-Size: 78.8 kB
Depends: python (>= 2.7), python (<< 2.8), python-cliapp, python-ttystatus, python-markdown
Homepage: http://liw.fi/cmdtest/
Download-Size: 18.8 kB
APT-Sources: http://us.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages
Description: blackbox testing of Unix command line programs

Uh, do you actually use cmdtest? o.O
Cool if so, but I'm not seeing the necessary files in the project?

[OvermindDL1]

From looking at the code though that you posted above, key is doing what it should be, it is keying it based on the value you pass in to it and is performing no updates at all to that element or any children until it changes, and since it ('it' being counter.id) never changes then it never updates, you probably meant to put this instead:

         div ~key:(string_of_int counter.value) []

That alone would be fine since value is the only thing that changes in the children. If however you intend to dynamically add/remove things on the list then you'd want to key on both the counter.id and the counter.value both, but as it's a static length list that performs no re-ordering, then just the counter.value would be fine. :-)

EDIT: Think of key as an early-out, if key is a perfect match with the previous key value then it does a 'return' without any further checking of this tree branch. It is substantially nice to put it on the top of rarely-changing or static large branches to remove all the checks entirely, basically making them a no-op after their first render. :-)

Oh, and yes, if using a key it will also efficiently re-order nodes as well.

[utkarshkukreti]

This and this explain the use of keys. Basically if you have:

[input ~key:"foo" [] []; input ~key:"bar" [] []]

and you swap them to

[input ~key:"bar" [] []; input ~key:"foo" [] []]

it is guaranteed that the DOM nodes will be swapped underneath and their attributes will not be reassigned. This is required when doing e.g. CSS transitions or "unmanaged inputs" in React.

Is there an equivalent feature in bs-tea?

[utkarshkukreti]

Also, if this:

[input ~key:"foo" [] []; input ~key:"bar" [] []]

changes to:

[input ~key:"baz" [] []; [input ~key:"bar" [] []; input ~key:"foo" [] []]

It's guaranteed that the baz node will be recreated on every swap while foo and bar will remain the same nodes underneath.

[utkarshkukreti]

Think of key as an early-out, if key is a perfect match with the previous key value then it does a 'return' without any further checking of this tree branch.

Ah, this is actually a bit like "lazy" in Elm. "key" in all frameworks I know work like I described above. This is important if some attribute is externally modified to make sure that the nodes are moved instead of reused.

[utkarshkukreti]

Now that I think, ~key reorders the nodes correctly here, but what key in Vue/React/Elm does is to still diff the attributes and children.

[utkarshkukreti]

It looks like you do reapply attributes if the key remains the same. The only difference to make this match with the behavior of key in pretty much every framework I know is to diff the children as well. Here's a test case. If you run it you'll see that the type of the fields are set properly on the swap but the children aren't. Would you be open to this change? The not diffing value underneath if the key remains the same really belongs to lazy.

open Tea.App
open Tea.Html

type model = {order: bool; username: string; password: string}

let init () = {order= true; username= "foo"; password= "bar"}

type msg = Swap | Username of string | Password of string

let update model = function
  | Swap -> {model with order= not model.order}
  | Username username -> {model with username}
  | Password password -> {model with password}


let view model =
  let a =
    div ~key:"username" []
      [ input' [onInput (fun s -> Username s); value model.username] []
      ; text model.username ]
  in
  let b =
    div ~key:"password" []
      [ input'
          [ type' "password"
          ; onInput (fun s -> Password s)
          ; value model.password ]
          []
      ; text model.password ]
  in
  div []
    [ div [] (if model.order then [a; b] else [b; a])
    ; button [onClick Swap] [text "Swap"] ]


let main = beginnerProgram {model= init (); update; view}

[utkarshkukreti]

Sorry to spam replies but I've implemented that benchmark and the results are promising! It's faster than Elm and much faster than React.

They do require the key behavior to be the same as I described above though to classify this as a "keyed" entry.

[OvermindDL1]

Is there an equivalent feature in bs-tea?

Actually the same thing will happen in this case as well. The issue you were running in to was changing a part of the children without updating the key as well.

nodes underneath.

To guarantee a swap you use uniq instead of key (or with key).

I'm thinking maybe it is good to split these functions into 3 parts instead of 2... it keeps growing... >.>

Ah, this is actually a bit like "lazy" in Elm. "key" in all frameworks I know work like I described above. This is important if some attribute is externally modified to make sure that the nodes are moved instead of reused.

Lazy exists as well, though it does not function like key. Key is more like a mapping key (more functional terminology than web terminology) in that if it matches it skips further testing. Lazy prevents the VDOM from even being created at all if it's own key matches the past, each have their own non-overlapping purposes for various efficiency reasons.

Now that I think, ~key reorders the nodes correctly here, but what key in Vue/React/Elm does is to still diff the attributes and children.

Yeah that's why I think I need a third element. I'm thinking id to handle re-ordering, key to handle check-skipping, and uniq to handle forced element recreation...

The not diffing value underneath if the key remains the same really belongs to lazy.

In a sense, but using lazy involves making a closure, which is more efficient in some cases and less efficient in other cases...

Sorry to spam replies

Nah, all good. :-)

but I've implemented that benchmark and the results are promising! It's faster than Elm and much faster than React.

Nice! I've done quite a load of changes since my last personal benchmarking so this is good to see that it still remains faster than Elm. It should always be faster than React just because React cannot make immutable assumptions about the VDom that Elm and we can. :-)

I'm worried about that partial update test, that should be something that runs really really well... I'll need to look in to that...

Memory allocation is high because of all the list usage. If I were to switch to using array's instead of lists it would require typing [| ... |] everywhere instead of [ ... ], which is a bit higher of a syntax cost than I'd really like. However the users of tea can help minimize that cost by hoisting more of the VDom to other functions too if I don't make that breaking API change. :-)

Do you have a repo of these benchmarks that I could run?

[utkarshkukreti]

Yeah that's why I think I need a third element. I'm thinking id to handle re-ordering, key to handle check-skipping, and uniq to handle forced element recreation...

I think key should be used for reordering to make it match with pretty much every other framework (the benchmark is even called "keyed" benchmark!). id (or a better name, maybe something related to "lazy") could do what key does right now.

Memory allocation is high because of all the list usage.

Elm uses lists as well and in Elm they have a more complicated representation than what BuckleScript produces AFAIK. Maybe there's some other reason?

Do you have a repo of these benchmarks that I could run?

Yep: https://github.com/utkarshkukreti/js-framework-benchmark/tree/bucklescript-tea

Note that the numbers can't really be compared right now because this benchmark is for keyed nodes. Non keyed nodes are apparently usually faster as they can reuse more nodes than keyed ones.

[OvermindDL1]

I think key should be used for reordering to make it match with pretty much every other framework (the benchmark is even called "keyed" benchmark!). id (or a better name, maybe something related to "lazy") could do what key does right now.

Except id is more standardized to the web, you know how you set the id of a DOM element to reference that and only that element.

As well as key is used in the functional world for keying a state.

Elm uses lists as well and in Elm they have a more complicated representation than what BuckleScript produces AFAIK. Maybe there's some other reason?

Bucklescript may be doing some excess allocations, rather I think the allocations may be smaller but potentially more, which may not be bad anyway since they are all statically shaped (which is fantastic for javascript JIT'ing reasons).

Note that the numbers can't really be compared right now because this benchmark is for keyed nodes. Non keyed nodes are apparently usually faster as they can reuse more nodes than keyed ones.

Yeah key'ing is good for certain reasons, but it is harmful in others.

Subbed to it. :-)

[utkarshkukreti]

Except id is more standardized to the web, you know how you set the id of a DOM element to reference that and only that element.

Yeah that makes sense. One key (pun!) difference is that HTML's id values are unique to the page but key values are meant to be unique among siblings only. Not sure who started calling this feature "key" but ¯\(ツ)/¯.

Yeah key'ing is good for certain reasons, but it is harmful in others.

Yes, it's essential to prevent this though. The current "~key" feature in bs-tea is probably pretty unique though -- I haven't seen it before.

[OvermindDL1]

Yeah that makes sense. One key (pun!) difference is that HTML's id values are unique to the page but key values are meant to be unique among siblings only. Not sure who started calling this feature "key" but ¯(ツ)/¯.

Honestly I want it to be unique per page when possible anyway, in a later 'side-design' I want it to allow moving a key'd element anywhere around on a page instead of just among side-siblings. :-)

So id makes even more sense for that. :-)

Yes, it's essential to prevent this though. The current "~key" feature in bs-tea is probably pretty unique though -- I haven't seen it before.

Heh, it was a feature I needed in my work project... ^.^;

Also, wouldn't uniq really really be what you want to be able to ensure clearing passwords and such?

I don't actually have much experience in javascript frameworks so this design is really far more black-boxed then most, I come from the server and systems development world rather than the javascript-horror world... ^.^;

/me so SO badly wants webassembly to take over!

[utkarshkukreti]

Also, wouldn't uniq really really be what you want to be able to ensure clearing passwords and such?

I'm not sure what uniq does. It seems to be removing and recreating the node on every diff? The semantics of key as I described above in terms of other frameworks would guarantee that the nodes are swapped underneath so that the typed in password remains intact if you swap the nodes as well as the keys.

[OvermindDL1]

I'm not sure what uniq does.

uniq will recreate the node is the uniq value changes, but it otherwise diff's as normal. Thus if you want to make sure a stateful field in the DOM (like an input) is properly and fully reset/wiped, then you want to change the uniq value. :-)

The semantics of key as I described above in terms of other frameworks would guarantee that the nodes are swapped underneath so that the typed in password remains intact if you swap the nodes as well as the keys.

Yep, key does that here too currently, though I think that functionality should be moved to id (probably having key set id if id is not set for ease of use).