kenchris / lit-element

A base class for creating web components using lit-html
BSD 3-Clause "New" or "Revised" License
81 stars 10 forks source link

How to observe a property change in a child component #22

Closed rogeru closed 6 years ago

rogeru commented 6 years ago

Example app at http://plnkr.co/edit/JCVyjTEWRopqoL8WbowV?p=preview

Parent component changes string property this.fruit which is passed to child component via <my-fruit fruit="${this.fruit}"></my-fruit>. How can the child component observe that change?

The only ways I can see this working are: a) Call invalidate on the child component. E.g. this.shadowRoot.querySelector('my-fruit').invalidate();

b) Use an attribute instead of a property and use observedAttributes and attributeChangedCallback in the child component. E.g. <my-fruit fruit$="${this.fruit}"></my-fruit>.

What am I missing here?

I am build an example app using lit-html-element. Will post it here once I got these last few issues solved.

kenchris commented 6 years ago

Do you know how this works in eg. polymer? I am sure we can fix it but we need to think about how a fix might affect performance.

So you want elements in the light DOM to be able to know when props on the parent change, right?

rogeru commented 6 years ago

No, not sure how it works in Polymer. I'm just trying to understand what the suggested way is to update a property in the child component.

For attributes the attributeChangedCallback can be used when changing the attribute on the child element, but only for strings/booleans.

Is it correct that for properties the setter needs to be called in JS?

<!DOCTYPE html>
<html>
  <head>
    <script type="module">
      import { LitElement, html } from '//unpkg.com/lit-html-element';
      class MyApp extends LitElement {
        constructor() {
          super();
          this.fruit = 'Apple';
        }
        change() {
          this.fruit = this.$('new').value;
          this.$('new').value = '';

          // This will work as it calls the setter, but only if the
          // froot attribute in the HTML below is removed.
          this.shadowRoot.querySelector('my-fruit').froot = this.fruit;

          this.invalidate();
        }
        render() {
          return html`
            <p>Fruit in parent: ${this.fruit}</p>
            <my-fruit froot="${this.fruit}"></my-fruit>
            <input id="new" type="text"/>
            <button on-click="${() => this.change()}">Change</button
          `;
        }
      }
      customElements.define('my-app', MyApp.withProperties());
    </script>
    <script type="module">
      import { LitElement, html } from '//unpkg.com/lit-html-element';
      class MyFruit extends LitElement {
        static get properties() {
          return {
            froot: {
              type: String
            }
          }
        }
        render() {
          return html`
            <p>Fruit in child: ${this.froot}</p>
          `;
        }
      }
      customElements.define('my-fruit', MyFruit.withProperties());
    </script>
  </head>
  <body>
    <my-app></my-app>
  </body>
</html>
kenchris commented 6 years ago

@justinfagnani how does this work in Polymer? I don't think that I have needed this for light dom

kenchris commented 6 years ago

In the MyApp, why don't you define "fruit" as a property. That way, when it is modified it will automatically call invalidate?

kenchris commented 6 years ago

Ok, there definitely seem to be a bug somewhere. I can grep the my-fruit element manually using DevTools and change froot to something else. Doing that and nothing happens. If I call .invalidate() on it, it updates, so somehow invalidate() isn't called!

If I add another my-fruit element at the same level as my-app, and change froot, invalidate is correctly called.

kenchris commented 6 years ago

So the bug has something to do with you defining each element as its own module - something doesn't work with the setters it seems. The following works fine:

    <script type="module">
      import { LitElement, html } from '//unpkg.com/lit-html-element';
      class MyApp extends LitElement {
      static get properties() {
          return {
            fruit: {
              type: String
            }
          }
        }

        constructor() {
          super();
          this.fruit = 'Apple';
        }
        change() {
          this.fruit = this.$('new').value;
          this.$('new').value = '';
        }
        render() {
          return html`
            <p>Fruit in parent: ${this.fruit}</p>
            <my-fruit froot="${this.fruit}"></my-fruit>
            <input id="new" type="text"/>
            <button on-click="${() => this.change()}">Change</button
          `;
        }
      }
      customElements.define('my-app', MyApp.withProperties());

      class MyFruit extends LitElement {

        static get properties() {
          return {
            froot: {
              type: String,
              attrName: "froot"
            }
          }
        }
        render() {
          return html`
            <p>Fruit in child: ${this.froot}</p>
          `;
        }
      }
      customElements.define('my-fruit', MyFruit.withProperties());
    </script>
  </head>
  <body>
    <my-app></my-app>
  </body>
</html>

Interesting bug, maybe you could debug a bit more and open a new issue for this problem?

rogeru commented 6 years ago

It works if the child module is imported first. Looks like when defining as separate modules the order matters. This is not specific to lit-html-element, the same applies to lit-html.

For completeness here is the working lit-html example.

<!DOCTYPE html>
<html>
  <head>
    <script type="module">
      import {html, render} from '//unpkg.com/lit-html/lib/lit-extended';
      class MyFruit extends HTMLElement {
        constructor() {
          super();
          this.attachShadow({mode: 'open'});
        }
        get froot() { return this._froot; }
        set froot(v) { this._froot = v; this.invalidate(); }
        render() {
          return html`
            <p>Fruit in child: ${this.froot}</p>
          `;
        }
        invalidate() {
          if (!this.needsRender) {
            this.needsRender = true;
            Promise.resolve().then(() => {
              this.needsRender = false;
              render(this.render(), this.shadowRoot);
            });
          }
        }
      }
      customElements.define('my-fruit', MyFruit);
    </script>

    <script type="module">
      import {html, render} from '//unpkg.com/lit-html/lib/lit-extended';
      class MyApp extends HTMLElement {
        constructor() {
          super();
          this.attachShadow({mode: 'open'});
        }
        connectedCallback() {
          this.fruit = 'apple';
        }
        get fruit() { return this._fruit; }
        set fruit(v) { this._fruit = v; this.invalidate(); }
        change() {
          this.fruit = this.shadowRoot.querySelector('#new').value;
          this.shadowRoot.querySelector('#new').value = '';
          this.shadowRoot.querySelector('my-fruit').froot = this.fruit;
        }
        render() {
          return html`
            <p>Fruit in parent: ${this.fruit}</p>
            <my-fruit froot="${this.fruit}"></my-fruit>
            <input id="new" type="text"/>
            <button on-click="${() => this.change()}">Change</button
          `;
        }
        invalidate() {
          if (!this.needsRender) {
            this.needsRender = true;
            Promise.resolve().then(() => {
              this.needsRender = false;
              render(this.render(), this.shadowRoot);
            });
          }
        }
      }
      customElements.define('my-app', MyApp);
    </script>
  </head>
  <body>
    <my-app></my-app>
  </body>
</html>

and the working lit-html-element example:

<!DOCTYPE html>
<html>
  <head>
    <script type="module">
      import { LitElement, html } from '//unpkg.com/lit-html-element';
      class MyFruit extends LitElement {
        static get properties() {
          return {
            froot: {
              type: String
            }
          }
        }
        render() {
          return html`
            <p>Fruit in child: ${this.froot}</p>
          `;
        }
      }
      customElements.define('my-fruit', MyFruit.withProperties());
    </script>

    <script type="module">
      import { LitElement, html } from '//unpkg.com/lit-html-element';
      class MyApp extends LitElement {
        constructor() {
          super();
          this.fruit = 'Apple';
        }
        change() {
          this.fruit = this.$('new').value;
          this.$('new').value = '';
          this.invalidate();
        }
        render() {
          return html`
            <p>Fruit in parent: ${this.fruit}</p>
            <my-fruit froot="${this.fruit}"></my-fruit>
            <input id="new" type="text"/>
            <button on-click="${() => this.change()}">Change</button
          `;
        }
      }
      customElements.define('my-app', MyApp.withProperties());
    </script>

  </head>
  <body>
    <my-app></my-app>
  </body>
</html>
kenchris commented 6 years ago

Will you follow up with an issue for lit-html? Maybe we should add it to the documentation for lit-html-element like common pitfalls

kenchris commented 6 years ago

So I basically fixed this for lit-html-element, if you would like to test and create some tests? :-)

  renderCallback() {
    if (this._resolved) {
      render(this.render(), this.shadowRoot as ShadowRoot);
    } else {
      const template = this.render().template;
      const rootNode = template.element.content;
      const walker = document.createTreeWalker(rootNode, NodeFilter.SHOW_ELEMENT, null as any, false);

      let deps = new Set();
      while (walker.nextNode()) {
        const element = walker.currentNode as Element;
        if (element.tagName.includes('-')) {
          deps.add(element.tagName.toLowerCase());
        }
      }

      Promise.all(Array.from(deps)
        .map((tagName) => customElements.whenDefined(tagName)))
        .then(() => {
          this._resolved = true;
          this.renderCallback();
        });
    }
  }
kenchris commented 6 years ago

28e8aae adds this patch, but it is still lacking tests. We might also be able to do this better, by maybe removing the shadowing property and render the elements pending upgrade at first. Or potentially we could do something inside lit-html instead