Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secondary property change not reflected when first property set via attribute #965

Closed
6 tasks done
dbatiste opened this issue Apr 17, 2020 · 4 comments
Closed
6 tasks done

Comments

@dbatiste
Copy link

dbatiste commented Apr 17, 2020

Description

When a second property is changed internally due to a change in another property via setting its attribute, the second property is not reflected as expected (given reflect: true). For example, given the following element, where setting myprop in turn changes another property _someprop, the change to _someprop does not get reflected if myprop was changed via setAttribute/removeAttribute:

import { html, LitElement } from 'lit-element/lit-element.js';
class MyElem extends LitElement {
    static get properties() {
        return {
            myprop: { type: Boolean, attribute: true },
            _someprop: { type: Number, reflect: true }
        };
    }
    constructor() {
        super();
        this._myprop = false;
        this._someprop = 0;
    }
    get myprop() {
        return this._myprop;
    }
    set myprop(val) {
        const oldVal = this._myprop;
        if (oldVal !== val) {
            this._myprop = val;
            this._someprop += 1;
            this.requestUpdate('myprop', oldVal);
        }
    }
    render() {
        console.log('render', this.myprop, this._someprop);
        return html`<div></div>`;
    }
}
customElements.define('my-elem', MyElem);

From @arthurevans (Slack thread):

OK, I can reproduce this, and it seems to me like a bug.
The following appears to work around the issue:

this.requestUpdate('myprop', oldVal).then(() => {
    this._someprop += 1;
});

This renders twice whenever you change myprop, so it's not great.

A little more info: this is sort of a corner case. When LitElement sets a property because an attribute has changed, it sets a flag. It checks that flag before reflecting a property to an attribute, to avoid an infinite loop.
In this case, because one attribute change is causing a different property to be set, the second property isn't being reflected, because LitElement doesn't actually check that the property that triggered the requestUpdate is the same as the one doing the reflecting. That's the bug.

As Arthur mentioned, waiting for the first property update to render before changing the second property works around the issue but results in more than one render.

Live Demo

https://stackblitz.com/edit/property-reflect-bug?file=my-element.js

Steps to Reproduce

  1. Create the element as describe above, and set myprop via attribute
  2. Observe that _someprop change is not reflected to it's corresponding attribute as expected

or...

  1. Go to the StackBlitz live example linked above.
  2. Click on the button to update myprop via attribute
  3. Observe that _someprop change is not reflected to it's corresponding attribute as expected

Expected Results

The second property change should be reflected to its corresponding attribute (given reflect: true) without requiring the second iteration of render.

Actual Results

The second property change is not reflected to its corresponding attribute.

Browsers Affected

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

Versions

  • lit-element: v2.3.1
  • webcomponents: v2.4.3
@sorvell
Copy link
Member

sorvell commented May 4, 2020

Sorry for the delay on this. We're looking at addressing the issue here, but we're concerned about potentially degrading performance. We also view this as somewhat of a corner case. In the meantime, you can workaround the issue by computing the secondary property in update as is shown here:

https://stackblitz.com/edit/property-reflect-bug-mrjep4?file=my-element.js

sorvell pushed a commit that referenced this issue May 4, 2020
It is now possible to set a computed reflecting property in another property's setter as long as `requestUpdate` is called before setting the computed property.

Note, this does require a slightly user code modification to address #965 but it addresses the use case without degrading performance by adding per-property tracking (which is the other alternative).
@Leokuma
Copy link

Leokuma commented May 22, 2020

sorvell, thank you for the PR and the workaround. I'm facing the same problem. I didn't think it was so "corner case".

I have an element with 2 reflected attributes/properties: points and maxPoints. Both have custom setters so that:

  1. if you assign points higher than maxPoints, the operation is "rolled back", i.e. points is not changed;
  2. if you assign maxPoints lower than the currently assigned points, points are decreased to maxPoints.

Making those assignments through properties works fine. But making them via attributes doesn't: in the second scenario, 2 attribute changes should be triggered, but only maxPoints is actually updated. In the end, the property points holds the correct value, but the attribute points is not updated.

@gabrielgons
Copy link

gabrielgons commented Jun 14, 2020

We got this problem too.
While there's no fix, we solve it using:
myObject = Object.Assign({}, myObject)

after any changes on 'myObject' occurs.
This should trigger the shouldUpdate method and do the trick for us.

sorvell pushed a commit to lit/lit that referenced this issue Sep 4, 2020
* Streamlines some internal private API
* renamed `_getUpdateComplete` to `getUpdateComplete`
* when a reflecting property is undefined, its attribute is removed (previously it was unchanged)
* dirty check in `attributeChangedCallback` removed (requested by user)
* Fixes lit/lit-element#965
sorvell pushed a commit to lit/lit that referenced this issue Sep 12, 2020
Refactor UpdatingElement
* Streamlines some internal private API
* renamed `_getUpdateComplete` to `getUpdateComplete`
* when a reflecting property is undefined, its attribute is removed (previously it was unchanged)
* dirty check in `attributeChangedCallback` removed (requested by user)
* Fixes lit/lit-element#965
* Additional minor refactoring
* Also fixes an issue where the default fromAttribute function was not run if only a toAttribute function was set.
* Reduces code size for instanceProperties
* Removes `hasFInalized` in favor of returning true from `finalize` if the element needed to finalize.
* Separates decorators into individual modules
* Also separates decorators tests.
* Moved tests around so decorators are only used in tests for those decorators.
sorvell pushed a commit to lit/lit that referenced this issue Sep 12, 2020
Refactor LitElement and UpdatingElement

LitElement
* removes all polyfill code and places in `lit-element-polyfill` which is an in place patch and must be loaded separately (preserves support for styling shadowRoot without adoptedStylesheets).
* Includes caching of CSSResult (adopted from lit/lit-element#952)
* `finalizeStyles` is now the main entry point for gathering element styles and can be overridden to customize. It is called once per class in `finalize`.
* `adoptStyles` is now always called, previously was only called if renderRoot was a shadowRoot.
* [lit-element] Refactor UpdatingElement (#1240)

UpdatingElement
* Streamlines some internal private API
* renamed `_getUpdateComplete` to `getUpdateComplete`
* when a reflecting property is undefined, its attribute is removed (previously it was unchanged)
* dirty check in `attributeChangedCallback` removed (requested by user)
* Fixes lit/lit-element#965
* Additional minor refactoring
* Also fixes an issue where the default fromAttribute function was not run if only a toAttribute function was set.
* Reduces code size for instanceProperties
* Separates decorators into individual modules
* Also separates decorators tests.
* Moved tests around so decorators are only used in tests for those decorators.
@kevinpschaaf kevinpschaaf moved this to 🏥 In Triage in Lit Project Board Sep 8, 2022
@sorvell
Copy link
Member

sorvell commented Sep 8, 2022

Closing as this is fixed in the current Lit version and there are no plans to change the behavior in the previous version of LitElement.

See https://lit.dev/playground/#gist=9a6934613b6204e035a3baff1fa54a3e.

@sorvell sorvell closed this as completed Sep 8, 2022
Repository owner moved this from 🏥 In Triage to ✅ Done in Lit Project Board Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants