Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes property options to support converter #369

Merged
merged 15 commits into from
Dec 17, 2018
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
Unreleased section, uncommenting the header as necessary.
-->

<!-- ### Changed -->
### Changed
* [Breaking] Changes property options to add `converter`. This option works the same as the previous `type` option except that the `converter` methods now also get `type` as the second argument. This effectively changes `type` to be a hint for the `converter`. A default `converter` is used if none is provided and it now supports `Boolean`, `String`, `Number`, `Object`, and `Array`. In addition, numbers and strings now become null if their reflected attribute is removed. ([#264](https://github.com/Polymer/lit-element/issues/264)).

<!-- ### Added -->
<!-- ### Removed -->
<!-- ### Fixed -->
Expand Down
9 changes: 6 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,14 @@ for additional information on how to create templates for lit-element.
If the value is `false`, the property is not added to the static `observedAttributes` getter.
If `true` or absent, the lowercased property name is observed (e.g. `fooBar` becomes `foobar`).
If a string, the string value is observed (e.g `attribute: 'foo-bar'`).
* `type`: Indicates how to serialize and deserialize the attribute to/from a property.
* `converter`: Indicates how to serialize and deserialize the attribute to/from a property.
The value can be a function used for both serialization and deserialization, or it can
be an object with individual functions via the optional keys, `fromAttribute` and `toAttribute`.
`type` defaults to the `String` constructor, and so does the `toAttribute` and `fromAttribute`
keys.
A default `converter` is used if none is provided; it supports
`Boolean`, `String`, `Number`, `Object`, and `Array`.
* `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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to converter section.

* `reflect`: Indicates whether the property should reflect to its associated
attribute (as determined by the attribute option).
If `true`, when the property is set, the attribute which name is determined
Expand Down
2 changes: 1 addition & 1 deletion demo/lit-element.html
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
foo: {},
bar: {},
whales: {type: Number},
fooBar: {type: {fromAttribute: parseInt, toAttribute: (value) => value + '-attr'}, reflect: true}
fooBar: {converter: {fromAttribute: parseInt, toAttribute: (value) => value + '-attr'}, reflect: true}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/demo/ts-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ class TSElement extends LitElement {

@property() message = 'Hi';

@property({attribute : 'more-info', type: (value: string) => `[${value}]`})
@property(
{attribute : 'more-info', converter: (value: string) => `[${value}]`})
extra = '';

render() {
Expand Down
4 changes: 2 additions & 2 deletions src/lib/decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ export const customElement = (tagName: string) =>
* corresponding attribute value. A `PropertyDeclaration` may optionally be
* supplied to configure property features.
*/
export const property = (options?: PropertyDeclaration) => (proto: Object,
name: PropertyKey) => {
export const property = (options?: PropertyDeclaration) => (
proto: Object, name: PropertyKey) => {
(proto.constructor as typeof UpdatingElement).createProperty(name, options);
};

Expand Down
123 changes: 74 additions & 49 deletions src/lib/updating-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* value.
*/
fromAttribute?(value: string): T;
fromAttribute?(value: string, type?: TypeHint): Type;

/**
* Serializing function called to convert a property value to an attribute
* value.
*/
toAttribute?(value: T): string|null;
toAttribute?(value: Type, type?: TypeHint): string|null;
}

type AttributeType<T = any> = AttributeSerializer<T>|((value: string) => T);
type AttributeConverter<Type = any, TypeHint = any> =
ComplexAttributeConverter<Type>|((value: string, type?: TypeHint) => Type);

/**
* Defines options for a property accessor.
*/
export interface PropertyDeclaration<T = any> {
export interface PropertyDeclaration<Type = any, TypeHint = any> {

/**
* Indicates how and whether the property becomes an observed attribute.
Expand All @@ -46,6 +47,13 @@ export interface PropertyDeclaration<T = any> {
*/
attribute?: boolean|string;

/**
* 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.
*/
type?: TypeHint;

/**
* Indicates how to serialize and deserialize the attribute to/from a
* property. If this value is a function, it is used to deserialize the
Expand All @@ -54,16 +62,17 @@ export interface PropertyDeclaration<T = any> {
* deserialize function and `toAttribute` is a serialize function used to set
* the property to an attribute. If no `toAttribute` function is provided and
* `reflect` is set to `true`, the property value is set directly to the
* attribute.
* attribute. A default `converter` is used if none is provided; it supports
* `Boolean`, `String`, `Number`, `Object`, and `Array`.
*/
type?: AttributeType<T>;
converter?: AttributeConverter<Type, TypeHint>;

/**
* Indicates if the property should reflect to an attribute.
* If `true`, when the property is set, the attribute is set using the
* attribute name determined according to the rules for the `attribute`
* property option and the value of the property serialized using the rules
* from the `type` property option.
* from the `converter` property option.
*/
reflect?: boolean;

Expand All @@ -72,7 +81,7 @@ export interface PropertyDeclaration<T = any> {
* it is set. The function should take the `newValue` and `oldValue` and
* return `true` if an update should be requested.
*/
hasChanged?(value: T, oldValue: T): boolean;
hasChanged?(value: Type, oldValue: Type): boolean;
}

/**
Expand All @@ -90,9 +99,33 @@ type AttributeMap = Map<string, PropertyKey>;

export type PropertyValues = Map<PropertyKey, unknown>;

// serializer/deserializers for boolean attribute
const fromBooleanAttribute = (value: string) => value !== null;
const toBooleanAttribute = (value: string) => value ? '' : null;
export const defaultConverter: ComplexAttributeConverter = {

toAttribute(value: any, type?: any) {
switch (type) {
case Boolean:
return value ? '' : null;
case Object:
case Array:
return JSON.stringify(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this #375 to ensure this is well documented.

}
return value;
},

fromAttribute(value: any, type?: any) {
switch (type) {
case Boolean:
return value !== null;
case Number:
return value === null ? null : Number(value);
case Object:
case Array:
return JSON.parse(value);
}
return value;
}

};

export interface HasChanged {
(value: unknown, old: unknown): boolean;
Expand All @@ -110,6 +143,7 @@ export const notEqual: HasChanged = (value: unknown, old: unknown): boolean => {
const defaultPropertyDeclaration: PropertyDeclaration = {
attribute : true,
type : String,
converter : defaultConverter,
reflect : false,
hasChanged : notEqual
};
Expand Down Expand Up @@ -260,21 +294,16 @@ export abstract class UpdatingElement extends HTMLElement {

/**
* Returns the property value for the given attribute value.
* Called via the `attributeChangedCallback` and uses the property's `type`
* or `type.fromAttribute` property option.
* Called via the `attributeChangedCallback` and uses the property's
* `converter` or `converter.fromAttribute` property option.
*/
private static _propertyValueFromAttribute(value: string,
options?: PropertyDeclaration) {
const type = options && options.type;
if (type === undefined) {
return value;
}
// Note: special case `Boolean` so users can use it as a `type`.
const converter = options && options.converter || defaultConverter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const fromAttribute =
type === Boolean
? fromBooleanAttribute
: (typeof type === 'function' ? type : type.fromAttribute);
return fromAttribute ? fromAttribute(value) : value;
(typeof converter === 'function' ? converter : converter.fromAttribute);
return fromAttribute ? fromAttribute(value, type) : value;
}

/**
Expand All @@ -289,14 +318,12 @@ export abstract class UpdatingElement extends HTMLElement {
if (options === undefined || options.reflect === undefined) {
return;
}
// Note: special case `Boolean` so users can use it as a `type`.
const type = options && options.type;
const converter = options && options.converter;
const toAttribute =
options.type === Boolean
? toBooleanAttribute
: (options.type &&
(options.type as AttributeSerializer).toAttribute ||
String);
return toAttribute(value);
converter && (converter as ComplexAttributeConverter).toAttribute ||
defaultConverter.toAttribute;
return toAttribute!(value, type);
}

private _updateState: UpdateState = 0;
Expand Down Expand Up @@ -416,27 +443,25 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and added test.

const attr = ctor._attributeNameForProperty(name, options);
if (attr !== undefined) {
// Track if the property is being reflected to avoid
// setting the property again via `attributeChangedCallback`. Note:
// 1. this takes advantage of the fact that the callback is synchronous.
// 2. will behave incorrectly if multiple attributes are in the reaction
// stack at time of calling. However, since we process attributes
// in `update` this should not be possible (or an extreme corner case
// that we'd like to discover).
// mark state reflecting
this._updateState = this._updateState | STATE_IS_REFLECTING;
if (attrValue === null) {
this.removeAttribute(attr);
} else {
this.setAttribute(attr, attrValue);
}
// mark state not reflecting
this._updateState = this._updateState & ~STATE_IS_REFLECTING;
const attr = ctor._attributeNameForProperty(name, options);
if (attr !== undefined) {
const attrValue = ctor._propertyValueToAttribute(value, options);
// Track if the property is being reflected to avoid
// setting the property again via `attributeChangedCallback`. Note:
// 1. this takes advantage of the fact that the callback is synchronous.
// 2. will behave incorrectly if multiple attributes are in the reaction
// stack at time of calling. However, since we process attributes
// in `update` this should not be possible (or an extreme corner case
// that we'd like to discover).
// mark state reflecting
this._updateState = this._updateState | STATE_IS_REFLECTING;
if (attrValue == null) {
this.removeAttribute(attr);
} else {
this.setAttribute(attr, attrValue);
}
// mark state not reflecting
this._updateState = this._updateState & ~STATE_IS_REFLECTING;
}
}

Expand Down
Loading