Phoenix_live_view: Unexpected behaviour with multiple comprehensions in the same container and phx-append.

[cthulahoops]

Environment

• Elixir version (elixir -v): 1.10.4
• Phoenix version (mix deps): 1.5.4
• Phoenix LiveView version (mix deps): 0.14.6 (Current master: c2fc891ceebf9cdc96d49a215d63dbe6e7c81249)
• NodeJS version (node -v): v10.19.0
• NPM version (npm -v): 6.14.5
• Operating system: Ubuntu
• Browsers you attempted to reproduce this bug on (the more the merrier): Firefox
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no:

Demo LiveView

defmodule DemoAppWeb.ListTemp4Live do
  use DemoAppWeb, :live_view

  def render(assigns) do
    ~L"""
    <button phx-click="add_item">Add Item to List</button>
    <div id="container" phx-update="append">
      <%= if @red_item do %>
        <div style="background-color: red" id="<%= @red_item %>">
        <%= @red_item %>
        </div>
        <% end %>
      <%= for item <- @list do %>
      <div id="<%= item %>">
       <%= item %>
       <button phx-click="make_red" phx-value-item="<%= item %>">Make Red</button>
      </div>
      <% end %>
    </div>
    """
  end

  def mount(_params, _session, socket) do
    {:ok,
     socket
     |> assign(:list, [1,2,3])
     |> assign(:next_index, 4)
     |> assign(:red_item, nil),
    temporary_assigns: [list: [], red_item: nil]
    }
  end

  def handle_event("add_item", _, socket) do
    i = socket.assigns.next_index

    {:noreply,
     socket
     |> assign(:list, [i])
     |> assign(:next_index, i + 1)}
  end

  def handle_event("make_red", %{"item" => item}, socket) do
    IO.inspect(item)
    {:noreply,
      socket
      |> assign(:red_item, item)}
  end
end

Expected behavior

We're in phx-update=append mode, so when we add an element with an existing id, we expect it to be updated in place. In this case, we expect the "Make Red" button to make the div red, and the contents to just be the number.

Actual behavior

Instead an extra red div is added at the beginning of the container. Further attempts to add elements result in multiples of the same item in very strange orders.

You can get the desired behaviour back by wrapping the for loop in an else block as shown below. We discovered this behaviour while writing documentation
for phx-remove, and this bug is more likely to be triggered in that context.

Eg:

    <div id="container" phx-update="append">
      <%= if @red_item do %>
        <div style="background-color: red" id="<%= @red_item %>">
        <%= @red_item %>
        </div>
    <% else %>
      <%= for item <- @list do %>
      <div id="<%= item %>">
       <%= item %>
       <button phx-click="make_red" phx-value-item="<%= item %>">Make Red</button>
      </div>
      <% end %>
    <% end %>
    </div>

We're planning to investigate this bug next, but didn't want to delay opening a PR for the phx-remove changes.

[chrismccord]

As a sanity check before I dig into this, can you ensure your id's are prefixed by an alpha character? DOM ids cannot start with numbers, and here they are only numbers. Thanks!

[cthulahoops]

Ah! I've just tried with <div id="id_prefix_<%= item %>"> instead and it's still the same.

[mveytsman]

As a sanity check before I dig into this, can you ensure your id's are prefixed by an alpha character? DOM ids cannot start with numbers, and here they are only numbers. Thanks!

@chrismccord re you digging into this, I'd like to take a crack at fixing this with the group I've been pairing with (@cthulahoops @janiceshiu @7hoenix @ifeLawal). How would you feel about us taking it on if you don't think it's critical and don't mind us taking longer to fix this than you & Jose would :)

Definitely would appreciate a pointer on where to look :)

[josevalim]

Awesome @mveytsman! I would first starting by investigating if this is a client issue or a server issue. If it is a client issue, then you probably have a better idea than me on where to look. If it is on the server, I would be glad to pair and show you around!

[mveytsman]

We think we're well on our way to a fix:

--- a/lib/phoenix_live_view/utils.ex
+++ b/lib/phoenix_live_view/utils.ex
@@ -40,7 +40,11 @@ defmodule Phoenix.LiveView.Utils do
   """
   def clear_changed(%Socket{private: private, assigns: assigns} = socket) do
     temporary = Map.get(private, :temporary_assigns, %{})
-    %Socket{socket | changed: %{}, assigns: Map.merge(assigns, temporary)}
+    # TODO:
+    # Setting changed to temporary sends extra data, instead changed should only be the diff between assigns and temporary
+    # Test this ??
+
+    %Socket{socket | changed: temporary, assigns: Map.merge(assigns, temporary)}
   end

This gives the correct behavior but sends too many updates (see TODO in code). The issue has to do with the client holding state that it shouldn't when temporary assigns are cleared out -- it's similar to the issue I described here.

[josevalim]

@mveytsman the patch above feels incorrect to me as it is forcing it to always re-render the temporary bits. The "instead changed should only be the diff between assigns and temporary" is what we do in the assign/3 function so this is perhaps undoing the work being done there? 🤔

[mveytsman]

@josevalim @cthulahoops and I were just chatting about this!

I think it might be something like (psuedo-code as this isn't the exact signature of assign):

socket = assign(%Socket{socket | changed: %{}}, temporary)

Also, what do you think about splitting out clear_changed and clear_temporary_assigns as they are kind of doing different things?

[josevalim]

The above would also re-render the temporary assigns on every page, which is not what we want, because they will be marked as changed every time. For example, we use temporary asisgns when we need to render complex data and we don't want to keep it around. If we re-render things, then the expensive rendered data will disappear. So think of temporary assigns as a default value... it won't be changed unless you set it to something differently than this default value.

But I see the issue now... red_item is being set to nil but it is never re-rendered as nil! I think the issue here is incorrect usage of temporary assigns. Maybe not really incorrect but a weird interplay between append and temporary assigns - although I am not sure if there is something we can do. :(

[mveytsman]

I just tried this (sorry if the reduce isn't the cleanest way to do it :))

  def clear_changed(%Socket{private: private, assigns: assigns} = socket) do
    temporary = Map.get(private, :temporary_assigns, %{})
    socket = Enum.reduce(temporary, %Socket{socket | changed: %{}}, fn {k, v}, socket ->
      assign(socket, k, v)
    end)

    socket
  end

and it works as expected:

clicking add item

{
  "0": {
    "1": {
      "d": [
        [
          "4",
          "contents: 4",
          "4"
        ]
      ]
    }
  }
}

clicking make red the first time sends an empty list

{
  "0": {
    "0": {
      "0": "4",
      "1": "Red content: 4",
      "s": [
        "\n    <div style=\"background-color: red\" id=\"",
        "\">\n    ",
        "\n    </div>\n    "
      ]
    },
    "1": {
      "d": []
    }
  }
}

clicking it again doesn't send the empty list

{
  "0": {
    "0": {
      "0": "3",
      "1": "Red content: 3"
    }
  }
}

Clicking add item now will send "" for red item as it has to be cleared out

{
  "0": {
    "0": "",
    "1": {
      "d": [
        [
          "5",
          "contents: 5",
          "5"
        ]
      ]
    }
  }
}

Clicking add item again doesn't send an empty red_item

{
  "0": {
    "1": {
      "d": [
        [
          "6",
          "contents: 6",
          "6"
        ]
      ]
    }
  }
}

This follows my expectation of telling the client to clear out temporary assigns only if our last event changed it. Does this seem right to you?

[mveytsman]

re-reading your comment:

So think of temporary assigns as a default value... it won't be changed unless you set it to something differently than this default value.

I think the above code is doing exactly that. Before we were directly putting the default temporary values into assigns and ignoring changed. What we should do is treat it as if we are assigning the default value to them on every event that doesn't touch them.

That said, maybe it's better to do this outside of clear_changed...

[mveytsman]

BTW @josevalim if you're still available to pair, our group's next meeting is next Thursday at 9:30ET. Would you be available to to discuss this synchronously then (or we can find another time 😄 )

[josevalim]

@mveytsman are you sure the code above is not marking the temporary assigns as changed? What does socket.changed after the reduce block looks like? As I said, if they are marked as changed, then it goes directly against the original use case of temporary assigns, which is to render something once, purge the assign from memory, and then never touch it again. :)

[cthulahoops]

We were intending to update socket.changed so that the temporary assign would be propagated to the client. We've been working on the assumption that temporary assigns are always for use with append/prepend. (That's the only documented use?) I'd argue that for your use case the block should be marked as append to make it clear that it shouldn't be replaced empty due to the temporary assign.

That said, I guess the usage you're suggesting is established and we can't break it, so we have to find a different way to solve this.

[josevalim]

Right. Imagine I want to show all comments for a post. But the comments themselves are not interactive. I can mark the comments as temporary assigns so they are rendered once and then never touched again. With your change, in the next cycle all comments would disappear.

However, I am still surprised that the red lines appear duplicated in the front-end. Since they have the same ID, shouldn't the existing ID be changed in place?

[josevalim]

Also, I will be glad to join the meeting, but if the issue is a front-end issue I certainly won't be of much help!

[mveytsman]

With your change, in the next cycle all comments would disappear.

I don't think this is true because the container is marked as append or prepend so the dom would keep the state.

Also, I will be glad to join the meeting, but if the issue is a front-end issue I certainly won't be of much help!

I think it's a complicated interplay of the front-end and the back-end but the solution is a backend change (at least we think so 😄 ). I sent you an invite!

[josevalim]

I don't think this is true because the container is marked as append or prepend so the dom would keep the state.

To be clear, I was talking about a scenario where append/prepend are not being used. They are not a requirement for using temporary assigns (although they were the initial use case). At this point I really don’t think this is a backend issue. :)

[mveytsman]

To be clear, I was talking about a scenario where append/prepend are not being used.

Ah I see -- if that's a usecase I think there's a very strong argument to be made to treat that kind of temporary assigns differently than the kind that are used in append/prepend. I think there's a lot of subtle bugs in how append/prepend works that will go away if we do that. I'll think about the possibility of a client-only fix for this but I think your instinct is correct in that it may be impossible.

[cthulahoops]

However, I am still surprised that the red lines appear duplicated in the front-end. Since they have the same ID, shouldn't the existing ID be changed in place?

The second red line gets added during an update that attempts to add a new item, but there's still a make red which hasn't been reset and gets reapplied.

The document is: id_1 (red), id_1(button).
The update is: id_1 (red), id_2 (button).

Where there's a duplicate id, it's looks like the diff matches the last element with that id, so the operations applied are:

Update: id_1 (button) -> id_1 (red)
Add: id_2 (button)

I think once the page is in an inconsistent state (multiple instances of the same id) the behaviour is undefined anyway, so the goal is to fix this before it gets into this statue.

I think it is possible to fix this client side, but it will be more complex and confusing.

[josevalim]

@cthulahoops oh, the id_1 is duplicated in the same update. That makes sense, thanks for the follow up!

Yeah, I am not sure if we should care about this case. In any case, I have added some docs about this particular pitfall for temporary assigns, so hopefully that helps other avoid using temporary_assigns as some sort of state control.

I think the functionality that you want here would be another kind of assigns, something like reset_assigns, that are reset whenever they change.