[LiveComponent][Autocomplete] Adding support for libraries to play nicely together

weaverryan · weaverryan · commit c8f51be1f55a · 2022-08-10T16:35:28.000-04:00
This includes several important points:

A) In autocomplete, data-live-ignore was added so that, when a "live component" re-renders,
the element is not affected.

B) For morphdom, the node key was changed from "id" to "data-live-id". This was due to
a very complex "bad situation" that arised when autocomplete+live components. TomSelect
finds the label for the element and give it an id attribute, which helps with the
aria- attributes. However, this caused, on re-render, morphdom to think the "new label"
did not match the old label. This small difference caused the elements in the parent
div to be re-added one-by-one. Most importantly, a very innocent fromEl.appendChild(selectEl)
is called deep in morphdom. In reality, selectEl already exists in fromEl... and the purpose
of this call is actually just to help order things in the correct way. However, appendChild()
caused the select element to momentarily be removed from the DOM then re-added. Stimulus
noticed this and disconnected and then reconnected the autocomplete controller, causing
the element to re-initialize (this, at the very least, would cause the element to lose focus).

Anyways, this whole bad situation (which, is a bit poorly handled by morphdom), was caused
by morphdom being confused by the "id" attribute on the label, which is present in the DOM
but not in the re-rendered HTML. To avoid side effects like this, the nodeKey was changed
to data-live-id.

C) In live components, we also fixed a bug where if an element had data-live-ignore, it could
be, incorrectly, removed on re-render if not present.
diff --git a/src/Autocomplete/assets/dist/controller.js b/src/Autocomplete/assets/dist/controller.js
@@ -28,10 +28,16 @@ class default_1 extends Controller {
         super(...arguments);
         _instances.add(this);
     }
-    connect() {
-        if (this.tomSelect) {
-            return;
+    initialize() {
+        this.element.setAttribute('data-live-ignore', '');
+        if (this.element.id) {
+            const label = document.querySelector(`label[for="${this.element.id}"]`);
+            if (label) {
+                label.setAttribute('data-live-ignore', '');
+            }
         }
+    }
+    connect() {
         if (this.urlValue) {
             this.tomSelect = __classPrivateFieldGet(this, _instances, "m", _createAutocompleteWithRemoteData).call(this, this.urlValue);
             return;
@@ -43,6 +49,7 @@ class default_1 extends Controller {
         this.tomSelect = __classPrivateFieldGet(this, _instances, "m", _createAutocomplete).call(this);
     }
     disconnect() {
+        this.tomSelect.revertSettings.innerHTML = this.element.innerHTML;
         this.tomSelect.destroy();
     }
     get selectElement() {
@@ -80,6 +87,10 @@ _instances = new WeakSet(), _getCommonConfig = function _getCommonConfig() {
         onItemAdd: () => {
             this.tomSelect.setTextboxValue('');
         },
+        onInitialize: function () {
+            const tomSelect = this;
+            tomSelect.wrapper.setAttribute('data-live-ignore', '');
+        },
         closeAfterSelect: true,
     };
     if (!this.selectElement && !this.urlValue) {
diff --git a/src/Autocomplete/assets/src/controller.ts b/src/Autocomplete/assets/src/controller.ts
@@ -18,12 +18,17 @@ export default class extends Controller {
     readonly tomSelectOptionsValue: object;
     tomSelect: TomSelect;
 
-    connect() {
-        // this avoids initializing the same field twice (TomSelect shows an error otherwise)
-        if (this.tomSelect) {
-            return;
+    initialize() {
+        this.element.setAttribute('data-live-ignore', '');
+        if (this.element.id) {
+            const label = document.querySelector(`label[for="${this.element.id}"]`);
+            if (label) {
+                label.setAttribute('data-live-ignore', '');
+            }
         }
+    }
 
+    connect() {
         if (this.urlValue) {
             this.tomSelect = this.#createAutocompleteWithRemoteData(this.urlValue);
 
@@ -40,9 +45,9 @@ export default class extends Controller {
     }
 
     disconnect() {
+        // make sure it will "revert" to the latest innerHTML
+        this.tomSelect.revertSettings.innerHTML = this.element.innerHTML;
         this.tomSelect.destroy();
-        // Fixes https://github.com/symfony/ux/issues/407
-        this.tomSelect = undefined;
     }
 
     #getCommonConfig(): Partial<TomSettings> {
@@ -73,6 +78,10 @@ export default class extends Controller {
             onItemAdd: () => {
                 this.tomSelect.setTextboxValue('');
             },
+            onInitialize: function() {
+                const tomSelect = this as any;
+                tomSelect.wrapper.setAttribute('data-live-ignore', '');
+            },
             closeAfterSelect: true,
         };
 
@@ -124,7 +133,7 @@ export default class extends Controller {
             },
             // VERY IMPORTANT: use 'function (query, callback) { ... }' instead of the
             // '(query, callback) => { ... }' syntax because, otherwise,
-            // the 'this.XXX' calls inside of this method fail
+            // the 'this.XXX' calls inside this method fail
             load: function (query: string, callback: (results?: any) => void) {
                 const url = this.getUrl(query);
                 fetch(url)
diff --git a/src/Autocomplete/assets/test/controller.test.ts b/src/Autocomplete/assets/test/controller.test.ts
@@ -10,7 +10,7 @@
 'use strict';
 
 import { Application, Controller } from '@hotwired/stimulus';
-import { getByTestId, waitFor, getAllByLabelText } from '@testing-library/dom';
+import { getByTestId, waitFor } from '@testing-library/dom';
 import { clearDOM, mountDOM } from '@symfony/stimulus-testing';
 import AutocompleteController from '../src/controller';
 import fetchMock from 'fetch-mock-jest';
@@ -137,4 +137,32 @@ describe('AutocompleteController', () => {
             expect(container.querySelectorAll('.option[data-selectable]')).toHaveLength(2);
         });
     });
+
+    it('adds live-component support', async () => {
+        const container = mountDOM(`
+            <div>
+                <label for="the-select" data-testid="main-element-label">Select something</label>
+                <select
+                    id="the-select"
+                    data-testid="main-element"
+                    data-controller="check autocomplete"
+                ></select>
+            </div>
+        `);
+
+        application = startStimulus();
+
+        await waitFor(() => {
+            expect(getByTestId(container, 'main-element')).toHaveClass('connected');
+        });
+
+        expect(getByTestId(container, 'main-element')).toHaveAttribute('data-live-ignore');
+        expect(getByTestId(container, 'main-element-label')).toHaveAttribute('data-live-ignore');
+        const tsDropdown = container.querySelector('.ts-wrapper');
+
+        await waitFor(() => {
+            expect(tsDropdown).not.toBeNull();
+        });
+        expect(tsDropdown).toHaveAttribute('data-live-ignore');
+    });
 });
diff --git a/src/LiveComponent/CHANGELOG.md b/src/LiveComponent/CHANGELOG.md
@@ -1,5 +1,12 @@
 # CHANGELOG
 
+## 2.4.0
+
+-   [BC BREAK] Previously, the `id` attribute was used with `morphdom` as the
+    "node id" when updating the DOM after a render. This has changed to
+    `data-live-id`. This is useful when maintaining the correct order of a list
+    of elements.
+
 ## 2.3.0
 
 -   [BC BREAK] The `data-action="live#update"` attribute must now be
diff --git a/src/LiveComponent/assets/dist/live_controller.js b/src/LiveComponent/assets/dist/live_controller.js
@@ -1084,8 +1084,8 @@ function getModelDirectiveFromInput(element, throwOnMissing = true) {
     }
     if (element.getAttribute('name')) {
         const formElement = element.closest('form');
-        if (formElement && formElement.dataset.model) {
-            const directives = parseDirectives(formElement.dataset.model);
+        if (formElement && ('model' in formElement.dataset)) {
+            const directives = parseDirectives(formElement.dataset.model || '*');
             const directive = directives[0];
             if (directive.args.length > 0 || directive.named.length > 0) {
                 throw new Error(`The data-model="${formElement.dataset.model}" format is invalid: it does not support passing arguments to the model.`);
@@ -1222,9 +1222,7 @@ class default_1 extends Controller {
         if (!(this.element instanceof HTMLElement)) {
             throw new Error('Invalid Element Type');
         }
-        if (this.element.dataset.poll !== undefined) {
-            this._initiatePolling(this.element.dataset.poll);
-        }
+        this._initiatePolling();
         window.addEventListener('beforeunload', this.markAsWindowUnloaded);
         this._startAttributesMutationObserver();
         this.element.addEventListener('live:update-model', this.handleUpdateModelEvent);
@@ -1234,9 +1232,7 @@ class default_1 extends Controller {
         this._dispatchEvent('live:connect', { controller: this });
     }
     disconnect() {
-        this.pollingIntervals.forEach((interval) => {
-            clearInterval(interval);
-        });
+        this._stopAllPolling();
         window.removeEventListener('beforeunload', this.markAsWindowUnloaded);
         this.element.removeEventListener('live:update-model', this.handleUpdateModelEvent);
         this.element.removeEventListener('input', this.handleInputEvent);
@@ -1598,6 +1594,12 @@ class default_1 extends Controller {
     _executeMorphdom(newHtml, modifiedElements) {
         const newElement = htmlToElement(newHtml);
         morphdom(this.element, newElement, {
+            getNodeKey: (node) => {
+                if (!(node instanceof HTMLElement)) {
+                    return;
+                }
+                return node.dataset.liveId;
+            },
             onBeforeElUpdated: (fromEl, toEl) => {
                 if (!(fromEl instanceof HTMLElement) || !(toEl instanceof HTMLElement)) {
                     return false;
@@ -1625,6 +1627,15 @@ class default_1 extends Controller {
                     return false;
                 }
                 return true;
+            },
+            onBeforeNodeDiscarded(node) {
+                if (!(node instanceof HTMLElement)) {
+                    return true;
+                }
+                if (node.hasAttribute('data-live-ignore')) {
+                    return false;
+                }
+                return true;
             }
         });
         this._exposeOriginalData();
@@ -1665,7 +1676,12 @@ class default_1 extends Controller {
         }
         this._updateModelFromElement(target, 'change');
     }
-    _initiatePolling(rawPollConfig) {
+    _initiatePolling() {
+        this._stopAllPolling();
+        if (this.element.dataset.poll === undefined) {
+            return;
+        }
+        const rawPollConfig = this.element.dataset.poll;
         const directives = parseDirectives(rawPollConfig || '$render');
         directives.forEach((directive) => {
             let duration = 2000;
@@ -1790,9 +1806,12 @@ class default_1 extends Controller {
         const element = this.element;
         this.mutationObserver = new MutationObserver((mutations) => {
             mutations.forEach((mutation) => {
-                if (mutation.type === 'attributes' && !element.dataset.originalData) {
-                    this.originalDataJSON = this.valueStore.asJson();
-                    this._exposeOriginalData();
+                if (mutation.type === 'attributes') {
+                    if (!element.dataset.originalData) {
+                        this.originalDataJSON = this.valueStore.asJson();
+                        this._exposeOriginalData();
+                    }
+                    this._initiatePolling();
                 }
             });
         });
@@ -1803,6 +1822,11 @@ class default_1 extends Controller {
     getDefaultDebounce() {
         return this.hasDebounceValue ? this.debounceValue : DEFAULT_DEBOUNCE;
     }
+    _stopAllPolling() {
+        this.pollingIntervals.forEach((interval) => {
+            clearInterval(interval);
+        });
+    }
 }
 default_1.values = {
     url: String,
diff --git a/src/LiveComponent/assets/src/live_controller.ts b/src/LiveComponent/assets/src/live_controller.ts
@@ -685,6 +685,13 @@ export default class extends Controller implements LiveController {
     _executeMorphdom(newHtml: string, modifiedElements: Array<HTMLElement>) {
         const newElement = htmlToElement(newHtml);
         morphdom(this.element, newElement, {
+            getNodeKey: (node: Node) => {
+              if (!(node instanceof HTMLElement)) {
+                  return;
+              }
+
+              return node.dataset.liveId;
+            },
             onBeforeElUpdated: (fromEl, toEl) => {
                 if (!(fromEl instanceof HTMLElement) || !(toEl instanceof HTMLElement)) {
                     return false;
@@ -726,6 +733,18 @@ export default class extends Controller implements LiveController {
                     return false;
                 }
 
+                return true;
+            },
+
+            onBeforeNodeDiscarded(node) {
+                if (!(node instanceof HTMLElement)) {
+                    // text element
+                    return true;
+                }
+
+                if (node.hasAttribute('data-live-ignore')) {
+                    return false;
+                }
                 return true;
             }
         });
diff --git a/src/LiveComponent/assets/test/controller/render.test.ts b/src/LiveComponent/assets/test/controller/render.test.ts
@@ -13,6 +13,7 @@ import { shutdownTest, createTest, initComponent } from '../tools';
 import { createEvent, fireEvent, getByText, waitFor } from '@testing-library/dom';
 import userEvent from '@testing-library/user-event';
 import fetchMock from 'fetch-mock-jest';
+import { htmlToElement } from '../../src/dom_utils';
 
 describe('LiveController rendering Tests', () => {
     afterEach(() => {
@@ -166,6 +167,7 @@ describe('LiveController rendering Tests', () => {
 
         // imitate some JavaScript changing this element
         test.element.querySelector('span')?.setAttribute('data-foo', 'bar');
+        test.element.appendChild(htmlToElement('<div data-live-ignore>I should not be removed</div>'));
 
         test.expectsAjaxCall('get')
             .expectSentData(test.initialData)
@@ -181,6 +183,38 @@ describe('LiveController rendering Tests', () => {
         const ignoreElement = test.element.querySelector('div[data-live-ignore]');
         expect(ignoreElement).not.toBeNull();
         expect(ignoreElement?.outerHTML).toEqual('<div data-live-ignore="">Inside Ignore Name: <span data-foo="bar">Ryan</span></div>');
+        expect(test.element.innerHTML).toContain('I should not be removed');
+    });
+
+    it('if data-live-id changes, data-live-ignore elements ARE re-rendered', async () => {
+        const test = await createTest({ firstName: 'Ryan', containerId: 'original' }, (data: any) => `
+            <div ${initComponent(data)}>
+                <div data-live-id="${data.containerId}">
+                    <div data-live-ignore>Inside Ignore Name: <span>${data.firstName}</span></div>
+                </div>
+                
+                Outside Ignore Name: ${data.firstName}
+
+                <button data-action="live#$render">Reload</button>
+            </div>
+        `);
+
+        test.expectsAjaxCall('get')
+            .expectSentData(test.initialData)
+            .serverWillChangeData((data: any) => {
+                // change the data on the server so the template renders differently
+                data.firstName = 'Kevin';
+                data.containerId = 'updated';
+            })
+            .init();
+
+        getByText(test.element, 'Reload').click();
+
+        await waitFor(() => expect(test.element).toHaveTextContent('Outside Ignore Name: Kevin'));
+        const ignoreElement = test.element.querySelector('div[data-live-ignore]');
+        expect(ignoreElement).not.toBeNull();
+        // check that even the ignored element re-rendered
+        expect(ignoreElement?.outerHTML).toEqual('<div data-live-ignore="">Inside Ignore Name: <span>Kevin</span></div>');
     });
 
     it('cancels a re-render if the page is navigating away', async () => {
diff --git a/src/LiveComponent/src/Resources/doc/index.rst b/src/LiveComponent/src/Resources/doc/index.rst
@@ -1730,7 +1730,7 @@ changes, a child component will re-render even though it was there
 before *and* after the list changed. This can cause that child component
 to lose some state (i.e. it re-renders with its original live props data).
 
-To fix this, add a unique ``id`` attribute to the root component of each
+To fix this, add a unique ``data-live-id`` attribute to the root component of each
 child element. This will helps LiveComponent identify each item in the
 list and correctly determine if a re-render is necessary, or not.
 
@@ -1748,6 +1748,12 @@ To handle this, add the ``data-live-ignore`` attribute to the element:
 
     <input name="favorite_color" data-live-ignore>
 
+.. note::
+
+    To *force* an ignored element to re-render, give its parent element a
+    ``data-live-id`` attribute. During a re-render, if this value changes, all
+    of the children of the element will be re-rendered, even those with ``data-live-ignore``.
+
 Backward Compatibility promise
 ------------------------------
 
