lit / lit-element

LEGACY REPO. This repository is for maintenance of the legacy LitElement library. The LitElement base class is now part of the Lit library, which is developed in the lit monorepo.
https://lit-element.polymer-project.org
BSD 3-Clause "New" or "Revised" License
4.49k stars 318 forks source link

Advice on Validating/Updating Properties #1110

Closed jvoccia closed 3 years ago

jvoccia commented 3 years ago

When building elements that have properties, we sometimes want to limit the property values. For instance, we might have a property called - someValue that might be only valid with the values "" or "1". If someone passes in "abc", we might want to set it to empty, as opposed to throwing an exception.

set someValue  (value) {
     let oldValue = this.someValue ;
     if (value !== '' || value !== '1') value = '';
     this._someValue  = value;
     this.requestUpdate('someValue', oldValue);
}

get someValue () {
    return this._someValue ;
}

Code like the above works generally, but fails with reflectAttribute. Consider the case where someValue is '' and someone calls myElem.setAttribute('somevalue', 'abc'); In this case oldValue and new value match, but now reflectAttribute does not occur.

With problems such as this, it makes me think we might be doing this in a less then optimal way. So, I am wondering is there a better place to be doing this work that requires less boilerplate and works well with reflectAttribute true, or if this is the correct method. If it is the correct method, is there a good way of handling reflectAttribute to make the updated property reflect back? With code like the above, the attribute will remain with the invalid value, but the property will contain the correct valid value.

sorvell commented 3 years ago

This is a complex issue as highlighted by the problem you're having with attributes. In general, changing a property or attribute from the value the user set can violate expectations. To avoid this, it's often better to have an internal validated value that you use inside the element and leave the input as is. In particular, if a user sets an attribute and it immediate changes to something else, they might be really confused. That said, we do want to support a variety of opinions on this.

In this case, writing a setter as you've done handles the property side, but the attribute side is problematic as noted. Property/attribute reflection in general is a bit tricky and we have a rule in LitElement that prevents reflection from bouncing from the property to the attribute and back and visa versa. In this case, when you call myElem.setAttribute('somevalue', 'abc'), the property is set, but LitElement specifically knows that it's set in response to the attribute changing and does not change the attribute again. Without this check, it's very easy to have an infinite loop for any value that does not deserialize to strict equality.

You can customize this behavior in your setter. Here's an example.

It uses static getPropertyDescriptor to support a validator property option which is used to create the valid value. If the valid value is different from the set value and it's a reflecting attribute, the attribute is specifically set. The property will subsequently be set after the attribute changes again.

Hope that helps. Feel free to reopen the issue if there are additional questions.

jvoccia commented 3 years ago

Thank you. This response is very helpful. It is good to know what you all are thinking in this situation. It helps us understand what we may want to rethink around this topic. Maintaining an internal state separately seems like an interesting way of handling some of these situations and we did not know about how we could use the getPropertyDescriptor API to address some of the attribute syncing complexities. These both may be options for us depending on the particular property representation.