fix(field): don't return Unparseable on empty strings

Author: tlouisse

packages/choice-input/src/ChoiceInputMixin.js

@@ -212,6 +212,13 @@ export const ChoiceInputMixin = superclass =>
       };
     }
 
+    /**
+     * @override
+     * Overridden from FormatMixin, since a different modelValue is used for choice inputs.
+     * Synchronization from user input is already arranged in this Mixin.
+     */
+    _syncValueUpwards() {}
+
     /**
      * @deprecated use .checked
      */

packages/choice-input/test/ChoiceInputMixin.test.js

@@ -12,8 +12,6 @@ describe('ChoiceInputMixin', () => {
         if (super.connectedCallback) super.connectedCallback();
         this.type = 'checkbox'; // could also be 'radio', should be tested in integration test
       }
-
-      _syncValueUpwards() {} // We need to disable the method for the test to pass
     }
     customElements.define('choice-input', ChoiceInput);
   });
@@ -82,7 +80,6 @@ describe('ChoiceInputMixin', () => {
     const nativeInput = el.inputElement;
     nativeInput.dispatchEvent(new CustomEvent('input', { bubbles: true })); // fired by (at least) Chrome
     expect(counter).to.equal(0);
-    el._syncValueUpwards = () => {}; // We need to disable the method for the test to pass
     nativeInput.dispatchEvent(new CustomEvent('change', { bubbles: true }));
     expect(counter).to.equal(1);
   });
@@ -105,8 +102,6 @@ describe('ChoiceInputMixin', () => {
       const precheckedElementAttr = await fixture(html`
         <choice-input .checked=${true}></choice-input>
       `);
-      el._syncValueUpwards = () => {}; // We need to disable the method for the test to pass
-
       expect(precheckedElementAttr.checked).to.equal(true, 'initially checked via attribute');
     });
 
@@ -122,7 +117,6 @@ describe('ChoiceInputMixin', () => {
 
     it('can be checked and unchecked via user interaction', async () => {
       const el = await fixture(`<choice-input></choice-input>`);
-      el._syncValueUpwards = () => {}; // We need to disable the method for the test to pass
       el.inputElement.click();
       expect(el.checked).to.be.true;
       el.inputElement.click();

packages/field/src/FormatMixin.js

@@ -171,11 +171,37 @@ export const FormatMixin = dedupeMixin(
       }
 
       __callParser(value = this.formattedValue) {
-        let result;
-        if (typeof value === 'string') {
-          result = this.parser(value, this.formatOptions);
+        // A) check if we need to parse at all
+
+        // A.1) The end user had no intention to parse
+        if (value === '') {
+          // Ideally, modelValue should be undefined for empty strings.
+          // For backwards compatibility we return an empty string:
+          // - it triggers validation for required validators (see ValidateMixin.validate())
+          // - it can be expected by 3rd parties (for instance unit tests)
+          // TODO: In a breaking refactor of the Validation System, this behaviot can be corrected.
+          return '';
+        }
+
+        // A.2) Handle edge cases We might have no view value yet, for instance because
+        // inputElement.value was not available yet
+        if (typeof value !== 'string') {
+          // This means there is nothing to find inside the view that can be of
+          // interest to the Application Developer or needed to store for future form state
+          // retrieval.
+          return undefined;
         }
-        return typeof result !== 'undefined' ? result : new Unparseable(value);
+
+        // B) parse the view value
+
+        // - if result:
+        // return the successfully parsed viewValue
+        // - if no result:
+        // Apparently, the parser was not able to produce a satisfactory output for the desired
+        // modelValue type, based on the current viewValue. Unparseable allows to restore all
+        // states (for instance from a lost user session), since it saves the current viewValue.
+        const result = this.parser(value, this.formatOptions);
+        return result !== undefined ? result : new Unparseable(value);
       }
 
       __callFormatter() {

packages/field/test/FormatMixin.test.js

@@ -193,7 +193,7 @@ describe('FormatMixin', () => {
       expect(formatterSpy.args[0][1].decimalSeparator).to.equal('-');
     });
 
-    it('will only call the parser for string values', async () => {
+    it('will only call the parser for defined values', async () => {
       const parserSpy = sinon.spy();
       const el = await fixture(html`
         <${elem} .parser="${parserSpy}">
@@ -202,8 +202,25 @@ describe('FormatMixin', () => {
       `);
       el.modelValue = 'foo';
       expect(parserSpy.callCount).to.equal(1);
+      // This could happen for instance in a reset
       el.modelValue = undefined;
       expect(parserSpy.callCount).to.equal(1);
+      // This could happen when the user erases the input value
+      mimicUserInput(el, '');
+      expect(parserSpy.callCount).to.equal(1);
+    });
+
+    it('will not return Unparseable when empty strings are inputted', async () => {
+      const el = await fixture(html`
+        <${elem}>
+          <input slot="input" value="string">
+        </${elem}>
+      `);
+      // This could happen when the user erases the input value
+      mimicUserInput(el, '');
+      // For backwards compatibility, we keep the modelValue an empty string here.
+      // Undefined would be more appropriate 'conceptually', however
+      expect(el.modelValue).to.equal('');
     });
 
     it('will only call the formatter for valid values', async () => {