skip reordering elements with no parent, fix phx-remove on re-added s…

…tream items (#3121)

* skip reordering elements with no parent

* fix phx-remove on re-added stream elements

* Update assets/js/phoenix_live_view/dom_patch.js

---------

Co-authored-by: Chris McCord <chris@chrismccord.com>

assets/js/phoenix_live_view/dom_patch.js

@@ -331,14 +331,15 @@ export default class DOMPatch {
   }
 
   removeStreamChildElement(child){
-    if(!this.maybePendingRemove(child)){
-      if(this.streamInserts[child.id]){
-        // we need to store children so we can morph them later
-        this.streamComponentRestore[child.id] = child
-        child.remove()
-      } else {
+    // we need to store the node if it is actually re-added in the same patch
+    // we do NOT want to execute phx-remove, we do NOT want to call onNodeDiscarded
+    if(this.streamInserts[child.id]){
+      this.streamComponentRestore[child.id] = child
+      child.remove()
+    } else {
+      // only remove the element now if it has no phx-remove binding
+      if(!this.maybePendingRemove(child)){
         child.remove()
-        // TODO: check if we really don't want to call discarded for re-added stream items
         this.onNodeDiscarded(child)
       }
     }
@@ -365,6 +366,12 @@ export default class DOMPatch {
       return
     }
 
+    // check if the element has a parent element;
+    // it doesn't if we are currently recursively morphing (restoring a saved stream child)
+    // because the element is not yet added to the real dom;
+    // reordering does not make sense in that case anyway
+    if(!el.parentElement){ return }
+
     if(streamAt === 0){
       el.parentElement.insertBefore(el, el.parentElement.firstElementChild)
     } else if(streamAt > 0){

test/e2e/tests/streams.spec.js

@@ -726,3 +726,34 @@ test("stream nested in a LiveComponent is properly restored on reset", async ({
     { id: "nested-items-a-g", text: "N-G" },
   ]);
 });
+
+test("phx-remove is handled correctly when restoring nodes", async ({ page }) => {
+  await page.goto("/stream/reset?phx-remove");
+  await syncLV(page);
+
+  await expect(await listItems(page)).toEqual([
+    { id: "items-a", text: "A" },
+    { id: "items-b", text: "B" },
+    { id: "items-c", text: "C" },
+    { id: "items-d", text: "D" }
+  ]);
+
+  await page.getByRole("button", { name: "Filter" }).click();
+  await syncLV(page);
+
+  await expect(await listItems(page)).toEqual([
+    { id: "items-b", text: "B" },
+    { id: "items-c", text: "C" },
+    { id: "items-d", text: "D" }
+  ]);
+
+  await page.getByRole("button", { name: "Reset" }).click();
+  await syncLV(page);
+
+  await expect(await listItems(page)).toEqual([
+    { id: "items-a", text: "A" },
+    { id: "items-b", text: "B" },
+    { id: "items-c", text: "C" },
+    { id: "items-d", text: "D" }
+  ]);
+});

test/support/live_views/streams.ex

@@ -301,21 +301,26 @@ defmodule Phoenix.LiveViewTest.StreamResetLive do
 
   # see https://github.com/phoenixframework/phoenix_live_view/issues/2994
 
-  def mount(_params, _session, socket) do
+  def mount(params, _session, socket) do
     socket
     |> stream(:items, [
       %{id: "a", name: "A"},
       %{id: "b", name: "B"},
       %{id: "c", name: "C"},
       %{id: "d", name: "D"}
     ])
+    |> assign(:use_phx_remove, is_map(params) && params["phx-remove"])
     |> then(&{:ok, &1})
   end
 
   def render(assigns) do
     ~H"""
     <ul phx-update="stream" id="thelist">
-      <li :for={{id, item} <- @streams.items} id={id}>
+      <li
+        :for={{id, item} <- @streams.items}
+        id={id}
+        phx-remove={if @use_phx_remove, do: Phoenix.LiveView.JS.hide()}
+      >
         <%= item.name %>
       </li>
     </ul>