Lit-element: Extending does some "magic" for properties which reduces control

[daKmoR]

Description

Why do we have this magic of "auto" merging static get properties() while extending? Why not use a simple ...super.properties?

Doing this "magic" brings the following downsides:

1) When applying mixins that each have the same property it will execute requestUpdate for each definition.
Use-case would be Mixins that work independently but can also be combined.

const MixinA = superclass => class MixinA extends superclass {
  static get properties() { return { name: { type: String }, }; }
}

const MixinB = superclass => class MixinB extends superclass {
  static get properties() { return { name: { type: String }, }; }
}

class CustomGreeting extends MixinA(LitElement) {
// =>will execute 1 time

class CustomGreeting extends MixinA(MixinB(LitElement)) { 
// =>will execute 2 times

class CustomGreeting extends MixinA(MixinB(LitElement)) { 
  static get properties() { return { name: { type: String }, }; }
}
// =>will execute 3 times

2) When extending an element and using ...super.properties it will execute requestUpdate for each definition.
Use-case would be changing the property type on the extending element.

class MyBase extends LitElement {
  static get properties() { return { name: { type: String }, }; }
}

class ExtendGreeting extends MyBase {
  static get properties() { return { 
      ...super.properties,
      name: { type: Number},
};}

Live Demo

https://stackblitz.com/edit/lit-element-request-update-bug?file=index.html

Expected Results

One call to requestUpdate for changing properties.

Actual Results

Multiple calls to requestUpdate for each static get properties with the same name in the "extending tree".

Browsers Affected

• [x] Chrome

Versions

• lit-element: v2.0.0-rc.2

[sorvell]

Putting a property in into static get properties does two things: (1) it makes an accessor for the property (unless noAccessor is set to true) and (2) it stores the given metadata for the property, controlling things like whether or not the property should reflect to an attribute.

The reason we merge the properties metadata from superclasses under the hood is for consistency. Since the class inherits form the superclass, it already has the accessor for a property that was in the superclass' properties. If we did not merge the metadata, the subclass would not have the property metadata and this could lead to unexpected behavior (e.g. why did this property stop reflecting but still trigger an update?).

It is true that by default a property that also exists in a superclass creates an accessor that wraps the superclass setter and this can cause requestUpdate to be called multiple times. This is done to make auto-generated accessors compose naturally with superclass accessors, whether they be user-defined or generated. However, requestUpdate is an extremely cheap call since all it does is request an update if one is not already pending. Further, this can be customized by putting noAccessor on the subclass property.

Finally, there's no reason you cannot use object spread like this to customize the metadata for a given property if desired.

[tlouisse]

So if I understand correctly, wrapping of the superclass setter should happen only when setters and getters are defined on the prototype. If you look at this example:

class Level1 extends LitElement {
  static get properties() { 
    return { 
      name: { type: String },
      foo: { type: String }, 
    }; 
  }
}

class Level2 extends Level1 { 
  static get properties() { 
    return { 
      name: { type: String }, 
    }; 
  }
  // these setters should be wrapped, so they automatically trigger requestUpdate
  set foo(val) {
    this.__foo = val;
  }   
  get foo() {
    return this._foo;
  }
}

The foo setters and getters were put on the prototype by the developer, so they should be wrapped by accessors. This works correctly with this code:

const superDesc = descriptorFromPrototype(name, this.prototype);

(https://github.com/Polymer/lit-element/blob/master/src/lib/updating-element.ts#L292)

I think the problem is that property accessors that were never put on the prototype (only on the constructor, like name in the above example), also get involved in the wrapping process.

This is because, in the _finalize method, this happens:

const superCtor = Object.getPrototypeOf(this);
if (typeof superCtor._finalize === 'function') {
  superCtor._finalize();
}

(https://github.com/Polymer/lit-element/blob/master/src/lib/updating-element.ts#L335)

After this, the createProperty method is called. This means that, when the Level2.constructor.createProperty is called, Level1.constructor.createProperty already has been executed and created accessors on the prototype for name.
In Level2.constructor.createProperty, it will thus wrap the setters and getters created in Level1.constructor.createProperty.

This could probably be prevented by doing something like:

this.__superDesc = descriptorFromPrototype(name, this.prototype);

before looping over the parent constructor (L335). This then could be referenced in the createProperty method.

We (I'm a colleague of daKmoR :)) need this, because we are hooking into requestUpdate in order to get notified about synchronous updates (this in turn we need for synchronous observers for non render related updates).

Please see https://stackblitz.com/edit/lit-element-request-update-bug-djgbgg?file=level-2.js for an example of this (I would expect name to be logged exactly once when the button is pressed).

[tlouisse]

This would be a proposal to fix the issue: https://stackblitz.com/edit/lit-element-request-update-bug-cvhw7u?file=level-2-with-fix.js

In the console, you can see that requestUpdate is only called once when the fix is applied.

I needed to copy over a lot of code from UpdatingElement, so please search for [ fixing part ] for the relevant code changes :)

[sorvell]

We're actually considering removing the property accessor wrapping entirely. Instead, if user defined accessors are used, the property metadata should include noAccessor: true.

This would address the issue here; however, we're not considering this change specifically because of this issue. Upon further reflection, we think it's a bit too fancy and makes any future optimization of accessor generation much harder.

[tlouisse]

Thanks for your answer. It indeed feels good from the user perspective to be able to completely control getters and setters (and optionally call requestUpdate).
I tried the code in your pull request and this also does the job for us (our tests are passing again 👍).