getter/setter pairs should be available in extensions

[Westbrook]

Description

When extending a class with getter/setter controlled property management, these effects should persist in child extensions.

Live Demo

https://glitch.com/edit/#!/nervous-columnist?path=index.html:24:0

If you open you console, you should see a log of parent and one of child but the second log is missing.

Steps to Reproduce

1. Create an element that includes the mood attribute to property relationship.
2. Use a getter/setter pair to extend the response to changing the value of mood.
3. Create a new element with a class that extends the class of the previous element.
4. Attempt to use the getter/setter functionality....

Expected Results

Getter/Setter works.

Actual Results

Getter/Setter is ignored... 😢

Browsers Affected

• Chrome
• Firefox
• Edge
• Safari 11
• Safari 10
• IE 11

Versions

• "@polymer/lit-element": "^0.6.0",
• "polymer-cli": "next",
• "@webcomponents/webcomponentsjs": "latest"

[kevinpschaaf]

This looks at least somewhat unintentional, and can be fixed.

1. When finalizing a class, this code should only handle an ownProperty of this.properties; right now a subclass that defines no properties will re-create accessors on the subclass for accessors that may already exist on the super class, which is unintentional. This is what is causing the user accessors on the child to be shadowed by generated accessors in the repro.

2. The reason the parent user accessors are not shadowed by generated accessors is because we do have a check for hasOwnProperty before generating accessors; however this only helps prevent shadowing user accessors on the same prototype. One could conceive that a user might extend a class with user accessors, and then define a property in properties or via the property decorator on the subclass for some reason; in that case the generated accessor would shadow the user accessor, which is likely not what the user wants either. This could be improved by changing this line from this.hasOwnProperty(p) to p in this, and adopt a rule that generated accessors will never be created over the top of existing accessors.

[silenceisgolden]

I took a brief look at this a bit ago and ran into the problem that @kevinpschaaf describes at the end of (2). The brief change I explored is documented at https://github.com/silenceisgolden/lit-element/commit/eb9324ed302e5332a75eecb044be11ec4fecc11e.

I think if the rule is adopted that generated accessors will never overwrite existing accessors this might be a non-obvious limitation. The fix I worked on broke other tests that are expecting the accessor to be overwritten, and that does carry some logic. Separate from the context of custom elements, you can extend getters and setters if you wire up the super calls correct, as shown at https://glitch.com/edit/#!/surf-rise?path=script.js:44:52. I would be curious to know your thoughts on if people understand getter/setter extension in plain ole' JS that they might expect @property() extendedProp ... to work as an extended prop rather than blocking the parent accessor or seeing the parent accessor and refusing to set a child one.

Thanks for prioritizing this for 1.0!

[kevinpschaaf]

@silenceisgolden Yeah definitely good insight, thanks for bringing it up. We've definitely considered all of this in the past, and it gets fairly complicated.

As you say, @property could potentially always create an own-property accessor that supers to any superclass accessor. However, it would be ambiguous as to whether the super accessor itself was a generated accessor or a user accessor, so it would have to do the safe thing of always enqueueing an invalidation itself, and then doing the super assignment, which might re-do the invalidation work -- not fatal, but slightly sub-optimal performance-wise since the setter may be doing unnecessary double work. It also leaves ambiguity as to who owns the storage for the getter side. To be safe, the generated setter would always need to set the value to the super, then read the value from the super getter (since it may or may not be the same value received in the setter, depending on what the super accessor does) -- but only if one exists. I don't think any of this is impossible, it just adds complexity and overhead to maintain the purity of JS semantics, for marginal benefit.

In addition, we've also considered supporting "supering" to the generated accessors in the past. Conceptually it seems reasonable that a user might want to install a setter on a class and then decide to run the generated accessor or not (in the same way one might want to do from a subclass via super), however since the generated accessors need to be installed on the same prototype as the user's accessor, a super call wouldn't work. So then it's back to a custom rule about how existing accessors are wrapped or not and the semantics of that, and it starts to feel leaky again.

We'll try to get a few more opinions and try to come to a resolution on this. Thanks.

[sorvell]

This is a bit tricky. We toyed with the idea of automatically wrapping a user's getter/setter previously, but decided against it. The problem is that it's not really possible for the library to know if wrapping a user's getter/setter is actually what's desirable.

Instead, I think we want to keep the accessor generation system as simple as we can and encourage users to create their own accessors when this type of more complex composition is needed.

For (2) above, the issue is that we do want a subclass to be able to override the property options (e.g. type) for a property and for efficiency we store this in the setter's closure. So in order to maintain this optimization, we do want to allow generated accessors to be overwritten. This info is available so it should be possible.

[silenceisgolden]

Thanks for the responses, I appreciate the context.

One more thing I'll leave here is the question of if it would even make sense to add a property to PropertyDeclaration of extends?: boolean = false. This would make getter/setter inheritance an opt in.

Cheers

[sorvell]

For now, we landed on simply never stomping on a user's accessor. I think we can potentially revisit the idea of adding a property option for extends in the future based on demand, but it's unclear right now if it would add any value.

We've removed the optimization of storing the property options in the closure for the generated setter. This ensures any subclass can override property options easily.

The properties system really does 2 very different things:

1. generates an accessor that makes the property participate in the update system. Here we only want to ensure the accessor exists. If the user has created their own, we don't touch it and assume the user has called requestUpdate(name, oldValue)
2. provides metadata for the property that's used in the update system. Here we want to use the "youngest" (subclass') options defined for the property.

[blikblum]

Just like to point one caveat that is not clear in the docs:

• When creating property getter and setter and calling requestUpdate in the setter, is necessary to also define the property with properties or property decorator, otherwise will not work when passing value through property.

The example below will not work:

class MyElement extends LitElement {     
      
      get mood() { return this._mood; }
      
      set mood(mood) { this._mood = mood;  this.requestUpdate(); }
     
      render() {
        return html`<style> .mood { color: green; } </style>
          Web Components are <span class="mood">${this.mood}</span>!`;
      }      
    }

  customElements.define('my-element', MyElement);

  //later
  let mood = 'great' // -> if var mood is changed and template executed again no update occurs
  html`<my-element .mood=${mood}></my-element>`

This falls because lit-html sets the mood property in the own instance before the custom element is upgraded shadowing the setter

[kevinpschaaf]

We iterated on the solution here a little bit, and actually ended up landing on something I straw-manned in my comment above:

• When a property is declared in properties or via the property decorator, we will always generate an accessor on the current class that enqueues an update via requestUpdate
• However, if an accessor already existed on the class prototype chain, that accessor will be effectively super'ed to for both set and get (while still ensuring an update is enqueued); this is achieved by capturing the existing accessor and calling them from the generated accessor

We achieved this by adding a couple fast-paths to ensure extra work required to create the accessor and handle potentially multiple calls to requestUpdate due to chained super calls are minimized.

This should handle composition better, and also removes the need for users to call requestUpdate in custom setters if they have included the property in properties. That is, the meaning of properties becomes clear: LitElement will always generate an accessor that calls requestUpdate if in properties, and if an existing accessor existed, it will defer to those.

[kevinpschaaf]

@blikblum Acknowledged, we can improve the docs here to ensure users understand that inclusion in properties is required to get LitElement's value-added features for properties (e.g. adding them to observedAttributes, pre-upgrade instance property handling, etc.)

cc/@katejeffreys

[silenceisgolden]

We iterated on the solution here a little bit, and actually ended up landing on something I straw-manned in my comment above:

• When a property is declared in properties or via the property decorator, we will always generate an accessor on the current class that enqueues an update via requestUpdate
• However, if an accessor already existed on the class prototype chain, that accessor will be effectively super'ed to for both set and get (while still ensuring an update is enqueued); this is achieved by capturing the existing accessor and calling them from the generated accessor

We achieved this by adding a couple fast-paths to ensure extra work required to create the accessor and handle potentially multiple calls to requestUpdate due to chained super calls are minimized.

This should handle composition better, and also removes the need for users to call requestUpdate in custom setters if they have included the property in properties. That is, the meaning of properties becomes clear: LitElement will always generate an accessor that calls requestUpdate if in properties, and if an existing accessor existed, it will defer to those.

This sounds excellent but for 2 questions:

If a superclass has a static get properties or equivalent block, will the child class need to redeclare the properties property getter or equivalent to get these optimizations?

Will a user still need to include a requestUpdate call if they change a different property in the setter? Let's say:

...
static get properties() {
  return {
    ariaDisabled: ...,
    disabled: ...
  };
}
...

set disabled(b) {
  const oldAriaDisabled = this._ariaDisabled;

  this._disabled = b;
  this._ariaDisabled = b;

  this.requestUpdate('ariaDisabled', oldAriaDisabled); // will this be necessary?
}
...

Many thanks

[kevinpschaaf]

@silenceisgolden Right, so the new rule will be, if the property is listed in properties, it will always have a generated accessor that requests an update. requestUpdate will still dirty check against the previous getter's value, so it's important that the getter return a changed state when appropriate.

In the example above, since disabled is in properties, there will be a generated accessor that requests an update and puts disabled in changeProperties. So the only reason to call requestUpdate('ariaDisabled', oldAriaDisabled) is so that ariaDisabled also shows up in changedProperties (and so that it's e.g. reflect effect runs, if necessary).

That said, it's unclear if that was a contrived example or not, but if your actual goal is to set aria-disabled attribute based on the disabled property state, we'd probably recommend not making an reflecting ariaDisabled property (and exposing that as API, since the aria state should ideally be an implementation detail that goes away with AOM), and instead just implement updated like this:

updated(changedProperties) {
  if (changedProperties.has('disabled')) {
    if (this.disabled) {
      this.setAttribute('aria-disabled', 'true');
    } else {
      this.removeAttribute('aria-disabled');
    }
  }
}

[kevinpschaaf]

Note that while the originally reported issue (LitElement incorrectly re-creating accessor over top of superclass's custom accessor despite the subclass not declaring the property), we have since reverted the change in #359 that added "accessor wrapping." The rationale is that we want to align the property metaprogramming as closely as possible to the new class field semantics (especially since we expect @property decorator usage to become more and more common). Since class fields have "define property" semantics (meaning class fields always create a new property), declaring a property should do the same (i.e., always generate an accessor, regardless of what was existing before). This will also make code generation for decorators (an idea we're considering) more straightforward since it keeps information needed to code generate local to the file, rather than requiring introspection of the entire prototype chain.

That said, we've retained the noAccessor flag to opt-out of accessor creation, and noAccessor is implicitly assumed for cases where an own accessor exists (to prevent stomping on a user accessor). Custom accessors are supported, they will just be required to manually call requestUpdate in order to trigger an update.

See #454 for more details.