From bf355cbeb3df669b0c577bfb33b3fcad6bb1d61e Mon Sep 17 00:00:00 2001
From: Andrew Seier <andseier@gmail.com>
Date: Wed, 2 Aug 2023 09:57:37 -0700
Subject: [PATCH] Deserialize `""` to `NaN` for `Number` type.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It would always be counter-intuitive to declare `<my-element number>` in
HTML markup and introspect the element to find `element.number === 0`.

In other words, while it’s true that in JS `Number('') === 0`, this is
not sensible behavior when deserializing values in markup into typed
objects in JS.

This is mostly due to the concept of so-called “boolean attributes”. As
a custom element author, you don’t want to coerce a boolean attribute to
a `Number`. In otherwords, when dealing with numbers, the markup
`<my-element number>` should _not_ equal `<my-element number="0">`.

While this change makes a strong opinion, part of the goal of
`x-element` is to reduce obvious boiler plate. Since it seems incredibly
unlikely that any author would actually _desire_ the `Number('') >> 0`
behavior, taking a stance here seems more helpful than hurtful.

Additionally, we will always, conceptually, have a _default_
deserializer. In the future, if needed, we could allow authors to
declare their own deserializers / serializers within property
declarations.

Closes #147.
---
 test/test-basic-properties.js | 13 +++++++++++++
 x-element.js                  |  8 +++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/test/test-basic-properties.js b/test/test-basic-properties.js
index 481e3de..a825ee9 100644
--- a/test/test-basic-properties.js
+++ b/test/test-basic-properties.js
@@ -250,6 +250,19 @@ it('inheritance is considered in type checking', () => {
   assert(el.objectProperty === array, 'property was set');
 });
 
+it('numeric properties deserialize "" (empty) to "NaN"', () => {
+  const el = document.createElement('test-element');
+  el.setAttribute('numeric-property', '0');
+  document.body.append(el);
+  assert(el.numericProperty === 0, '"0" was coerced to 0');
+  el.setAttribute('numeric-property', '');
+  assert(Number.isNaN(el.numericProperty), '"" was coerced to NaN');
+  el.setAttribute('numeric-property', ' ');
+  assert(Number.isNaN(el.numericProperty), '" " was coerced to NaN');
+  el.setAttribute('numeric-property', '      ');
+  assert(Number.isNaN(el.numericProperty), '"      " was coerced to NaN');
+});
+
 it('cannot set to known properties', () => {
   class BadTestElement extends XElement {
     static get properties() {
diff --git a/x-element.js b/x-element.js
index 441ee4b..8a9d215 100644
--- a/x-element.js
+++ b/x-element.js
@@ -852,7 +852,13 @@ export default class XElement extends HTMLElement {
       return value;
     } else {
       // Coerce type as needed.
-      return property.type(value);
+      switch (property.type) {
+        case Number:
+          // Don't try and coerce something like "Number('') >> 0".
+          return value.trim() ? property.type(value) : Number.NaN;
+        default:
+          return property.type(value);
+      }
     }
   }