diff --git a/assets/js/phoenix_live_view/rendered.js b/assets/js/phoenix_live_view/rendered.js index 863e23efcc..70f60e22ca 100644 --- a/assets/js/phoenix_live_view/rendered.js +++ b/assets/js/phoenix_live_view/rendered.js @@ -287,11 +287,17 @@ export default class Rendered { // keyed comprehensions mergeKeyed(target, source) { - // we need to clone the target since elements can move and otherwise - // it could happen that we modify an element that we'll need to refer to - // later - const clonedTarget = this.clone(target); - Object.entries(source[KEYED]).forEach(([i, entry]) => { + const entries = Object.entries(source[KEYED]); + const hasMoves = entries.some( + ([i, entry]) => + i !== KEYED_COUNT && + (Array.isArray(entry) || typeof entry === "number"), + ); + // Moves read entries from their old positions. Clone before applying any + // entries so an earlier mutation cannot overwrite a position needed later. + // Stable-position updates never read from another position. + const clonedTarget = hasMoves && this.clone(target); + entries.forEach(([i, entry]) => { if (i === KEYED_COUNT) { return; } diff --git a/assets/test/rendered_test.ts b/assets/test/rendered_test.ts index 4e6a5be1d8..c0e73698c7 100644 --- a/assets/test/rendered_test.ts +++ b/assets/test/rendered_test.ts @@ -9,7 +9,7 @@ import { describe("Rendered", () => { describe("mergeDiff", () => { - test("recursively merges two diffs", () => { + test("merges a diff into the rendered tree", () => { const simple = new Rendered("123", simpleDiff1); simple.mergeDiff(simpleDiff2); expect(simple.get()).toEqual({ @@ -17,12 +17,91 @@ describe("Rendered", () => { [COMPONENTS]: {}, newRender: true, }); + }); + test("merges stable-position keyed updates without cloning", () => { const deep = new Rendered("123", deepDiff1); + const clone = jest.spyOn(deep, "clone"); deep.mergeDiff(deepDiff2); + expect(clone).not.toHaveBeenCalled(); expect(deep.get()).toEqual({ ...deepDiffResult, [COMPONENTS]: {} }); }); + test("clones before moving entries so later moves use pre-merge positions", () => { + const rendered = new Rendered("123", { + [KEYED]: { + 0: { 0: "first", 1: "unchanged" }, + 1: { 0: "second", 1: "old" }, + [KEYED_COUNT]: 2, + }, + [STATIC]: ["", "", ""], + }); + const before = (rendered.get() as any)[KEYED]; + const first = before[0]; + const second = before[1]; + const clone = jest.spyOn(rendered, "clone"); + + rendered.mergeDiff({ + [KEYED]: { + 0: [1, { 1: "updated" }], + 1: 0, + [KEYED_COUNT]: 2, + }, + }); + + expect(clone).toHaveBeenCalledTimes(1); + expect((rendered.get() as any)[KEYED]).toEqual({ + 0: { 0: "second", 1: "updated" }, + 1: { 0: "first", 1: "unchanged" }, + [KEYED_COUNT]: 2, + }); + expect(first).toEqual({ 0: "first", 1: "unchanged" }); + expect(second).toEqual({ 0: "second", 1: "old" }); + }); + + test("clones only a moved nested keyed subtree", () => { + const rendered = new Rendered("123", { + [KEYED]: { + 0: { + 0: "outer", + 1: { + [KEYED]: { + 0: { 0: "first" }, + 1: { 0: "second" }, + [KEYED_COUNT]: 2, + }, + [STATIC]: ["", ""], + }, + }, + [KEYED_COUNT]: 1, + }, + [STATIC]: ["", "", ""], + }); + const outer = (rendered.get() as any)[KEYED][0]; + const nested = outer[1]; + const clone = jest.spyOn(rendered, "clone"); + + rendered.mergeDiff({ + [KEYED]: { + 0: { + 1: { + [KEYED]: { 0: 1, 1: 0, [KEYED_COUNT]: 2 }, + }, + }, + [KEYED_COUNT]: 1, + }, + }); + + expect(clone).toHaveBeenCalledTimes(1); + expect(clone.mock.calls[0][0]).toBe(nested); + expect((rendered.get() as any)[KEYED][0]).toBe(outer); + expect(nested[KEYED]).toEqual({ + 0: { 0: "second" }, + 1: { 0: "first" }, + [KEYED_COUNT]: 2, + }); + }); + test("merges the latter diff if it contains a `static` key", () => { const diff1 = { 0: ["a"], 1: ["b"] }; const diff2 = { 0: ["c"], [STATIC]: ["c"] };