From 603d65380010035ee705ff9c625ac226107a9c81 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Thu, 29 Feb 2024 20:22:04 -0500 Subject: [PATCH] [Live] Improving child render handling: avoid removing element from DOM when possible --- .../assets/dist/live_controller.js | 46 +++++++-- src/LiveComponent/assets/src/morphdom.ts | 97 +++++++++++++++---- .../assets/test/controller/child.test.ts | 74 +++++++++++++- src/LiveComponent/assets/test/tools.ts | 6 +- 4 files changed, 193 insertions(+), 30 deletions(-) diff --git a/src/LiveComponent/assets/dist/live_controller.js b/src/LiveComponent/assets/dist/live_controller.js index 5fc487f450f..f4e0f1754ae 100644 --- a/src/LiveComponent/assets/dist/live_controller.js +++ b/src/LiveComponent/assets/dist/live_controller.js @@ -1332,7 +1332,21 @@ const syncAttributes = function (fromEl, toEl) { } }; function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, getElementValue, externalMutationTracker) { - const preservedOriginalElements = []; + const originalElementIdsToSwapAfter = []; + const originalElementsToPreserve = new Map(); + const markElementAsNeedingPostMorphSwap = (id, replaceWithClone) => { + const oldElement = originalElementsToPreserve.get(id); + if (!(oldElement instanceof HTMLElement)) { + throw new Error(`Original element with id ${id} not found`); + } + originalElementIdsToSwapAfter.push(id); + if (!replaceWithClone) { + return null; + } + const clonedOldElement = cloneHTMLElement(oldElement); + oldElement.replaceWith(clonedOldElement); + return clonedOldElement; + }; rootToElement.querySelectorAll('[data-live-preserve]').forEach((newElement) => { const id = newElement.id; if (!id) { @@ -1342,10 +1356,8 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, if (!(oldElement instanceof HTMLElement)) { throw new Error(`The element with id "${id}" was not found in the original HTML`); } - const clonedOldElement = cloneHTMLElement(oldElement); - preservedOriginalElements.push(oldElement); - oldElement.replaceWith(clonedOldElement); newElement.removeAttribute('data-live-preserve'); + originalElementsToPreserve.set(id, oldElement); syncAttributes(newElement, oldElement); }); Idiomorph.morph(rootFromElement, rootToElement, { @@ -1357,6 +1369,17 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, if (fromEl === rootFromElement) { return true; } + if (fromEl.id && originalElementsToPreserve.has(fromEl.id)) { + if (fromEl.id === toEl.id) { + return false; + } + const clonedFromEl = markElementAsNeedingPostMorphSwap(fromEl.id, true); + if (!clonedFromEl) { + throw new Error('missing clone'); + } + Idiomorph.morph(clonedFromEl, toEl); + return false; + } if (fromEl instanceof HTMLElement && toEl instanceof HTMLElement) { if (typeof fromEl.__x !== 'undefined') { if (!window.Alpine) { @@ -1406,6 +1429,10 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, if (!(node instanceof HTMLElement)) { return true; } + if (node.id && originalElementsToPreserve.has(node.id)) { + markElementAsNeedingPostMorphSwap(node.id, false); + return true; + } if (externalMutationTracker.wasElementAdded(node)) { return false; } @@ -1413,12 +1440,13 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, }, }, }); - preservedOriginalElements.forEach((oldElement) => { - const newElement = rootFromElement.querySelector(`#${oldElement.id}`); - if (!(newElement instanceof HTMLElement)) { - throw new Error('Missing preserved element'); + originalElementIdsToSwapAfter.forEach((id) => { + const newElement = rootFromElement.querySelector(`#${id}`); + const originalElement = originalElementsToPreserve.get(id); + if (!(newElement instanceof HTMLElement) || !(originalElement instanceof HTMLElement)) { + throw new Error('Missing elements.'); } - newElement.replaceWith(oldElement); + newElement.replaceWith(originalElement); }); } diff --git a/src/LiveComponent/assets/src/morphdom.ts b/src/LiveComponent/assets/src/morphdom.ts index d56f0f45f36..12fab827632 100644 --- a/src/LiveComponent/assets/src/morphdom.ts +++ b/src/LiveComponent/assets/src/morphdom.ts @@ -27,14 +27,48 @@ export function executeMorphdom( * To handle them, we: * 1) Create an array of the "current" HTMLElements that match each * "data-live-preserve" element. - * 2) Replace the "current" elements with clones so that the originals - * aren't modified during the morphing process. - * 3) After the morphing is complete, we find the preserved elements and - * replace them with the originals. + * 2) If we detect that a "current" preserved element is about to be morphed + * (updated or removed), replace the "current" element with a clone so + * the original isn't modified during the morphing process. Mark that + * this element needs to be replaced after the morphing is complete. + * 3) After the morphing is complete, loop over the elements that were + * replaced and swap the original element back into the new position. + * + * This allows Idiomorph to potentially morph the position of the preserved + * elements... but still allowing us to make sure that the element in the + * new position is exactly the original HTMLElement. + */ + const originalElementIdsToSwapAfter: Array = []; + const originalElementsToPreserve = new Map(); + + /** + * Called when a preserved element is about to be morphed. + * + * Instead of allowing the original to be morphed, a fake clone + * is created and morphed instead. The original is then marked + * to be replaced after the morph with wherever the final + * matching id element ends up. */ - const preservedOriginalElements: HTMLElement[] = []; + const markElementAsNeedingPostMorphSwap = (id: string, replaceWithClone: boolean): HTMLElement | null => { + const oldElement = originalElementsToPreserve.get(id); + if (!(oldElement instanceof HTMLElement)) { + throw new Error(`Original element with id ${id} not found`); + } + + originalElementIdsToSwapAfter.push(id); + if (!replaceWithClone) { + return null; + } + + const clonedOldElement = cloneHTMLElement(oldElement); + oldElement.replaceWith(clonedOldElement); + + return clonedOldElement; + }; + rootToElement.querySelectorAll('[data-live-preserve]').forEach((newElement) => { const id = newElement.id; + if (!id) { throw new Error('The data-live-preserve attribute requires an id attribute to be set on the element'); } @@ -44,11 +78,8 @@ export function executeMorphdom( throw new Error(`The element with id "${id}" was not found in the original HTML`); } - const clonedOldElement = cloneHTMLElement(oldElement); - preservedOriginalElements.push(oldElement); - oldElement.replaceWith(clonedOldElement); - newElement.removeAttribute('data-live-preserve'); + originalElementsToPreserve.set(id, oldElement); syncAttributes(newElement, oldElement); }); @@ -64,6 +95,27 @@ export function executeMorphdom( return true; } + if (fromEl.id && originalElementsToPreserve.has(fromEl.id)) { + if (fromEl.id === toEl.id) { + // the preserved elements match, prevent morph and + // keep the original element + return false; + } + + // a preserved element is being morphed into something else + // this means that preserved element is being moved + // to avoid the original element being morphed, we swap + // it for a clone, manually morph the clone, and then + // skip trying to morph the original element (we want it untouched) + const clonedFromEl = markElementAsNeedingPostMorphSwap(fromEl.id, true); + if (!clonedFromEl) { + throw new Error('missing clone'); + } + Idiomorph.morph(clonedFromEl, toEl); + + return false; + } + // skip special checking if this is, for example, an SVG if (fromEl instanceof HTMLElement && toEl instanceof HTMLElement) { // We assume fromEl is an Alpine component if it has `__x` property. @@ -168,6 +220,18 @@ export function executeMorphdom( return true; } + if (node.id && originalElementsToPreserve.has(node.id)) { + // a preserved element is being removed + // to avoid the original element being destroyed (but still + // allowing this spot on the dom to be removed), + // clone the original element and place it into the + // new position after morphing + markElementAsNeedingPostMorphSwap(node.id, false); + + // allow this to be morphed to the new element + return true; + } + if (externalMutationTracker.wasElementAdded(node)) { // this element was added by an external mutation, so we don't want to discard it return false; @@ -178,14 +242,13 @@ export function executeMorphdom( }, }); - preservedOriginalElements.forEach((oldElement) => { - const newElement = rootFromElement.querySelector(`#${oldElement.id}`); - if (!(newElement instanceof HTMLElement)) { - // should not happen, as preservedOriginalElements is built from - // the new HTML - throw new Error('Missing preserved element'); + originalElementIdsToSwapAfter.forEach((id: string) => { + const newElement = rootFromElement.querySelector(`#${id}`); + const originalElement = originalElementsToPreserve.get(id); + if (!(newElement instanceof HTMLElement) || !(originalElement instanceof HTMLElement)) { + // should not happen + throw new Error('Missing elements.'); } - - newElement.replaceWith(oldElement); + newElement.replaceWith(originalElement); }); } diff --git a/src/LiveComponent/assets/test/controller/child.test.ts b/src/LiveComponent/assets/test/controller/child.test.ts index aa9888a9da1..7f7f3cf63b2 100644 --- a/src/LiveComponent/assets/test/controller/child.test.ts +++ b/src/LiveComponent/assets/test/controller/child.test.ts @@ -16,10 +16,12 @@ import { shutdownTests, getComponent, dataToJsonAttribute, + getStimulusApplication } from '../tools'; import { getByTestId, waitFor } from '@testing-library/dom'; import userEvent from '@testing-library/user-event'; import { findChildren } from '../../src/ComponentRegistry'; +import { Controller } from '@hotwired/stimulus'; describe('Component parent -> child initialization and rendering tests', () => { afterEach(() => { @@ -128,6 +130,74 @@ describe('Component parent -> child initialization and rendering tests', () => { expect(test.element).not.toContainHTML('data-live-preserve'); }); + it('data-live-preserve child in same location is not removed/re-added to the DOM', async () => { + const originalChild = ` +
+
+ Original Child Component +
+ `; + const updatedChild = ` +
+ `; + + const test = await createTest({useOriginalChild: true}, (data: any) => ` +
+ ${data.useOriginalChild ? originalChild : updatedChild} +
+ `); + + getStimulusApplication().register('track-connect', class extends Controller { + disconnect() { + this.element.setAttribute('disconnected', ''); + } + }); + + test.expectsAjaxCall() + .serverWillChangeProps((data: any) => { + data.useOriginalChild = false; + }); + + await test.component.render(); + // sanity check that the child is there + expect(test.element).toHaveTextContent('Original Child Component'); + // check that the element was never disconnected/removed from the DOM + expect(test.element).not.toContainHTML('disconnected'); + }); + + it('data-live-preserve element moved correctly when position changes and old element morphed into different element', async () => { + const originalChild = ` +
+
+ Original Child Component +
+ `; + const updatedChild = ` +
+ `; + + // when morphing original -> updated, the outer div (which was the child) + // will be morphed into a normal div + const test = await createTest({useOriginalChild: true}, (data: any) => ` +
+ ${data.useOriginalChild ? originalChild : ''} + ${data.useOriginalChild ? '' : `
${updatedChild}
`} +
+ `) + + test.expectsAjaxCall() + .serverWillChangeProps((data: any) => { + data.useOriginalChild = false; + }); + + const childElement = getByTestId(test.element, 'child-component'); + await test.component.render(); + // sanity check that the child is there + expect(test.element).toHaveTextContent('Original Child Component'); + expect(test.element).toContainHTML('class="wrapper"'); + expect(childElement.parentElement).toHaveClass('wrapper'); + }); + it('existing child component gets props & triggers re-render', async () => { const childTemplate = (data: any) => `
child initialization and rendering tests', () => { .willReturn(childTemplate); // trigger the parent render, which will trigger the children to re-render - test.component.render(); + await test.component.render(); - // wait for parent Ajax call to finish - await waitFor(() => expect(test.element).not.toHaveAttribute('busy')); // wait for child to start and stop loading await waitFor(() => expect(getByTestId(test.element, 'child-component-1')).not.toHaveAttribute('busy')); diff --git a/src/LiveComponent/assets/test/tools.ts b/src/LiveComponent/assets/test/tools.ts index 4c70596d646..e0e1735160e 100644 --- a/src/LiveComponent/assets/test/tools.ts +++ b/src/LiveComponent/assets/test/tools.ts @@ -394,10 +394,14 @@ export async function startStimulus(element: string|HTMLElement) { return { controller, - element: controllerElement + element: controllerElement, } } +export const getStimulusApplication = (): Application => { + return application; +} + const getControllerElement = (container: HTMLElement): HTMLElement => { if (container.dataset.controller === 'live') { return container;