diff --git a/CHANGELOG.md b/CHANGELOG.md index ac850a902..5a307aea9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * Send `{:shutdown, :cancel}` to `handle_async/3` on `cancel_async` * Prevent events from child LiveViews from bubbling up to root LiveView when child is not mounted yet * Fix `phx-mounted` being called twice for stream items - * Do not move existing stream items when prepending + * Never move existing stream items if they already exist (use stream_delete and then stream_insert instead) * Fix live component rendering breaking when the server adds a component back that was pruned by the client (#3026) * Allow redirect from upload progress callback diff --git a/assets/js/phoenix_live_view/dom_patch.js b/assets/js/phoenix_live_view/dom_patch.js index 21656b7ad..d6110ba6b 100644 --- a/assets/js/phoenix_live_view/dom_patch.js +++ b/assets/js/phoenix_live_view/dom_patch.js @@ -149,7 +149,7 @@ export default class DOMPatch { let {ref, streamAt} = this.getStreamInsert(child) if(ref === undefined){ return parent.appendChild(child) } - DOM.putSticky(child, PHX_STREAM_REF, el => el.setAttribute(PHX_STREAM_REF, ref)) + this.setStreamRef(child, ref) // we may need to restore skipped components, see removeStreamChildElement child.querySelectorAll(`[${PHX_MAGIC_ID}][${PHX_SKIP}]`).forEach(el => { @@ -344,12 +344,21 @@ export default class DOMPatch { return insert || {} } - maybeReOrderStream(el, isNew){ - let {ref, streamAt} = this.getStreamInsert(el) - if(streamAt === undefined || (streamAt === 0 && !isNew)){ return } - - // we need to the PHX_STREAM_REF here as well as addChild is invoked only for parents + setStreamRef(el, ref){ DOM.putSticky(el, PHX_STREAM_REF, el => el.setAttribute(PHX_STREAM_REF, ref)) + } + + maybeReOrderStream(el, isNew){ + let {ref, streamAt, reset} = this.getStreamInsert(el) + if(streamAt === undefined){ return } + + // we need to set the PHX_STREAM_REF here as well as addChild is invoked only for parents + this.setStreamRef(el, ref) + + if(!reset && !isNew){ + // we only reorder if the element is new or it's a stream reset + return + } if(streamAt === 0){ el.parentElement.insertBefore(el, el.parentElement.firstElementChild) diff --git a/lib/phoenix_live_view.ex b/lib/phoenix_live_view.ex index 0a052b90d..15fc59924 100644 --- a/lib/phoenix_live_view.ex +++ b/lib/phoenix_live_view.ex @@ -1728,12 +1728,11 @@ defmodule Phoenix.LiveView do ## Updating Items - As shown, an existing item on the client can be updated by issuing a `stream_insert` for - the existing item. When the client updates an existing item with an "append" operation - (passing the `at: -1` option), the item will remain in the same location as it was - previously, and will not be moved to the end of the parent children. To both update an - existing item and move it to the end of a collection, issue a `stream_delete`, followed - by a `stream_insert`. For example: + As shown, an existing item on the client can be updated by issuing a `stream_insert` + for the existing item. When the client updates an existing item, the item will remain + in the same location as it was previously, and will not be moved to the end of the + parent children. To both update an existing item and move it to another position, + issue a `stream_delete`, followed by a `stream_insert`. For example: song = get_song!(id) diff --git a/lib/phoenix_live_view/test/dom.ex b/lib/phoenix_live_view/test/dom.ex index 89eeebc1a..beea41b95 100644 --- a/lib/phoenix_live_view/test/dom.ex +++ b/lib/phoenix_live_view/test/dom.ex @@ -474,7 +474,7 @@ defmodule Phoenix.LiveViewTest.DOM do acc # do not append existing child if already present, only update in place - current_index && insert_at == -1 && (existing? or appended?) -> + current_index && insert_at && existing? -> if deleted? do acc |> List.delete_at(current_index) |> List.insert_at(insert_at, child) else diff --git a/test/e2e/tests/streams.spec.js b/test/e2e/tests/streams.spec.js index 900d6799a..a3d9635d4 100644 --- a/test/e2e/tests/streams.spec.js +++ b/test/e2e/tests/streams.spec.js @@ -191,9 +191,10 @@ test.describe("Issue #2656", () => { }); }); -test.describe("Issue #2994", () => { - const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id)); +// helper function used below +const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id)); +test.describe("Issue #2994", () => { test("can filter and reset a stream", async ({ page }) => { await page.goto("/stream/reset"); await syncLV(page); @@ -234,7 +235,7 @@ test.describe("Issue #2994", () => { await expect(await listItems(page)).toEqual(["items-b", "items-c", "items-d"]); - await page.getByRole("button", { name: "Prepend" }).click(); + await page.getByRole("button", { name: "Prepend", exact: true }).click(); await syncLV(page); await expect(await listItems(page)).toEqual([ @@ -249,7 +250,7 @@ test.describe("Issue #2994", () => { await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); - await page.getByRole("button", { name: "Append" }).click(); + await page.getByRole("button", { name: "Append", exact: true }).click(); await syncLV(page); await expect(await listItems(page)).toEqual([ @@ -263,8 +264,6 @@ test.describe("Issue #2994", () => { }); test.describe("Issue #2982", () => { - const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id)); - test("can reorder a stream with LiveComponents as direct stream children", async ({ page }) => { await page.goto("/stream/reset-lc"); await syncLV(page); @@ -279,8 +278,6 @@ test.describe("Issue #2982", () => { }); test.describe("Issue #3023", () => { - const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id)); - test("can bulk insert items at a specific index", async ({ page }) => { await page.goto("/stream/reset"); await syncLV(page); @@ -533,3 +530,40 @@ test.describe("stream limit - issue #2686", () => { ]); }); }); + +test("any stream insert for elements already in the DOM does not reorder", async ({ page }) => { + await page.goto("/stream/reset"); + await syncLV(page); + + await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); + + await page.getByRole("button", { name: "Prepend C" }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); + + await page.getByRole("button", { name: "Append C" }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); + + await page.getByRole("button", { name: "Insert C at 1" }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); + + await page.getByRole("button", { name: "Insert at 1", exact: true }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual([ + "items-a", + expect.stringMatching(/items-a-.*/), + "items-b", + "items-c", + "items-d" + ]); + + await page.getByRole("button", { name: "Reset" }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]); + + await page.getByRole("button", { name: "Delete C and insert at 1" }).click(); + await syncLV(page); + await expect(await listItems(page)).toEqual(["items-a", "items-c", "items-b", "items-d"]); +}); diff --git a/test/phoenix_live_view/integrations/stream_test.exs b/test/phoenix_live_view/integrations/stream_test.exs index 909bc229a..5e78b8e3b 100644 --- a/test/phoenix_live_view/integrations/stream_test.exs +++ b/test/phoenix_live_view/integrations/stream_test.exs @@ -210,13 +210,13 @@ defmodule Phoenix.LiveView.StreamTest do html = assert lv |> element("button", "Filter") |> render_click() assert ids_in_ul_list(html) == ["items-b", "items-c", "items-d"] - html = assert lv |> element("button", "Prepend") |> render_click() + html = assert lv |> element(~s(button[phx-click="prepend"]), "Prepend") |> render_click() assert [<<"items-a-", _::binary>>, "items-b", "items-c", "items-d"] = ids_in_ul_list(html) html = assert lv |> element("button", "Reset") |> render_click() assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"] - html = assert lv |> element("button", "Append") |> render_click() + html = assert lv |> element(~s(button[phx-click="append"]), "Append") |> render_click() assert ["items-a", "items-b", "items-c", "items-d", <<"items-a-", _::binary>>] = ids_in_ul_list(html) @@ -328,6 +328,30 @@ defmodule Phoenix.LiveView.StreamTest do assert ids_in_ul_list(html) == ["items-a", "items-e", "items-f", "items-g", "items-b", "items-c", "items-d"] end + test "any stream insert for elements already in the DOM does not reorder", %{conn: conn} do + {:ok, lv, html} = live(conn, "/stream/reset") + + assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"] + + html = assert lv |> element("button", "Prepend C") |> render_click() + assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"] + + html = assert lv |> element("button", "Append C") |> render_click() + assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"] + + html = assert lv |> element("button", "Insert C at 1") |> render_click() + assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"] + + html = assert lv |> element("button", "Insert at 1") |> render_click() + assert ["items-a", _, "items-b", "items-c", "items-d"] = ids_in_ul_list(html) + + html = assert lv |> element("button", "Reset") |> render_click() + assert ["items-a", "items-b", "items-c", "items-d"] = ids_in_ul_list(html) + + html = assert lv |> element("button", "Delete C and insert at 1") |> render_click() + assert ["items-a", "items-c", "items-b", "items-d"] = ids_in_ul_list(html) + end + test "stream raises when attempting to consume ahead of for", %{conn: conn} do {:ok, lv, _html} = live(conn, "/stream") diff --git a/test/support/live_views/streams.ex b/test/support/live_views/streams.ex index 967c159cb..f4a41c09f 100644 --- a/test/support/live_views/streams.ex +++ b/test/support/live_views/streams.ex @@ -117,7 +117,10 @@ defmodule Phoenix.LiveViewTest.StreamLive do end def handle_event("admin-move-to-first", %{"id" => "admins-" <> id}, socket) do - {:noreply, stream_insert(socket, :admins, user(id, "updated"), at: 0)} + {:noreply, + socket + |> stream_delete_by_dom_id(:admins, "admins-" <> id) + |> stream_insert(:admins, user(id, "updated"), at: 0)} end def handle_event("admin-move-to-last", %{"id" => "admins-" <> id = dom_id}, socket) do @@ -294,6 +297,11 @@ defmodule Phoenix.LiveViewTest.StreamResetLive do + + + + + """ end @@ -374,6 +382,57 @@ defmodule Phoenix.LiveViewTest.StreamResetLive do at: 1 )} end + + def handle_event("insert-at-one", _, socket) do + {:noreply, + stream_insert( + socket, + :items, + %{id: "a" <> "#{System.unique_integer()}", name: "#{System.unique_integer()}"}, + at: 1 + )} + end + + def handle_event("insert-existing-at-one", _, socket) do + {:noreply, + stream_insert( + socket, + :items, + %{id: "c", name: "C"}, + at: 1 + )} + end + + def handle_event("delete-insert-existing-at-one", _, socket) do + {:noreply, + socket + |> stream_delete_by_dom_id(:items, "items-c") + |> stream_insert( + :items, + %{id: "c", name: "C"}, + at: 1 + )} + end + + def handle_event("prepend-existing", _, socket) do + {:noreply, + stream_insert( + socket, + :items, + %{id: "c", name: "C"}, + at: 0 + )} + end + + def handle_event("append-existing", _, socket) do + {:noreply, + stream_insert( + socket, + :items, + %{id: "c", name: "C"}, + at: -1 + )} + end end defmodule Phoenix.LiveViewTest.StreamResetLCLive do