bug #523 [Live] Fixed bug with "unsynced" inputs and <form data-model> (weaverryan)

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

Discussion
----------

[Live] Fixed bug with "unsynced" inputs and <form data-model>

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       |  none
| License       | MIT

This fixes an unreleased bug. In a somewhat edge-case use with forms, as you typed into the fields, the values you were typing would get run-over/replaced by the old values from the server - i.e. some of what you are typing gets deleted as you type. That ends up being 0️⃣ fun!

The technical details:

> If a field is modified during an Ajax request, previously, for model fields, after re-rendering, the modified values from the ValueStore were set back onto those fields via SetValueOntoModelFieldsPlugin. Effectively, morphdom would use the new value from the server (which is not correct) momentarily, and then we would reset it back to the modified value.

> However, that didn't work for fields inside of a <form data-model> that use the name attribute, as the "auto-set model field values" functionality purposely doesn't apply to those.

> So, the system was made smarter. It now keeps track of *all* field elements that have been modified (i.e. are unsynced). Then, only when those values are actually *sent* to the server on an Ajax request, those are removed as unsynced elements. This means that morphdom will now see all unsynced fields during re-rendering and will prefer the browser values over the server values.

Cheers!

Commits
-------

ecf124f [Live] Fixed bug with "unsynced" inputs and <form data-model>

Author: weaverryan

src/LiveComponent/assets/dist/live_controller.js

@@ -1184,7 +1184,7 @@ function normalizeAttributesForComparison(element) {
     });
 }
 
-function executeMorphdom(rootFromElement, rootToElement, modifiedElements, getElementValue, childComponents, findChildComponent, getKeyFromElement) {
+function executeMorphdom(rootFromElement, rootToElement, modifiedFieldElements, getElementValue, childComponents, findChildComponent, getKeyFromElement) {
     const childComponentMap = new Map();
     childComponents.forEach((childComponent) => {
         childComponentMap.set(childComponent.element, childComponent);
@@ -1215,7 +1215,7 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedElements, getEl
             if (childComponent) {
                 return childComponent.updateFromNewElement(toEl);
             }
-            if (modifiedElements.includes(fromEl)) {
+            if (modifiedFieldElements.includes(fromEl)) {
                 setValueOnElement(toEl, getElementValue(fromEl));
             }
             if (fromEl.isEqualNode(toEl)) {
@@ -1238,35 +1238,6 @@ function executeMorphdom(rootFromElement, rootToElement, modifiedElements, getEl
     });
 }
 
-/******************************************************************************
-Copyright (c) Microsoft Corporation.
-
-Permission to use, copy, modify, and/or distribute this software for any
-purpose with or without fee is hereby granted.
-
-THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH
-REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT,
-INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR
-OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-PERFORMANCE OF THIS SOFTWARE.
-***************************************************************************** */
-
-function __classPrivateFieldGet(receiver, state, kind, f) {
-    if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a getter");
-    if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot read private member from an object whose class did not declare it");
-    return kind === "m" ? f : kind === "a" ? f.call(receiver) : f ? f.value : state.get(receiver);
-}
-
-function __classPrivateFieldSet(receiver, state, value, kind, f) {
-    if (kind === "m") throw new TypeError("Private method is not writable");
-    if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
-    if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
-    return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
-}
-
-var _UnsyncedInputContainer_mappedFields, _UnsyncedInputContainer_unmappedFields;
 class UnsyncedInputsTracker {
     constructor(component, modelElementResolver) {
         this.elementEventListeners = [
@@ -1307,36 +1278,51 @@ class UnsyncedInputsTracker {
         this.unsyncedInputs.add(element, modelName);
     }
     getUnsyncedInputs() {
-        return this.unsyncedInputs.all();
+        return this.unsyncedInputs.allUnsyncedInputs();
+    }
+    getUnsyncedModels() {
+        return Array.from(this.unsyncedInputs.getUnsyncedModelNames());
     }
-    getModifiedModels() {
-        return Array.from(this.unsyncedInputs.getModifiedModels());
+    resetUnsyncedFields() {
+        this.unsyncedInputs.resetUnsyncedFields();
     }
 }
 class UnsyncedInputContainer {
     constructor() {
-        _UnsyncedInputContainer_mappedFields.set(this, void 0);
-        _UnsyncedInputContainer_unmappedFields.set(this, []);
-        __classPrivateFieldSet(this, _UnsyncedInputContainer_mappedFields, new Map(), "f");
+        this.unsyncedNonModelFields = [];
+        this.unsyncedModelNames = [];
+        this.unsyncedModelFields = new Map();
     }
     add(element, modelName = null) {
         if (modelName) {
-            __classPrivateFieldGet(this, _UnsyncedInputContainer_mappedFields, "f").set(modelName, element);
+            this.unsyncedModelFields.set(modelName, element);
+            if (!this.unsyncedModelNames.includes(modelName)) {
+                this.unsyncedModelNames.push(modelName);
+            }
             return;
         }
-        __classPrivateFieldGet(this, _UnsyncedInputContainer_unmappedFields, "f").push(element);
+        this.unsyncedNonModelFields.push(element);
     }
-    all() {
-        return [...__classPrivateFieldGet(this, _UnsyncedInputContainer_unmappedFields, "f"), ...__classPrivateFieldGet(this, _UnsyncedInputContainer_mappedFields, "f").values()];
+    resetUnsyncedFields() {
+        this.unsyncedModelFields.forEach((value, key) => {
+            if (!this.unsyncedModelNames.includes(key)) {
+                this.unsyncedModelFields.delete(key);
+            }
+        });
+    }
+    allUnsyncedInputs() {
+        return [...this.unsyncedNonModelFields, ...this.unsyncedModelFields.values()];
     }
     markModelAsSynced(modelName) {
-        __classPrivateFieldGet(this, _UnsyncedInputContainer_mappedFields, "f").delete(modelName);
+        const index = this.unsyncedModelNames.indexOf(modelName);
+        if (index !== -1) {
+            this.unsyncedModelNames.splice(index, 1);
+        }
     }
-    getModifiedModels() {
-        return Array.from(__classPrivateFieldGet(this, _UnsyncedInputContainer_mappedFields, "f").keys());
+    getUnsyncedModelNames() {
+        return this.unsyncedModelNames;
     }
 }
-_UnsyncedInputContainer_mappedFields = new WeakMap(), _UnsyncedInputContainer_unmappedFields = new WeakMap();
 
 class HookManager {
     constructor() {
@@ -1452,7 +1438,7 @@ class Component {
         return promise;
     }
     getUnsyncedModels() {
-        return this.unsyncedInputsTracker.getModifiedModels();
+        return this.unsyncedInputsTracker.getUnsyncedModels();
     }
     addChild(child, modelBindings = []) {
         if (!child.id) {
@@ -1521,6 +1507,7 @@ class Component {
     performRequest() {
         const thisPromiseResolve = this.nextRequestPromiseResolve;
         this.resetPromise();
+        this.unsyncedInputsTracker.resetUnsyncedFields();
         this.backendRequest = this.backend.makeRequest(this.valueStore.all(), this.pendingActions, this.valueStore.updatedModels, this.getChildrenFingerprints());
         this.hooks.triggerHook('loading.state:started', this.element, this.backendRequest);
         this.pendingActions = [];

src/LiveComponent/assets/src/Component/UnsyncedInputsTracker.ts

@@ -58,49 +58,87 @@ export default class {
     }
 
     getUnsyncedInputs(): HTMLElement[] {
-        return this.unsyncedInputs.all();
+        return this.unsyncedInputs.allUnsyncedInputs();
     }
 
-    getModifiedModels(): string[] {
-        return Array.from(this.unsyncedInputs.getModifiedModels());
+    getUnsyncedModels(): string[] {
+        return Array.from(this.unsyncedInputs.getUnsyncedModelNames());
+    }
+
+    resetUnsyncedFields(): void {
+        this.unsyncedInputs.resetUnsyncedFields();
     }
 }
 
 /**
  * Tracks field & models whose values are "unsynced".
  *
- * Unsynced means that the value has been updated inside of
+ * For a model, unsynced means that the value has been updated inside of
  * a field (e.g. an input), but that this new value hasn't
  * yet been set onto the actual model data. It is "unsynced"
  * from the underlying model data.
+ *
+ * For a field, unsynced means that it is "modified on the client side". In
+ * other words, the field's value in the browser would be different than the
+ * one returned from the server. This can happen because a field has no model
+ * (and so it is permanently unsynced once changed) or the field has been changed
+ * and the corresponding model has not yet been sent to the server.
+ *
+ * Note: a "model" can become synced when that value is set back
+ * onto the data store. But the corresponding field will
+ * remain unsynced until the next Ajax call starts.
  */
 export class UnsyncedInputContainer {
-    #mappedFields: Map<string, HTMLElement>;
-    #unmappedFields: Array<HTMLElement> = [];
+    private unsyncedModelFields: Map<string, HTMLElement>;
+    private unsyncedNonModelFields: Array<HTMLElement> = [];
+    private unsyncedModelNames: Array<string> = [];
 
     constructor() {
-        this.#mappedFields = new Map();
+        this.unsyncedModelFields = new Map();
     }
 
     add(element: HTMLElement, modelName: string|null = null) {
         if (modelName) {
-            this.#mappedFields.set(modelName, element);
+            this.unsyncedModelFields.set(modelName, element);
+            if (!this.unsyncedModelNames.includes(modelName)) {
+                this.unsyncedModelNames.push(modelName);
+            }
 
             return;
         }
 
-        this.#unmappedFields.push(element);
+        this.unsyncedNonModelFields.push(element);
     }
 
-    all(): HTMLElement[] {
-        return [...this.#unmappedFields, ...this.#mappedFields.values()]
+    /**
+     * Mark all fields as synced, except for those not bound to a model or whose
+     * values are still dirty.
+     */
+    resetUnsyncedFields(): void {
+        // clear out all unsynced fields, except those where the value is still unsynced
+        this.unsyncedModelFields.forEach((value, key) => {
+            if (!this.unsyncedModelNames.includes(key)) {
+                this.unsyncedModelFields.delete(key);
+            }
+        });
+    }
+
+    allUnsyncedInputs(): HTMLElement[] {
+        return [...this.unsyncedNonModelFields, ...this.unsyncedModelFields.values()]
     }
 
     markModelAsSynced(modelName: string): void {
-        this.#mappedFields.delete(modelName);
+        const index = this.unsyncedModelNames.indexOf(modelName);
+        if (index !== -1) {
+            this.unsyncedModelNames.splice(index, 1);
+        }
     }
 
-    getModifiedModels(): string[] {
-        return Array.from(this.#mappedFields.keys());
+    /**
+     * Returns a list of models whose fields have been modified, but whose values
+     * have not yet been set onto the data store.
+     */
+    getUnsyncedModelNames(): string[] {
+        return this.unsyncedModelNames;
     }
 }

src/LiveComponent/assets/src/Component/index.ts

@@ -164,8 +164,12 @@ export default class Component {
         return promise;
     }
 
+    /**
+     * Returns an array of models the user has modified, but whose model has not
+     * yet been updated.
+     */
     getUnsyncedModels(): string[] {
-        return this.unsyncedInputsTracker.getModifiedModels();
+        return this.unsyncedInputsTracker.getUnsyncedModels();
     }
 
     addChild(child: Component, modelBindings: ModelBinding[] = []): void {
@@ -277,6 +281,10 @@ export default class Component {
         // then create a fresh Promise, so any future .then() apply to it
         this.resetPromise();
 
+        // any fields that were modified will now be sent on this request:
+        // they are now "in sync" (with some exceptions noted inside)
+        this.unsyncedInputsTracker.resetUnsyncedFields();
+
         this.backendRequest = this.backend.makeRequest(
             this.valueStore.all(),
             this.pendingActions,

src/LiveComponent/assets/src/morphdom.ts

@@ -12,7 +12,7 @@ import Component from './Component';
 export function executeMorphdom(
     rootFromElement: HTMLElement,
     rootToElement: HTMLElement,
-    modifiedElements: Array<HTMLElement>,
+    modifiedFieldElements: Array<HTMLElement>,
     getElementValue: (element: HTMLElement) => any,
     childComponents: Component[],
     findChildComponent: (id: string, element: HTMLElement) => HTMLElement|null,
@@ -57,7 +57,7 @@ export function executeMorphdom(
 
             // if this field's value has been modified since this HTML was
             // requested, set the toEl's value to match the fromEl
-            if (modifiedElements.includes(fromEl)) {
+            if (modifiedFieldElements.includes(fromEl)) {
                 setValueOnElement(toEl, getElementValue(fromEl))
             }
 

src/LiveComponent/assets/test/UnsyncedInputContainer.test.ts

@@ -9,10 +9,10 @@ describe('UnsyncedInputContainer', () => {
         container.add(element1);
         container.add(element2, 'some_model');
 
-        expect(container.all()).toEqual([element1, element2]);
+        expect(container.allUnsyncedInputs()).toEqual([element1, element2]);
     });
 
-    it('markModelAsSynced removes items added to it', () => {
+    it('markModelAsSynced removes unsynced models but not fields', () => {
         const container = new UnsyncedInputContainer();
         const element1 = htmlToElement('<span>element1</span');
         const element2 = htmlToElement('<span>element2</span');
@@ -23,7 +23,7 @@ describe('UnsyncedInputContainer', () => {
 
         container.markModelAsSynced('some_model2');
 
-        expect(container.all()).toEqual([element1, element3]);
+        expect(container.allUnsyncedInputs()).toEqual([element1, element2, element3]);
     });
 
     it('returns modified models via getModifiedModels()', () => {
@@ -36,6 +36,21 @@ describe('UnsyncedInputContainer', () => {
         container.add(element3, 'some_model3');
 
         container.markModelAsSynced('some_model2');
-        expect(container.getModifiedModels()).toEqual(['some_model3'])
+        expect(container.getUnsyncedModelNames()).toEqual(['some_model3'])
+    });
+
+    it('resetUnsyncedFields removes all model fields except those unsynced', () => {
+        const container = new UnsyncedInputContainer();
+        const element1 = htmlToElement('<span>element1</span');
+        const element2 = htmlToElement('<span>element2</span');
+        const element3 = htmlToElement('<span>element3</span');
+        container.add(element1);
+        container.add(element2, 'some_model2');
+        container.add(element3, 'some_model3');
+
+        container.markModelAsSynced('some_model2');
+
+        container.resetUnsyncedFields();
+        expect(container.allUnsyncedInputs()).toEqual([element1, element3]);
     });
 });

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

@@ -687,6 +687,59 @@ describe('LiveController data-model Tests', () => {
         expect(unmappedTextarea.getAttribute('class')).toEqual('changed-class');
     });
 
+    it('keeps the unsynced value of a model field mapped via a form', async () => {
+        const test = await createTest({
+            comment: 'Live components',
+        }, (data: any) => `
+            <div ${initComponent(data)}>
+                <form data-model>
+                    <textarea name="comment" data-testid="comment">${data.comment}</textarea>
+               </form>
+
+               <button data-action="live#$render">Reload</button>
+           </div>
+       `);
+
+        test.expectsAjaxCall('get')
+            .expectSentData(test.initialData)
+            .serverWillChangeData((data) => {
+                data.comment = 'server tries to change comment, but it will be modified client side';
+            })
+            // delay slightly so we can type in the textarea
+            .delayResponse(10)
+            .init();
+
+        getByText(test.element, 'Reload').click();
+        // mimic changing the field, but without (yet) triggering the change event
+        const commentField = getByTestId(test.element, 'comment');
+        if (!(commentField instanceof HTMLTextAreaElement)) {
+            throw new Error('wrong type');
+        }
+        userEvent.type(commentField, ' ftw!');
+
+        // wait for loading start and end
+        await waitFor(() => expect(test.element).toHaveAttribute('busy'));
+        await waitFor(() => expect(test.element).not.toHaveAttribute('busy'));
+
+        expect(commentField).toHaveValue('Live components ftw!');
+
+        // refresh again, the value should now be in sync and accept the changed
+        // value from the server
+        test.expectsAjaxCall('get')
+            .expectSentData({ comment: 'Live components ftw!' })
+            .serverWillChangeData((data) => {
+                data.comment = 'server changed comment';
+            })
+            .init();
+
+        getByText(test.element, 'Reload').click();
+        // wait for loading start and end
+        await waitFor(() => expect(test.element).toHaveAttribute('busy'));
+        await waitFor(() => expect(test.element).not.toHaveAttribute('busy'));
+
+        expect(commentField).toHaveValue('server changed comment');
+    });
+
     it('allows model fields to be manually set as long as change event is dispatched', async () => {
         const test = await createTest({ food: '' }, (data: any) => `
             <div ${initComponent(data)}>