Netflix / x-element

A dead simple starting point for custom elements.
Apache License 2.0
29 stars 12 forks source link

Lazily computed properties can cause confusion #178

Closed klebba closed 3 months ago

klebba commented 7 months ago

A developer may attempt to access a lazily computed property value and be confused as to why it is not evaluated. Computed properties will only be evaluated when they are observed, used as input in downstream property computation, or included in the template function. Can we produce a warning in the console when an attempt is made to read from a still-lazy property?

klebba commented 7 months ago

fyi @theengineear — capturing a thought, not urgent

erahhal commented 7 months ago

I banged my head on this for a while, not realizing that props were lazy evaluated. I think the behavior makes sense though, so if there's a way to through a warning about unused computed props without affecting functionality or performance, perhaps we could add it.

theengineear commented 7 months ago

A developer may attempt to access a lazily computed property value and be confused as to why it is not evaluated.

Can you provide an example @erahhal? The property should be evaluated when you go to introspect it. I.e., what was the situation? Also, it would help to figure out the conditions under-which we’d want to issue a warning 👌

erahhal commented 7 months ago

Just create a computed property that isn't used anywhere, e.g. not the template. Put a console statement or breakpoint in the computed method. It doesn't get called.

theengineear commented 7 months ago

Ok, so here’s an example:

import XElement from 'https://deno.land/x/element/x-element.js';
class MyElement extends XElement {
  static get properties() {
    return {
      foo: { type: String, default: 'Foo' },
      bar: {
        type: String,
        input: ['foo'],
        compute: () => console.log('computing bar'),
      }
    };
  }
  static template(html) {
    return ({ foo }) => html`Hello ${foo}.`;
  }
}
customElements.define('my-element', MyElement);

It’s not possible to raise a warning without an interface change because:

  1. It may be the case that a computed property is meant to be on the public interface for integrators to inspect if they wish.
  2. It may be the case that a computed property (public or internal) is only accessed and therefore computed in an event handler (e.g., only if a user clicks or some such).
  3. It’s not possible to know whether a property will or won’t be used in a template. I.e., the template result can be dynamic at runtime.

I think the only thing we could do here is to change our default behavior to be less optimized — i.e., always compute all properties on every render cycle. Then, we could add a like beMoreLazy flag on the property to opt back into the current behavior? Is that maybe what you’re looking for?

erahhal commented 7 months ago

I wasn't looking for anything. Was just confused by the behavior before I understood what was happening, had a private chat with Casey, and he decided to open a ticket to track this as a potential pitfall for integrators. I only suggested the warning so as to be constructive here, but now that I understand how it works, it won't be an issue going forward for me.

Point 3 is the main reason not to throw warnings IMO. In any case, perhaps this behavior could be mentioned in the docs, if it's not already.

theengineear commented 7 months ago

Copy — we can absolutely document this better. I think that’s a good starting point.