Support ES6 getters and setters for data binding #4815
Conversation
…dden by PropertyEffects to be synchronous, we do not need to be concerned about a getter value changing between the time when a notification is triggered and processed.
|
|
||
| // For any predefined getters without setters, add a read only effect | ||
| // so that change notifications don't attempt to update the data cache | ||
| if (this.ACCESSOR_TYPE && this.__dataHasAccessor[prop] === this.ACCESSOR_TYPE.PRE_DEFINED) { |
There was a problem hiding this comment.
Why is this code here instead of in _createPropertyAccessor, any particular reason?
There was a problem hiding this comment.
My thinking was since this was adding a property effect - it shouldn't be in property accessors. I was trying to consider the cases where PropertyAccessors was mixed in without mixing in PropertyEffects.
test/unit/property-effects.html
Outdated
| let originalProp2TwoWay = el.$.prop2TwoWay.value; | ||
| assert.equal(originalProp2TwoWay.indexOf('prop2'), 0); | ||
|
|
||
| el.prop1_ = 'prop1changed'; |
There was a problem hiding this comment.
I would also like to see a test that sets both prop1 and prop2 and asserts that el.$.addProps is set correctly.
There was a problem hiding this comment.
Oh and initially I missed the fact that this is prop1_. Maybe rename this variable to make it clearer that this is the internal property cache, to make it more distinguishable. Something like _internal_prop1?
There was a problem hiding this comment.
prop2 has a getter but no setter. It's read only. Do you want me to add a 3rd property to test that?
I initially had that but removed it. Since PropertyEffects overrides PropertyAccessor to make _propertiesChanged synchronous, I removed the 3rd property.
…ery clear that we are changing the internal version and bypassing automatic change notification.
|
Not sure I understand how bindings & observers work for getters without setters. Are these only updated once, when the element is initialized? |
|
@arthurevans Getters without setters would be only updated once - unless you manually called Also I have a theory that using |
|
See comments posted on the related issue for some general points. I think it would probably make more sense if the existing setter became a property effect. This way it would run, for example, when I also think we'd want to make sure that any relevant code that de-references from Finally, there is the question of how this interplays with the rest of the system, specifically |
|
@sorvell I'll try to spend some on this next week. I'd really like to figure out some solution here. This very much feels like a #UseThePlatform thing. In addition, the sheer amount of property reflection needed with ClosureCompiler is extremely tedious - anything to alleviate that would make a big difference on internal Google projects. |
|
I recall an earlier discussion we had about this PR. We are disinclined to add support for this in the data binding system, as making the data sync properly is computationally intensive. Instead, with lit-element this should work out of the box where you have more control on when to invoke the getter/setter. Therefore I am closing this PR. |
Fixes #4595
Adds support for ES6 getters and setters. Getters are used as-is. Setters are wrapped to enable change notification. Getters without a corresponding setter are treated as a read-only property.