DOM elements are re-created with phx-update=append/prepend

[szlend]

I was toying around with rendering SVGs with phx-update=append/prepend when I noticed it was rendering unreasonably slow when scaled up. I opened up the DOM inspector and noted some odd behavior with how container children elements are updated.

Basically I'm rendering a list of tiles on a SVG. Whenever a user reveals a tile, I append it to the temporary assign list tiles. The entire SVG can also be translated by x and y.

Findings:

• Revealing a new tile (unique element id) appends the newly created DOM element fine
• Revealing a tile that already exists in the DOM (same element id) re-creates all sibling DOM elements
• Whenever an unrelated assigns is re-rendered (e.g. x or y are updated), all tile DOM elements inside the container are re-created.

An extract from my code:

def mount(_params, _session, socket) do
  {:ok, assign(socket, x: 0, y: 0), temporary_assigns: [tiles: []]}
end

def handle_event("reveal", %{"x" => x, "y" => y}, socket) do
  tiles = [{{x, y}, tile_type_at({x, y})}]
  {:noreply, assign(socket, tiles: tiles)}
end

def handle_event("key", %{"key" => key}, socket) do
  case key do
    "ArrowUp" -> {:noreply, assign(socket, y: socket.assigns.y - 1)}
    "ArrowDown" -> {:noreply, assign(socket, y: socket.assigns.y + 1)}
    "ArrowLeft" -> {:noreply, assign(socket, x: socket.assigns.x - 1)}
    "ArrowRight" -> {:noreply, assign(socket, x: socket.assigns.x + 1)}
    _ -> {:noreply, socket}
  end
end

def render(assigns) do
  ~L"""
  <svg width="1280" height="960">
    <g id="tiles" phx-update="append" transform="translate(<%= @x %>, <%= @y %>)">
      <%= for {{x, y}, type} <- @tiles do %>
        <use id="t<%= x %>-<%= y %>" x="<%= x * 128 %>" y="<%= y * 128 %>" xlink:href="#<%= type %>" />
      <% end %>
    </g>
  </svg>
  """
end

Environment

• Elixir version (elixir -v): 1.10.1
• Phoenix version (mix deps): 1.5.3
• Phoenix LiveView version (mix deps): 0.13.3
• NodeJS version (node -v): v12.15.0
• NPM version (npm -v): 6.13.4
• Operating system: Linux
• Browsers you attempted to reproduce this bug on (the more the merrier): Firefox, Chrome
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: /

Actual behavior

All DOM elements inside the container are re-created whenever a single child is updated, or an unrelated element is updated.

Expected behavior

Phoenix LiveView should only update the DOM elements that actually changed.

[josevalim]

This is expected, unless you keep their positions or you add IDs to the elements.

LiveView (morphdom really) can compare elements as long as they are in the same position. Adding new elements to the list shifts their positions, which causes them to be "re-added". An algorithm that can consider positions shifts could be implemented, but they would also be very expensive and complex to implement. For example, what if you add in the middle? Add at the end? etc.

[szlend]

@josevalim But both the container and child elements do have ids, like in the documentation. Am I missunderstanding something?

The docs mention that elements with the same id should be updated in-place.

Also, the list is re-rendered even when it hasn't changed at all. If you look at the key event, it doesn't touch the tiles at all.

[josevalim]

Thanks, I have missed the inner IDs the first time around. Can you please provide a complete example with all phx-click and keybindings hooked up?

[szlend]

@josevalim Here's a complete example: https://github.com/szlend/liveview_append_demo

There are 3 keybinds available:

• i (increment counter)
• a (append a new element to the list)
• r (repeat appending the previous element)

Open the DOM inspector and focus on the DOM changes. Any element that has been replaced will flash yellow. You can also place a breakpoint on an element and see that it will disappear after a re-render.

Things to test:

1. a, a, i - after the increment, all elements are re-rendered even though only the value of the counter changed
2. a, a, r - after inserting a duplicate element, all elements are re-rendered when only one should (or none, since the element is equivalent)

[tropperstyle]

I'm also running into issues that seems to boil down to limitations of morphdom.

https://github.com/patrick-steele-idem/morphdom/issues/200

Unfortunately this looks to be present in every release, which is surprising as it seems like such a basic case.

[mveytsman]

@josevalim I've been able to reproduce this using @szlend's repo, and I have a reasonable explanation of what's going on. It doesn't seem to be related to the morphdom issue linked above.

The issue is in what happens to phx-update=append/prepend inside the Rendered objects

Suppose we have a live view like this:

<div id="counter"><%= @counter %></div>
<div id="list" phx-update="append">
<%= for i <- @list do %>
  <div id="<%= i %>"><%= i %></div>
<% end %>
</div>

We mount it with counter: 0, list: [1]

The rendered will be:

{
"0": "0",
"1": {
    "d": [
        [
            "1",
            "1"
        ]
    ],
    "s": [
        "<div id=\"",
        "\">",
        "</div>\n"
    ]
},
"s": [
    "<div id=\"counter\">",
    "</div>\n<div id=\"list\" phx-update=\"append\">\n",
    "\n</div>\n"
],
"c": {}
}

and our DOM is

<div id="counter">0</div>
<div id="list" phx-update="append">
  <div id="1">1</div>
</div>

We assign list: [2] in order to append an element. The diff is

{
  "1": {
    "d": [
      [
        "2",
        "2"
      ]
    ]
  }
}

This is merged into the rendered and we get

{
"0": "0",
"1": {
    "d": [
        [
            "2",
            "2"
        ]
    ],
    "s": [
        "<div id=\"",
        "\">",
        "</div>\n"
    ]
},
"s": [
    "<div id=\"counter\">",
    "</div>\n<div id=\"list\" phx-update=\"append\">\n",
    "\n</div>\n"
],
"c": {}
}

but, in patching the DOM we handle the phx-update="append" correctly and get

<div id="counter">0</div>
<div id="list" phx-update="append">
  <div id="1">1</div>
  <div id="2">2</div>
</div>

So far this is working as intended, but the problem is that if we now assign a new value to the counter we send the diff

{
"0": "1"
}

this will get merged in giving us

{
"0": "1",
"1": {
    "d": [
        [
            "2",
            "2"
        ]
    ],
    "s": [
        "<div id=\"",
        "\">",
        "</div>\n"
    ]
},
"s": [
    "<div id=\"counter\">",
    "</div>\n<div id=\"list\" phx-update=\"append\">\n",
    "\n</div>\n"
],
"c": {}
}

Now we use morphdom to transform our existing DOM into the DOM representation of the above rendered:

<div id="counter">1</div>  <--- Updated correcty
<div id="list" phx-update="append"> <-- our DOM has two children here so we reprocess the subtree
  <div id="2">2</div>
</div>

The implication seems to be that if you have a phx-update="prepend"/"append" it will be reprocessed by morphdom on any other state change in the view.

I don't have a clear enough mental model of how this all works yet to come up with the right solution. One possibility is to mark everything that's phx-update="append"/"prepend" as PHX_SKIP at some point in this render.

The other thing I'm thinking about is that it would be nice for the rendered object in the view to have the full list after the append, but I'm not sure how we'd do that. Write back to it after the dom update? Mark dynamic parts as being append/prepend in the rendered object itself?

Maybe we assign elements with phx-update CIDs and do something special there?

[josevalim]

Thanks for the detailed report. Unfortunately it is not straight-forward for us to annotate that Those dynamics are inside a prepend and append. There is no relationship between the rendered structure and the DOM and it is not trivial do build one.

However, I still wonder why the elements are being re-created. I thought that morphdom merges elements with the same ID. Or maybe is it an issue about how we handle appends/prepends?

For example, in the case you described above, is only 2 recreated or all of them?

[mveytsman]

That makes sense!

In the scenario above, morphdom is going to think that we want to delete all of the elements in the append/prepend except for the last one to be updated, and then we will run through the loop to re-order them all again in the post-morphdom step. It's going to do that on every liveview update even when we're not touching the append/prepend container. The reason they are all flashing in the dom inspector is that we go through all the elements in the container and re-append them in the correct order.

Seems like figuring out how to skip updating the container in this instance is the right way to go..

[josevalim]

In the scenario above, morphdom is going to think that we want to delete all of the elements in the append/prepend except for the last one to be updated

onBeforeNodeDiscarded we have this though:

if(el.parentNode !== null && DOM.isPhxUpdate(el.parentNode, phxUpdate, ["append", "prepend"])){ return false }

Which means it shouldn't discard any of the current elements. The re-order step should theoretically only touch the new IDs.

[mveytsman]

Sorry I wasn't as clear as I could have been - you're right we skip discarding the nodes, but we do iterate through all of them doing the reordering.

Here's branch of liveview that logs to the console every time we try to reorder an element. I made a similar demo to @szlend's that uses my branch of LiveView: https://github.com/mveytsman/liveview_append_bug_demo

To reproduce, click "Add item to list" a few times. When you click "increment counter", you will see a log like this:

We're not creating or destroying any new elements, but we are still performing an extra morphdom operation and touching/re-ordering every element in the list on every update to state outside of the container. Not as bad as it could be, but it still seems like an issue if you have a large enough list.

[josevalim]

@mveytsman perfect, thank you. Here is what we need to do. Today we already have some logic to check if we should re-order the elements on append:

              let isOnlyNewIds = isAppend && !newIds.find(id => idsBefore.indexOf(id) >= 0)

              if(!isOnlyNewIds){
                appendPrependUpdates.push([toEl.id, idsBefore])
              }

This logic skips the re-ordering if there are only new ids. But we can also skip the re-ordering if the newIds are also trailing the idsBefore. Here are some examples of when we need or don't need to re-order. I am using ++ to show the ids being appended. The left side is idsBefore, the right side is newIds.

# Distinct ids, no need to reorder:
[a, b, c] ++ [d, e, f]

# Trailing ids in order, no need to reorder:
[a, b, c] ++ [b, c]

# Trailing ids with additions at the end, no need to reorder:
[a, b, c] ++ [b, c, d]

# Non trailing ids, needs to reorder:
[a, b, c] ++ [a, c]

# Mixing trailing ids with new ids, needs to reorder:
[a, b, c] ++ [b, d, c]

My suggestion to fix this problem is: create a new function called DOM.isTrailingIDs(idsBefore, newIds) that implements the logic above, write some unit tests for it, and ship it. :D

Thanks for the help btw, very very appreciated!

[szlend]

@mveytsman Would re-ordering elements also cause the DOM breakpoints to disappear? I assumed the elements were re-created because any breakpoints I put on them disappeared after a re-render.

[mveytsman]

@josevalim this makes sense and I'm going to try to tackle it!

Does it make sense to just check if newIds has any ids that aren't in idsBefore? This covers the current test

[josevalim]

Does it make sense to just check if newIds has any ids that aren't in idsBefore? This covers the current test

I probably misunderstood what you are asking, but to be clear, this is the check we do today. :)

[mveytsman]

Heh I was about to respond saying something like this. I think that with good test coverage there are more efficiencies we can get, like reordering the small number of new elements instead of the large number of beforeIds.

Let's see what I come up with when I actually write it and test it :)

[josevalim]

@mveytsman we probably have tests for append and prepend, so that's not the part I am worried with. The challenge in testing this particular piece of code is that it is an optimization. It doesn't even feel like the nodes are recreated, they are just moved around? That's why I proposed to move the "ordering" logic to a separate function, so we can unit test it, because it would be hard to "integration test" otherwise. :) But move as you prefer!

[mveytsman]

@szlend interesting!

I (in Firefox) set a dom breakpoint on an element and then moved it somewhere else with .appendChild() and the breakpoint disappeared so I think they do go away when you move elements. Not sure if this is expected behavior by the browser, to be honest I have never used DOM breakpoints before.

[josevalim]

@mveytsman a good test would be to manually get the DOM element for an append child in console and set a value on it, like dom["foo"], and then trigger it to be relocated. If dom["foo"] disappears afterwards, then odds are indeed that it is a new element.

[mveytsman]

@mveytsman a good test would be to manually get the DOM element for an append child in console and set a value on it, like dom["foo"], and then trigger it to be relocated. If dom["foo"] disappears afterwards, then odds are indeed that it is a new element.

Just tried this and the value sticks around, so we're good! TIL you can do that!