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

Problem with html styles?

Open playfulThinking opened this issue 7 years ago • 8 comments

If I use code like this:

  div 
  [styles
    [("position", "absolute")
    ;("left", toPX swipeyStyle##left)
    ;("top", toPX swipeyStyle##top)
    ]
  ]
  [
    img [(src Swipey.image)
    ][]
  ]

(toPX swipey... resolves to values like "200px")

I get the following js error:

[Error] Invalid_argument,-3,List.fold_left2
	fold_left2 (main.js:1019)
	patchVNodesOnElems_PropertiesApply (main.js:1594)
	patchVNodesOnElems_MutateNode (main.js:1718)
	patchVNodesOnElems (main.js:1805)
	patchVNodesOnElems (main.js:1805)
	patchVNodesIntoElement (main.js:2084)
	doRender (main.js:2366)
	handler (main.js:2415)
	handler (main.js:2309)

If instead, I use this code:

  div 
  []
  [
    img [(src Swipey.image)
          ;style "position" "absolute"
          ;style "left" (toPX swipeyStyle##left)
          ;style "top" (toPX swipeyStyle##top)
          ;style "width" Swipey.widthStr
    ][]
  ]

No problem.

Is this an error of mine, or somewhere else?

Separately, I also get messages like:

[Log] VDom:  Failed swapping properties because the property list length changed, use `noProp` to swap properties instead, not by altering the list structure.  This is a massive inefficiency until this issue is resolved. (main.js, line 1731, x2)

What is that about?

playfulThinking avatar Oct 12 '17 16:10 playfulThinking

I get the following js error:

Hmm, I thought I had a check to fail on lists of different lengths in that, probably not for efficiency reasons...

Either way, whenever a styles appears then it must always remain the same length (currently), if the length changes from run to run then it really should be (at least the parts that are added/removed) be done as individual style's.

[Log] VDom: Failed swapping properties because the property list length changed, use noProp to swap properties instead, not by altering the list structure. This is a massive inefficiency until this issue is resolved. (main.js, line 1731, x2)

Precisely what it says, if you change the length of the iteration list then the vdom has to guess and look 'around' it to see how to match things up again, this is inefficient. Basically instead of doing something like:

div
  List.append
    [ style a b
    ; style c d
    ]
    if condition then [style e f] else []
    [...]

Or so, you see how it is conditionally having an element in the list or not, this is not efficient so it has to look and check 'around' to see if it could match anything else. Instead the proper way to do the above would be:

div
  [ style a b
  ; style c d
  ; if condition then style e f else noProp
  ]

Here it either puts in the style or it puts in a noProp, which is just a placeholder meaning this 'slow' is empty but could potentially have something at other calls. This means the 'shape' of the structure does not change so it can perform a lot of optimizations that it cannot do otherwise. :-)

But still, I'm apparently missing a length check on styles, it may slow it down just a touch to add it, but it is worth it to add it instead of crashing on wrong usage, so I'll leave this open to remind me. ^.^

OvermindDL1 avatar Oct 12 '17 16:10 OvermindDL1

Excellent, thanks!

playfulThinking avatar Oct 12 '17 17:10 playfulThinking

There are also (as alternative approach to CSS in Reason):

  • https://github.com/SentiaAnalytics/bs-css
  • https://github.com/threepointone/bs-nice

stereobooster avatar Feb 21 '18 21:02 stereobooster

Yeah there are some interesting things popping up, thanks for the links! :-)

OvermindDL1 avatar Feb 21 '18 22:02 OvermindDL1

This came up for me when conditionally showing nodes: I was showing different views that happened to have a common prefix of dom nodes (they all had a div as their first child). My workaround was to return an array with the same length as the possible views in each branch of my pattern match, with all but one set to noNode, similar to solution above.

joprice avatar Jul 13 '19 02:07 joprice

with all but one set to noNode, similar to solution above.

That is precisely the point of noNode, to act as a placeholder for fast swaps! :-)

In Elm you have to do similar things at times but as it doesn't have a noNode you have to replace it with a comment node or an empty span or something.

I really should write docs... ^.^;

OvermindDL1 avatar Jul 15 '19 15:07 OvermindDL1

I thought there would be a way to use a key to indicate that these values should be swapped, but I guess that doesn't apply here. I instead sorted the values by the key and generated the array relative to that stable sort. Since the values are static, that works. But if they were dynamic, I assume that it would break in the same way. If you were writing something that was mirroring an updating list of elements, would you have to keep 'tombstones' to avoid this issue?

joprice avatar Jul 22 '19 01:07 joprice

I thought there would be a way to use a key to indicate that these values should be swapped, but I guess that doesn't apply here.

Being able to 'swap' nodes in a controlled way is a higher level thing and would require a new node type, however 'trivial' moves (moving a node up/down that is otherwise unchanged, such as inserting a node or deleting a node) are already built in, no need for keys or anything.

Tombstones are always good to have on often changing structures though (excepting the above trivial moves), you can always 'clear' them out after a period or time or after some complex operations have completed or so. :-)

In general, you shouldn't really need to worry about it unless performance becomes an issue, which it really won't in 99% of cases, or you want to keep things like inputs unchanged/moved.

OvermindDL1 avatar Jul 23 '19 17:07 OvermindDL1