Simplify x-element.

[theengineear]

Concept branch — the goal here is to reimagine x-element based on our learnings from using it in hundreds of components in a complex SPA over the last year.

Significant changes:

• Static analysis is done up-front, once.
• Anything that can be validated statically throws errors except for interface shadowing which changes as browsers update — the best we can do is console.warn in this case.
• Almost all of the XElement interface is static (though we provide trivial convenience wrappers). Note that render is the only non-trivial instance method.
• Static interface is ready for private (#) static fields when they're ready.
• observer >> observe is now a function (proper context is maintained if it's a constructor method).
• computed >> input + compute. compute is now a function (proper context is maintained if it's a constructor method). input is an array of property names which will be resolved to arguments at runtime and passed to compute.
• value is now initial and default. initial is used if initial value is nullish. default is used if property value is ever nullish.
• Type coercion is no longer done on set.
• Errors are thrown when you set a value with an incorrect type in the runtime.
• Concept of readOnly is first-class.
• Concept of internal is first-class.
• Internal setting/getting and read-only setting can be done via the .internal proxy.
• Computed properties are now lazily evaluated.
• Observation, reflection, and computation are no longer synchronous. Importantly, this allows us to set properties one-by-one and then get a single compute at the end.
• Concept of a "render root" is now supported via createRenderRoot.
• Initial template function is now cached on each host (vs defining a new function on each render).
• We always allow computation of computed properties before any arguments are defined. Previously, we had thought we might want to defer initial compute until at least one argument was defined.
• Swapped the order of values in observe callbacks to be value, oldValue — it was previously oldValue, value.
• Order of operations are now compute, reflect, render, then observe.
• Listener logic has been improved and interface is now a function, not a string (proper context is maintained if it's a constructor method).
• Coercion of null and undefined to '' is removed, the underlying templating engine takes care of this for us.
• The notion of a render proxy is gone. Previously we had only used it to convert null and undefined to the empty string. However, that is handled by the templating engine now.
• Deserialization of anything other than [Boolean, String, Number] now throws a halting error (e.g., setting an attribute for a property that is of type: Object). Previously, we basically did nothing, especially for custom classes.

[theengineear]

@klebba — mind taking a look when you have the chance? Also happy to walk you through the changes. I think you'll like the general direction here.

[theengineear]

The most recent change is because I was using some functionality not supported in eslint yet because it's stage 3 functionality. I didn't think the code suffered much to walk it back, so I just made sure it adhered to stage 4 functionality.

[theengineear]

@klebba — I left a couple things in limbo here. Please hold off on looking at the code until I get the chance to fix.

[klebba]

I tried swapping this in for a feature I'm working and might have caught an issue; my observer functions are not being called yet. Here's my property block:

  static get properties() {
    return {
      targetRect: {
        type: DOMRect,
      },
      open: {
        type: Boolean,
        reflect: true,
        observer: this.observeOpen,
      },
      _parentElement: {
        type: HTMLElement,
      },
      _cssProperties: {
        type: Object,
        dependencies: ['_parentElement', 'targetRect', 'open'],
        resolver: this.computeCSSProperties,
        observer: this.observeCSSProperties,
      },
    };
  }

My demo page has markup like:

<x-inspector debug open .targetRect="${rect1}">[...]</x-inspector>

So some of the values need to be read immediately following element upgrade. Also I just grabbed whatever I saw in the PR for x-element.js a few minutes ago so maybe it's not ready for this yet, but I thought I'd give it a try because I think the change in order for render/reflect etc might be helpful for this feature

[theengineear]

So some of the values need to be read immediately following element upgrade.

The initial update is done synchronously upon the first connectedCallback. This will cause, synchronously, your properties to be initialized, computed, reflected, rendered, and then observed.

I think that your component will be supported, the PR has just been thrashing a bit. Importantly, I think that properties block is looking good! As a reminder, the new way you'd want to write this would be like:

class CustomElement extends XElement {
  static get properties() {
    return {
      targetRect: {
        type: DOMRect,
      },
      open: {
        type: Boolean,
        reflect: true,
        observer: this.observeOpen,
      },
      parentElement: {
        type: HTMLElement,
        internal: true,
      },
      cssProperties: {
        type: Object,
        internal: true,
        dependencies: ['parentElement', 'targetRect', 'open'],
        resolver: this.computeCSSProperties,
        observer: this.observeCSSProperties,
      },
    };
  }
}

[theengineear]

@klebba I circled back on the issue you were having and perhaps there is a different bug that I'm misunderstanding. I've had a test in place the entire time to ensure that resolved, observed properties are synchronously called upon initial connection.

Would you mind trying to create a minimal example element that demonstrates your issue? It'd be super helpful!

[theengineear]

OK, I'm willing to say that it makes sense to start trying to use this in your work as an experiment. I'm quite sure there will be some issues that need ironing out, but things should be stabilizing at this point.

[klebba]

Let's start curating a migration guide:

was:

static get properties {
  return {
    myProperty: {
       type: String,
       computed: 'computeMyProperty(foo, bar)',
    }
  }
}

now:

static get properties {
  return {
    myProperty: {
       type: String,
       resolver: this.computeMyProperty,
       dependencies: ['foo', 'bar'],
    }
  }
}

was:

this.listen(target, 'eventName',  callbackPointer);
this.unlisten(target, 'eventName',  callbackPointer);

now:

this.addListener(target, 'eventName',  callbackPointer, options);
this.removeListener(target, 'eventName',  callbackPointer);

Also a question; if I throw in a resolver function what happens?

[klebba]

static async observePage(host, current, last) {

This throw an error:

Uncaught Error: Unexpected value for "XPages.properties.page.observer" (expected Function).

If I remove the async it appears to work

[klebba]

I hit an issue with a compute function that stopped working. This element presents the problem: https://gist.github.com/klebba/cd795a01223beaa219c8560db1ec512c — the computeBinding function is never actually called. I tried renaming and adding other functions with different inputs, but none were called. Any ideas?

Edit:

Damn, the lazily evaluated properties got me again! 🤦 However this time adding an observe function does not do the trick; I have to include it in my template although it is unused. Is this expected?

[klebba]

Is it possible to throw an error when a listen or unlisten callback is not found? Would be nice feedback

[theengineear]

I hit an issue with a compute function...

Yep, we stopped forcing the compute callbacks inside render completely now. I don't understand why setting a dummy observe wouldn't work though 🤔

static get properties() {
  return {
    a: {},
    b: {},
    c: {
      input: ['a', 'b'],
      compute: (a, b) => a.sideEffect(b),
      observe: () => {}, // dummy observer to force a sideEffect in compute.
    },
  };
}

☝️ I'd expect that to work. Note that observe is called after render — could that be the issue? You're getting the side effect, but it's happening only render has occurred? If that's the case, you could also pluck it from the properties proxy inside your render routine.

static get properties() {
  return {
    a: {},
    b: {},
    c: {
      input: ['a', 'b'],
      compute: (a, b) => a.sideEffect(b),
    },
  };
}

static template(html) {
  return properties => {
    properties.c; // forces a side effect inside compute
    const { a, b } = properties;
    return html`<div>${a}</div><div>${b}</div>`;
  }
}

[theengineear]

Is it possible to throw an error when...

For sure, we do that for the static listeners block — seems consistent to do it when using listen / unlisten as well.

[theengineear]

@klebba — listen and unlisten through insightful errors when passed something other than a Function now.

EDIT: Actually, I went ahead and validated all the arguments 👌

[klebba]

Based on our conversation I reworked my x-pages variant here: https://gist.github.com/klebba/cd795a01223beaa219c8560db1ec512c#file-x-pages-js-L33 — (note line 33) — this cut down quite a bit on lines of code and should actually "do about the same thing" with a negligible performance delta (if any)

[theengineear]

@klebba — the host is now passed as the second argument to the template callback again.

[theengineear]

@codeStryke — as requested, I updated the documentation in the source code.

[klebba]

@theengineear can you remind me what all remains here? If my memory serves, we're really close! 🥇

[theengineear]

We need to complete the refactor in our application to prove that we like this change set. Then, we can merge / publish. There's no real reason to merge this PR in before we've finished testing it on the ~500 components we have IMO. At a minimum, I'd like to get 50% of these components refactored before merging this in. We can always merge it and then wait to publish. It's just that once we merge it in, we break our ability to introduce any sort of hot-fix.

…

On Sat, Sep 26, 2020 at 4:42 PM Casey ***@***.***> wrote: @theengineear <https://github.com/theengineear> can you remind me what all remains here? If my memory serves, we're really close! 🥇 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSOEWU6JNW2A7NIFM2QN33SHZ35DANCNFSM4NJ4XLWA> .

[theengineear]

OK — new branch, new PR. I'm going to close down this exploratory PR in favor of a fresh start (just a copy of what's here).

Let's continue the discussion (if there's more to discuss) on #69.