Skip to content

Commit

Permalink
Deserialize "" to NaN for Number type.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
theengineear committed Aug 18, 2023
1 parent 42d8ffe commit bf355cb
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 1 deletion.
13 changes: 13 additions & 0 deletions test/test-basic-properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
8 changes: 7 additions & 1 deletion x-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

Expand Down

0 comments on commit bf355cb

Please sign in to comment.