Lit-element: _requestUpdate does not reflect properly

[erikkroes]

Description

Updating a property through _requestUpdate does not reflect properly. We use this a lot to synchronously update properties, for example with events. For more explanation on why we use this, check #643

Live Demo

You can find a live demo here: https://stackblitz.com/edit/lit-element-foo-ktvexc
But unfortunately, it does not reproduce on stackblitz. It does however if you run this code locally. This might also be an indication of why it fails.

Steps to Reproduce

1. Create a custom element with a property that reflects to an attribute
2. Update the property using _requestUpdate

Expected Results

The attribute of the custom element is updated to reflect the property change.

Actual Results

Nothing changes

Browsers affected

Tested in Firefox, Chrome and Safari. Assuming it's not browser specific.

Versions

Using lit-element v2.2.1

[tlouisse]

This is still a bug for us and I did a little bit more investigation to find its root cause.

Code example indicating the problem

Here is a code example (originally created by @jorenbroekema and @erikkroes) that only uses the api as suggested by LitElement.
See: https://codesandbox.io/s/sync-sync-props-without-fix-crki1
This image illustrates the problem (__note that some-prop should switch from 0 to -1__):

The cause of the problem

The problem occurs when we set attribute 'disabled', not when we set property 'disabled'.

How come?

Inside function '_attributeToProperty', UpdatingElement does this:
this._updateState = this._updateState | STATE_IS_REFLECTING_TO_PROPERTY;
https://github.com/Polymer/lit-element/blob/master/src/lib/updating-element.ts#L568

This will prevent condition !(this._updateState & STATE_IS_REFLECTING_TO_PROPERTY) inside
method _requestUpdate of UpdatingElement to pass.
https://github.com/Polymer/lit-element/blob/master/src/lib/updating-element.ts#L599

The reason for this, if I'm right is to prevent endless cycles like dev sets attr disabled -> UpdatingElement reflects to prop -> prop reflects to attr.

Only, now the 'lock' introduced by disabled attr will prevent someProp to reflect to an attribute....

Code example with a fix

Here, I reset the STATE_IS_REFLECTING_TO_PROPERTY status before the property effect runs:
https://codesandbox.io/s/sync-sync-props-with-fix-uy8d0
(not that this demo is meant to indicate the problem, it is not the proposal for a solution)

This image illustrates the desired situation with a temp fix applied (__note that some-prop now switches from 0 to -1__):

A (possible) solution

In case disabled would not block all props from being reflected to attrs, but instead a map of 'non reflecting' props would be maintained, it would be possible to prevent disabled from reflection, but still allow someProp to reflect.

@sorvell Any thoughts on this?

[sorvell]

Sorry for the delay on this. As I understand it, you want to compute one property (someProp or tabIndex) based on another disabled. The best way to do that is to override update and compute the property there.

update(changedProps) {
  // Compute the appropriate state of `someProp`.
  if (this.disabled) {
    this.someProp = -1;
  } else {
    this.someProp = 0;
  }
  super.update(changedProps);
}

Note: this will be possible in render in the next release including https://github.com/Polymer/lit-element/pull/555).

Updated example: https://stackblitz.com/edit/lit-element-foo-rxxnkw?file=src%2Findex.js

You should not override _requestUpdate since it's private, but what you did is similar to computing a dependent property in a manually implemented property setter. We don't recommend that and instead recommend computing properties in update (before calling super.update).

In this case what is happening is that for efficiency LitElement short-circuits reflection such that when you set an attribute that reflects to a property, no attribute should re-reflected and when you set a property that reflects to an attribute, no property should be re-reflected. Currently, this is just a flag the element checks and is not tracked per property. Here, the disabled attribute is set and in response the property is set. While the property setter runs the element is in "do not reflect to attribute" mode and therefore the someProp setter that runs does not reflect to an attribute.

Doing the computation in the update cycle avoids this interaction and works as expected.

Hope that helps.

[tlouisse]

Thanks for the response!
It does help us for now, although we kind of have the best practice to put super calls always on top for predictability. In case we have a chain of parent mixins we rely on, order of property effects becomes important, so we can still not use it for all our synchronous prop effects.

Ideal would be to have something like a public syncUpdate(name, oldValue) that allows the prototype hierarchy to determine the order of execution