feature #250 [Live] Fixing bug where inputs would not re-render if the value changed (weaverryan)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Live] Fixing bug where inputs would not re-render if the value changed

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       | Fix #144 and Bug E on #102.
| License       | MIT

~~NOTE to self: I'm considering reversing this PR as it has some side effects where we lose input values on re-render. That can be solved with `data-live-ignore`, but it may be better to reverse this, and "fix" the original problem by telling the user to add a `data-live-update` attribute (or something like that) where we opt INTO re-rendering. There is basically a situation where we don't know if the user will want a form element to re-render (and update the value) or not. And so, the user needs to choose. The question is, which way should the "default" be.~~

For example, if an input rendered initially empty, then you typed
into it, and, on re-render, the value was set BACK to the original,
the input would *not* update on re-render, and it would be stuck
with the old value.

This was because the old input node and new input node were seen
as identical... and so morphdom didn't bother to update that node.
However, in reality, the value property of the "old" input had
been changed and would no longer match the value "attribute" of the
incoming input. Effectively, the 2 inputs had different values, but
this difference was not caught by the system.

Commits
-------

a27fdd3 [Live] Fixing bug where inputs would not re-render if the value changed

Author: weaverryan

src/LiveComponent/CHANGELOG.md

@@ -2,10 +2,13 @@
 
 ## 2.1.0
 
+-   Added `data-live-ignore` attribute. If included in an element, that element
+    will not be updated on re-render.
+
 -   The Live Component AJAX endpoints now return HTML in all situations
     instead of JSON.
 
--   Send live action arguments to backend
+-   Ability to send live action arguments to backend
 
 -   [BC BREAK] Remove `init_live_component()` twig function, use `{{ attributes }}` instead:
     ```diff

src/LiveComponent/assets/src/clone_html_element.ts

@@ -0,0 +1,8 @@
+export function cloneHTMLElement(element: HTMLElement): HTMLElement {
+    const newElement = element.cloneNode(true);
+    if (!(newElement instanceof HTMLElement)) {
+        throw new Error('Could not clone element');
+    }
+
+    return newElement;
+}

src/LiveComponent/assets/src/live_controller.ts

@@ -5,6 +5,8 @@ import { combineSpacedArray } from './string_utils';
 import { buildFormData, buildSearchParams } from './http_data_helper';
 import { setDeepData, doesDeepPropertyExist, normalizeModelName } from './set_deep_data';
 import { haveRenderedValuesChanged } from './have_rendered_values_changed';
+import { normalizeAttributesForComparison } from './normalize_attributes_for_comparison';
+import { cloneHTMLElement } from './clone_html_element';
 
 interface ElementLoadingDirectives {
     element: HTMLElement,
@@ -197,11 +199,7 @@ export default class extends Controller {
         const model = element.dataset.model || element.getAttribute('name');
 
         if (!model) {
-            const clonedElement = (element.cloneNode());
-            // helps typescript know this is an HTMLElement
-            if (!(clonedElement instanceof HTMLElement)) {
-                throw new Error('cloneNode() produced incorrect type');
-            }
+            const clonedElement = cloneHTMLElement(element);
 
             throw new Error(`The update() method could not be called for "${clonedElement.outerHTML}": the element must either have a "data-model" or "name" attribute set to the model name.`);
         }
@@ -558,7 +556,18 @@ export default class extends Controller {
             onBeforeElUpdated: (fromEl, toEl) => {
                 // https://github.com/patrick-steele-idem/morphdom#can-i-make-morphdom-blaze-through-the-dom-tree-even-faster-yes
                 if (fromEl.isEqualNode(toEl)) {
-                    return false
+                    // the nodes are equal, but the "value" on some might differ
+                    // lets try to quickly compare a bit more deeply
+                    const normalizedFromEl = cloneHTMLElement(fromEl);
+                    normalizeAttributesForComparison(normalizedFromEl);
+
+                    const normalizedToEl = cloneHTMLElement(toEl);
+                    normalizeAttributesForComparison(normalizedToEl);
+
+                    if (normalizedFromEl.isEqualNode(normalizedToEl)) {
+                        // don't bother updating
+                        return false;
+                    }
                 }
 
                 // avoid updating child components: they will handle themselves
@@ -571,6 +580,11 @@ export default class extends Controller {
                     return false;
                 }
 
+                // look for data-live-ignore, and don't update
+                if (fromEl.hasAttribute('data-live-ignore')) {
+                    return false;
+                }
+
                 return true;
             }
         });

src/LiveComponent/assets/src/normalize_attributes_for_comparison.ts

@@ -0,0 +1,18 @@
+/**
+ * Updates an HTML node to represent its underlying data.
+ *
+ * For example, this finds the value property of each underlying node
+ * and sets that onto the value attribute. This is useful to compare
+ * if two nodes are identical.
+ */
+export function normalizeAttributesForComparison(element: HTMLElement): void {
+    if (element.value) {
+        element.setAttribute('value', element.value);
+    } else if (element.hasAttribute('value')) {
+        element.setAttribute('value', '');
+    }
+
+    Array.from(element.children).forEach((child: HTMLElement) => {
+        normalizeAttributesForComparison(child);
+    });
+}

src/LiveComponent/assets/test/controller/action.test.ts

@@ -42,6 +42,9 @@ describe('LiveController Action Tests', () => {
 
     afterEach(() => {
         clearDOM();
+        if (!fetchMock.done()) {
+            throw new Error('Mocked requests did not match');
+        }
         fetchMock.reset();
     });
 
@@ -62,16 +65,14 @@ describe('LiveController Action Tests', () => {
         await waitFor(() => expect(element).toHaveTextContent('Comment Saved!'));
         expect(getByLabelText(element, 'Comments:')).toHaveValue('hi weaver');
 
-        fetchMock.done();
-
         expect(postMock.lastOptions().body.get('comments')).toEqual('hi WEAVER');
     });
 
     it('Sends action named args', async () => {
         const data = { comments: 'hi' };
         const { element } = await startStimulus(template(data));
 
-        fetchMock.postOnce('http://localhost/_components/my_component/sendNamedArgs?values=a%3D1%26b%3D2%26c%3D3', {
+        fetchMock.postOnce('http://localhost/_components/my_component/sendNamedArgs?args=a%3D1%26b%3D2%26c%3D3', {
             html: template({ comments: 'hi' }),
         });
 

src/LiveComponent/assets/test/controller/child.test.ts

@@ -72,6 +72,9 @@ describe('LiveController parent -> child component tests', () => {
 
     afterEach(() => {
         clearDOM();
+        if (!fetchMock.done()) {
+            throw new Error('Mocked requests did not match');
+        }
         fetchMock.reset();
     });
 

src/LiveComponent/assets/test/controller/csrf.test.ts

@@ -40,6 +40,9 @@ describe('LiveController CSRF Tests', () => {
 
     afterEach(() => {
         clearDOM();
+        if (!fetchMock.done()) {
+            throw new Error('Mocked requests did not match');
+        }
         fetchMock.reset();
     });
 
@@ -56,7 +59,5 @@ describe('LiveController CSRF Tests', () => {
         await waitFor(() => expect(element).toHaveTextContent('Comment Saved!'));
 
         expect(postMock.lastOptions().headers['X-CSRF-TOKEN']).toEqual('123TOKEN');
-
-        fetchMock.done();
     });
 });

src/LiveComponent/assets/test/controller/model.test.ts

@@ -34,6 +34,9 @@ describe('LiveController data-model Tests', () => {
 
     afterEach(() => {
         clearDOM();
+        if (!fetchMock.done()) {
+            throw new Error('Mocked requests did not match');
+        }
         fetchMock.reset();
     });
 
@@ -52,9 +55,6 @@ describe('LiveController data-model Tests', () => {
         await waitFor(() => expect(getByLabelText(element, 'Name:')).toHaveValue('Ryan Weaver'));
         expect(controller.dataValue).toEqual({name: 'Ryan Weaver'});
 
-        // assert all calls were done the correct number of times
-        fetchMock.done();
-
         // assert the input is still focused after rendering
         expect(document.activeElement.dataset.model).toEqual('name');
     });
@@ -69,9 +69,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(getByLabelText(element, 'Name:')).toHaveValue('Jan'));
         expect(controller.dataValue).toEqual({name: 'Jan'});
-
-        // assert all calls were done the correct number of times
-        fetchMock.done();
     });
 
 
@@ -111,9 +108,6 @@ describe('LiveController data-model Tests', () => {
         await waitFor(() => expect(getByLabelText(element, 'Name:')).toHaveValue('Ryanguy_'));
         expect(controller.dataValue).toEqual({name: 'Ryanguy_'});
 
-        // assert all calls were done the correct number of times
-        fetchMock.done();
-
         // only 1 render should have ultimately occurred
         expect(renderCount).toEqual(1);
     });
@@ -135,9 +129,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('Ryan Weaver'));
         expect(controller.dataValue).toEqual({name: 'Ryan Weaver'});
-
-        // assert all calls were done the correct number of times
-        fetchMock.done();
     });
 
     it('uses data-model when both name and data-model is present', async () => {
@@ -157,8 +148,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('Ryan Weaver'));
         expect(controller.dataValue).toEqual({name: 'Ryan Weaver'});
-
-        fetchMock.done();
     });
 
     it('uses data-value when both value and data-value is present', async () => {
@@ -178,8 +167,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('first_name'));
         expect(controller.dataValue).toEqual({name: 'first_name'});
-
-        fetchMock.done();
     });
 
     it('standardizes user[firstName] style models into post.name', async () => {
@@ -211,9 +198,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('Ryan Weaver'));
         expect(controller.dataValue).toEqual({ user: { firstName: 'Ryan Weaver' } });
-
-        // assert all calls were done the correct number of times
-        fetchMock.done();
     });
 
     it('updates correctly when live#update is on a parent element', async () => {
@@ -245,8 +229,6 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('Ryan Weaver'));
 
-        fetchMock.done();
-
         // assert the input is still focused after rendering
         expect(document.activeElement.getAttribute('name')).toEqual('firstName');
     });
@@ -264,9 +246,6 @@ describe('LiveController data-model Tests', () => {
         await userEvent.type(inputElement, ' WEAVER');
 
         await waitFor(() => expect(inputElement).toHaveValue('Ryan Weaver'));
-
-        // assert all calls were done the correct number of times
-        fetchMock.done();
     });
 
     it('data changed on server should be noticed by controller', async () => {
@@ -283,7 +262,5 @@ describe('LiveController data-model Tests', () => {
 
         await waitFor(() => expect(inputElement).toHaveValue('Kevin Bond'));
         expect(controller.dataValue).toEqual({name: 'Kevin Bond'});
-
-        fetchMock.done();
     });
 });

src/LiveComponent/assets/test/controller/render.test.ts

@@ -40,6 +40,9 @@ describe('LiveController rendering Tests', () => {
 
     afterEach(() => {
         clearDOM();
+        if (!fetchMock.done()) {
+            throw new Error('Mocked requests did not match');
+        }
         fetchMock.reset();
     });
 
@@ -57,19 +60,46 @@ describe('LiveController rendering Tests', () => {
         expect(controller.dataValue).toEqual({name: 'Kevin'});
     });
 
-    it('conserves values of fields modified after a render request', async () => {
+    it('renders over local value in input', async () => {
         const data = { name: 'Ryan' };
         const { element } = await startStimulus(template(data));
 
         fetchMock.get(
+            // name=Ryan is sent to the server
             'http://localhost/_components/my_component?name=Ryan',
+            // server changes that data
             template({ name: 'Kevin' }),
             { delay: 100 }
         );
-        getByText(element, 'Reload').click();
+        // type into the input that is not bound to a model
         userEvent.type(getByLabelText(element, 'Comments:'), '!!');
+        getByText(element, 'Reload').click();
 
         await waitFor(() => expect(element).toHaveTextContent('Name: Kevin'));
+        // value if unmapped input is reset
+        expect(getByLabelText(element, 'Comments:')).toHaveValue('i like pizza');
+        expect(document.activeElement.name).toEqual('comments');
+    });
+
+    it('conserves values of fields modified after a render request IF data-live-ignore', async () => {
+        const data = { name: 'Ryan' };
+        const { element } = await startStimulus(template(data));
+
+        fetchMock.get(
+            // name=Ryan is sent to the server
+            'http://localhost/_components/my_component?name=Ryan',
+            // server changes that data
+            template({ name: 'Kevin' }),
+            { delay: 100 }
+        );
+        // type into the input that is not bound to a model
+        const input = getByLabelText(element, 'Comments:');
+        input.setAttribute('data-live-ignore', '');
+        userEvent.type(input, '!!');
+        getByText(element, 'Reload').click();
+
+        await waitFor(() => expect(element).toHaveTextContent('Name: Kevin'));
+        // value of unmapped input is NOT reset because of data-live-ignore
         expect(getByLabelText(element, 'Comments:')).toHaveValue('i like pizza!!');
         expect(document.activeElement.name).toEqual('comments');
     });
@@ -88,8 +118,10 @@ describe('LiveController rendering Tests', () => {
             template({ name: 'Kevin' }, true),
             { delay: 100 }
         );
+        const input = getByLabelText(element, 'Comments:');
+        input.setAttribute('data-live-ignore', '');
+        userEvent.type(input, '!!');
         getByText(element, 'Reload').click();
-        userEvent.type(getByLabelText(element, 'Comments:'), '!!');
 
         await waitFor(() => expect(element).toHaveTextContent('Name: Kevin'));
         expect(getByLabelText(element, 'Comments:')).toHaveValue('i like pizza!!');

src/LiveComponent/assets/test/normalize_attributes_for_comparison.test.ts

@@ -0,0 +1,64 @@
+import { normalizeAttributesForComparison } from '../src/normalize_attributes_for_comparison';
+
+const createElement = function(html: string): HTMLElement {
+    const template = document.createElement('template');
+    html = html.trim();
+    template.innerHTML = html;
+
+    const child = template.content.firstChild;
+    if (!child || !(child instanceof HTMLElement)) {
+        throw new Error('Child not found');
+    }
+
+    return child;
+}
+
+describe('normalizeAttributesForComparison', () => {
+    it('makes no changes if value and attribute not set', () => {
+        const element = createElement('<div class="foo"></div>');
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<div class="foo"></div>');
+    });
+
+    it('sets the attribute if the value is present', () => {
+        const element = createElement('<input class="foo">');
+        element.value = 'set value';
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<input class="foo" value="set value">');
+    });
+
+    it('sets the attribute to empty if the value is empty', () => {
+        const element = createElement('<input class="foo" value="starting">');
+        element.value = '';
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<input class="foo" value="">');
+    });
+
+    it('changes the value attribute if value is different', () => {
+        const element = createElement('<input class="foo" value="starting">');
+        element.value = 'changed';
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<input class="foo" value="changed">');
+    });
+
+    it('changes the value attribute on a child', () => {
+        const element = createElement('<div><input id="child" value="original"></div>');
+        element.querySelector('#child').value = 'changed';
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<div><input id="child" value="changed"></div>');
+    });
+
+    it('changes the value on multiple levels', () => {
+        const element = createElement('<div><input id="child" value="original"><div><input id="grand_child"></div></div>');
+        element.querySelector('#child').value = 'changed';
+        element.querySelector('#grand_child').value = 'changed grand child';
+        normalizeAttributesForComparison(element);
+        expect(element.outerHTML)
+            .toEqual('<div><input id="child" value="changed"><div><input id="grand_child" value="changed grand child"></div></div>');
+    });
+});