From a27fdd390ee4af0bbce280cfc481accd8c207834 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Thu, 27 Jan 2022 20:26:05 -0500 Subject: [PATCH] [Live] Fixing bug where inputs would not re-render if the value changed --- src/LiveComponent/CHANGELOG.md | 5 +- .../assets/src/clone_html_element.ts | 8 +++ .../assets/src/live_controller.ts | 26 ++++++-- .../normalize_attributes_for_comparison.ts | 18 ++++++ .../assets/test/controller/action.test.ts | 7 +- .../assets/test/controller/child.test.ts | 3 + .../assets/test/controller/csrf.test.ts | 5 +- .../assets/test/controller/model.test.ts | 29 +-------- .../assets/test/controller/render.test.ts | 38 ++++++++++- ...ormalize_attributes_for_comparison.test.ts | 64 +++++++++++++++++++ src/LiveComponent/src/Resources/doc/index.rst | 14 ++++ 11 files changed, 176 insertions(+), 41 deletions(-) create mode 100644 src/LiveComponent/assets/src/clone_html_element.ts create mode 100644 src/LiveComponent/assets/src/normalize_attributes_for_comparison.ts create mode 100644 src/LiveComponent/assets/test/normalize_attributes_for_comparison.test.ts diff --git a/src/LiveComponent/CHANGELOG.md b/src/LiveComponent/CHANGELOG.md index 824a8167d96..352d892ee7b 100644 --- a/src/LiveComponent/CHANGELOG.md +++ b/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 ## 2.0.0 diff --git a/src/LiveComponent/assets/src/clone_html_element.ts b/src/LiveComponent/assets/src/clone_html_element.ts new file mode 100644 index 00000000000..b4ea6b5eecc --- /dev/null +++ b/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; +} diff --git a/src/LiveComponent/assets/src/live_controller.ts b/src/LiveComponent/assets/src/live_controller.ts index e240b35228e..e13768932bb 100644 --- a/src/LiveComponent/assets/src/live_controller.ts +++ b/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; } }); diff --git a/src/LiveComponent/assets/src/normalize_attributes_for_comparison.ts b/src/LiveComponent/assets/src/normalize_attributes_for_comparison.ts new file mode 100644 index 00000000000..011de99400b --- /dev/null +++ b/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); + }); +} diff --git a/src/LiveComponent/assets/test/controller/action.test.ts b/src/LiveComponent/assets/test/controller/action.test.ts index 7f64a798a4e..d2a00ff1fe0 100644 --- a/src/LiveComponent/assets/test/controller/action.test.ts +++ b/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,8 +65,6 @@ 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'); }); @@ -71,7 +72,7 @@ describe('LiveController Action Tests', () => { 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' }), }); diff --git a/src/LiveComponent/assets/test/controller/child.test.ts b/src/LiveComponent/assets/test/controller/child.test.ts index 263d6ac2da9..f313c1bb6a8 100644 --- a/src/LiveComponent/assets/test/controller/child.test.ts +++ b/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(); }); diff --git a/src/LiveComponent/assets/test/controller/csrf.test.ts b/src/LiveComponent/assets/test/controller/csrf.test.ts index 39958236fad..17896ce7d60 100644 --- a/src/LiveComponent/assets/test/controller/csrf.test.ts +++ b/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(); }); }); diff --git a/src/LiveComponent/assets/test/controller/model.test.ts b/src/LiveComponent/assets/test/controller/model.test.ts index 45e4485b4c3..3bed409f951 100644 --- a/src/LiveComponent/assets/test/controller/model.test.ts +++ b/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(); }); }); diff --git a/src/LiveComponent/assets/test/controller/render.test.ts b/src/LiveComponent/assets/test/controller/render.test.ts index 505842472dd..f29f0ee4b91 100644 --- a/src/LiveComponent/assets/test/controller/render.test.ts +++ b/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!!'); diff --git a/src/LiveComponent/assets/test/normalize_attributes_for_comparison.test.ts b/src/LiveComponent/assets/test/normalize_attributes_for_comparison.test.ts new file mode 100644 index 00000000000..f4b83e6a790 --- /dev/null +++ b/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('
'); + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual('
'); + }); + + it('sets the attribute if the value is present', () => { + const element = createElement(''); + element.value = 'set value'; + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual(''); + }); + + it('sets the attribute to empty if the value is empty', () => { + const element = createElement(''); + element.value = ''; + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual(''); + }); + + it('changes the value attribute if value is different', () => { + const element = createElement(''); + element.value = 'changed'; + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual(''); + }); + + it('changes the value attribute on a child', () => { + const element = createElement('
'); + element.querySelector('#child').value = 'changed'; + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual('
'); + }); + + it('changes the value on multiple levels', () => { + const element = createElement('
'); + element.querySelector('#child').value = 'changed'; + element.querySelector('#grand_child').value = 'changed grand child'; + normalizeAttributesForComparison(element); + expect(element.outerHTML) + .toEqual('
'); + }); +}); diff --git a/src/LiveComponent/src/Resources/doc/index.rst b/src/LiveComponent/src/Resources/doc/index.rst index 9ad6ac1748a..a16b0fe97d1 100644 --- a/src/LiveComponent/src/Resources/doc/index.rst +++ b/src/LiveComponent/src/Resources/doc/index.rst @@ -1477,6 +1477,20 @@ form. But it also makes sure that when the ``textarea`` changes, both the ``value`` model in ``MarkdownTextareaComponent`` *and* the ``post.content`` model in ``EditPostcomponent`` will be updated. +Skipping Updating Certain Elements +---------------------------------- + +Sometimes you may have an element inside a component that you do *not* want to +change whenever your component re-renders. For example, some elements managed by +third-party JavaScript or a form element that is not bound to a model... where you +don't want a re-render to reset the data the user has entered. + +To handle this, add the ``data-live-ignore`` attribute to the element: + +.. code-block:: html + + + Backward Compatibility promise ------------------------------