OvermindDL1 / bucklescript-tea

TEA for Bucklescript
Other
601 stars 45 forks source link

VDOM bug: patchVNodesOnElems_PropertiesApply_Mutate #48

Open jackalcooper opened 6 years ago

jackalcooper commented 6 years ago

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 commented 6 years ago

jump to the line in the file

jackalcooper commented 6 years ago

fork to reproduce it is here

jackalcooper commented 6 years ago

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.

OvermindDL1 commented 6 years ago

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... ^.^

jackalcooper commented 6 years ago

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 commented 6 years ago

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.

OvermindDL1 commented 6 years ago

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. ^.^;