bucklescript-tea icon indicating copy to clipboard operation
bucklescript-tea copied to clipboard

VDOM bug: patchVNodesOnElems_PropertiesApply_Mutate

Open jackalcooper opened this issue 7 years ago • 7 comments

match clause Style s as _newProp of Vdom.patchVNodesOnElems_PropertiesApply_Mutate can't handle the case two lists have different lengths. It is because OCaml's List.fold_left2 will raise Invalid_argument if the two lists are determined to have different lengths.

jackalcooper avatar Sep 04 '17 04:09 jackalcooper

fork to reproduce it is here

jackalcooper avatar Sep 04 '17 06:09 jackalcooper

This can be walked around by setting drag's xy as unique but I still opened an issue. Although after adding unique there is no runtime exception, this will cause the problem that animation created by transition in css become useless. So I suggest that we should have another implementation of this function and the new implementation should be robust and performant.

jackalcooper avatar Sep 04 '17 06:09 jackalcooper

Yeah it is entirely expected that the two lists should always be the same length, always, as it is faster, changeable styles should be a standalone style swap and/or noProp, though I guess it should support it both ways (with a slight speed hit by doing so though). I'll get it added when I next can, the speed hit I guess is fine, though for non-changing lists it is needless, hmm, I might have an idea around that anyway too... ^.^

OvermindDL1 avatar Sep 05 '17 17:09 OvermindDL1

Yeah, I got the point. The thing is that it could be tricky to add some css animation while at the same time you also need to pay attention to DOM rebuild by updating unique. Is it a good idea to add an api accept a long style string?

jackalcooper avatar Sep 06 '17 01:09 jackalcooper

I have update the code following the requirement of identical lengths of style tuple lists. If you think it is worth including the test you can merge the PR.

jackalcooper avatar Sep 06 '17 01:09 jackalcooper

Is it a good idea to add an api accept a long style string?

Just the single style, not styles is really for that purpose. :-)

I might include it, always good to have cases to test, bit behind at work due to an emergency that closed us down yesterday, but will get to it soon. ^.^;

OvermindDL1 avatar Sep 06 '17 14:09 OvermindDL1