Changes property options to support `converter` #369

sorvell · 2018-12-13T23:50:31Z

Fixes #264

Changes type to be only a hint to the converter option which has the previous type functionality, an object with toAttribute and fromAttribute or just a function which is fromAttribute. In addition to the value these functions now also get the property's type.

Also provides a default converter that supports Boolean, String, Number, Object, and Array out of the box.

In addition, numbers and strings now become null if their reflected attribute is removed.

Fixes #264 Changes `type` to be only a hint to the `converter` option which has the previous `type` functionality, an object with `toAttribute` and `fromAttribute` or just a function which is `fromAttribute`. In addition to the `value` these functions now also get the property's `type`. Also provides a default converter that supports `Boolean`, `String`, `Number`, `Object`, and `Array` out of the box. In addition, numbers and strings now become `null` if their reflected attribute is removed.

kevinpschaaf · 2018-12-14T01:57:47Z

README.md

-    keys.
+    * `type`: Indicates the type of the property. This is used only as a hint for the
+    `converter` to determine how to serialize and deserialize the attribute
+    to/from a property.


Add:

Default converters are provided for String, Number, ...

Added to converter section.

kevinpschaaf · 2018-12-14T02:30:14Z

src/lib/updating-element.ts

+      return value ? '' : null;
+    case Object:
+    case Array:
+      return JSON.stringify(value);


Based on experience with this being a footgun for users, consider anything better?

Added this #375 to ensure this is well documented.

kevinpschaaf · 2018-12-14T02:33:33Z

src/lib/updating-element.ts

-      return value;
-    }
-    // Note: special case `Boolean` so users can use it as a `type`.
+    const converter = options && options.converter || defaultConverter;


Shouldn't need a null check since there's a defaultPropertyDeclaration?

This is working around https://github.com/webcomponents/custom-elements/issues/167.

Remove superfluous null checks

Previously, when an attribute changed as a result of a reflecting property changing, the property was prevented from mutating again as can happen when a custom `converter` is used. Now, the oppose is also true. When a property changes as a result of an attribute changing, the attribute is prevented from mutating again This change helps ensure that when a user calls `setAttribute`, a `converter.toAttribute` does not cause the attribute to immediately mutate. This is unexpected behavior and this change discourages it.

vicb · 2018-12-15T00:21:33Z

README.md

+    `converter` to determine how to serialize and deserialize the attribute
+    to/from a property. Note, when a property changes and the converter is used
+    to update the attribute, the property is never updated again as a result of
+    the attribute changing, and visa versa.


Suggested change

the attribute changing, and visa versa.

the attribute changing, and vice versa.

vicb · 2018-12-15T00:23:12Z

src/lib/updating-element.ts

@@ -15,27 +15,28 @@
 /**
 * Converts property values to and from attribute values.
 */
-export interface AttributeSerializer<T = any> {
+export interface ComplexAttributeConverter<Type = any, TypeHint = any> {

  /**
   * Deserializing function called to convert an attribute value to a property


IMO Deserializing and Deserializing is a confusing terminology.
Is it still needed after this change ?

vicb · 2018-12-15T00:32:59Z

README.md

+    `converter` to determine how to serialize and deserialize the attribute
+    to/from a property. Note, when a property changes and the converter is used
+    to update the attribute, the property is never updated again as a result of
+    the attribute changing, and visa versa.


docs/_guide/properties.md should also be updated, see #370

Added this #375 to ensure this is well documented.

kevinpschaaf · 2018-12-15T00:40:19Z

src/lib/updating-element.ts

@@ -416,41 +446,44 @@ export abstract class UpdatingElement extends HTMLElement {
      name: PropertyKey, value: unknown,
      options: PropertyDeclaration = defaultPropertyDeclaration) {
    const ctor = (this.constructor as typeof UpdatingElement);
-    const attrValue = ctor._propertyValueToAttribute(value, options);
-    if (attrValue !== undefined) {


Was this removed for a reason? Apparently undefined was being used as a sentinel return value from toAttribute meaning "no change". Should we keep this?

Done and added test.

kevinpschaaf · 2018-12-15T00:42:21Z

src/lib/updating-element.ts

-    if (options.reflect === true) {
+    // Add to reflecting properties set if `reflect` is true and the property
+    // is not reflecting from the attribute
+    if (options.reflect === true &&  !(this._updateState & STATE_IS_REFLECTING)) {


It might be nice to separate STATE_IS_REFLECTING_TO_ATTRIBUTE and STATE_IS_REFLECTING_TO_PROPERTY; I think they should be mutually exclusive timing-wise, but the setting/reading the flags are paired to two different use cases.

Ensure Object/Array properties respect `undefined` (no change to attribute) and `null` (remove attribute) values.

kevinpschaaf

LGTM.

Won't merge until Sauce service incident is resolved and tests pass.

carsonpowers · 2021-09-30T20:55:55Z

In addition, numbers and strings now become null if their reflected attribute is removed.

Shouldn't numbers and strings become undefined if they are removed? Changing them to null means that defaults set by the component will now be bypassed:

function func(foo='bar'){
  console.log(foo);
}

func(null); // null
func(undefined); // 'bar'

Steven Orvell added 3 commits December 13, 2018 10:07

Format

c5c261e

Merge branch 'master' into type-converter

bc1caa0

sorvell added this to the 1.0.0 milestone Dec 13, 2018

sorvell requested a review from kevinpschaaf December 13, 2018 23:50

googlebot added the cla: yes label Dec 13, 2018

Steven Orvell added 2 commits December 13, 2018 17:55

Address review feedback

b384bdc

Update CHANGELOG.md

abc945f

kevinpschaaf reviewed Dec 14, 2018

View reviewed changes

Steven Orvell added 2 commits December 13, 2018 18:00

Clarify documentation about default converter

3392e9b

Format

f5f95e1

kevinpschaaf reviewed Dec 14, 2018

View reviewed changes

Fix test to use lowercase attribute names

383510e

This is working around https://github.com/webcomponents/custom-elements/issues/167.

sorvell mentioned this pull request Dec 14, 2018

introduce default type serializers #356

Closed

Steven Orvell added 3 commits December 14, 2018 10:06

Fix test on IE

0ba8f1b

Address review feedback

6408fc5

Remove superfluous null checks

This was referenced Dec 15, 2018

[Docs] Fix property type - function case #370

Closed

[docs] Update properties docs #373

Closed

Format.

f2748f3

vicb reviewed Dec 15, 2018

View reviewed changes

kevinpschaaf requested changes Dec 15, 2018

View reviewed changes

Address review feedback.

168c384

kevinpschaaf mentioned this pull request Dec 15, 2018

[docs] Document changes to property declarations in #369 #375

Closed

Address review feedback

54f060f

Ensure Object/Array properties respect `undefined` (no change to attribute) and `null` (remove attribute) values.

kevinpschaaf approved these changes Dec 15, 2018

View reviewed changes

Merge branch 'master' into type-converter

83f5bdd

kevinpschaaf merged commit 47717a7 into master Dec 17, 2018

kevinpschaaf mentioned this pull request Dec 18, 2018

Removing an attribute for a property of type String sets the property to null #293

Closed

keanulee deleted the type-converter branch December 20, 2018 17:14

cdata mentioned this pull request Jan 28, 2019

UpdatingElement sets a remove attribute as 'null' for string properties google/model-viewer#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes property options to support `converter` #369

Changes property options to support `converter` #369

sorvell commented Dec 13, 2018

kevinpschaaf Dec 14, 2018

sorvell Dec 14, 2018

kevinpschaaf Dec 14, 2018

sorvell Dec 15, 2018

kevinpschaaf Dec 14, 2018

vicb Dec 15, 2018

sorvell Dec 15, 2018

vicb Dec 15, 2018

sorvell Dec 15, 2018

vicb Dec 15, 2018

sorvell Dec 15, 2018

kevinpschaaf Dec 15, 2018

sorvell Dec 15, 2018

kevinpschaaf Dec 15, 2018

sorvell Dec 15, 2018

kevinpschaaf left a comment

carsonpowers commented Sep 30, 2021

	the attribute changing, and visa versa.
	the attribute changing, and vice versa.

Changes property options to support converter #369

Changes property options to support converter #369

Conversation

sorvell commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinpschaaf left a comment

Choose a reason for hiding this comment

carsonpowers commented Sep 30, 2021

Changes property options to support `converter` #369

Changes property options to support `converter` #369