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]

Uncaught Error: Unexpected value for "XPages.properties.currentElement" (expected HTMLElement).

Would be nice to say got ${value} here as well

[klebba]

Error: Property "XPages.properties.currentElement" has resolver (cannot be set).

Might read better as:

Error: Property "XPages.properties.currentElement" is read only. (defined by resolver ${resolverName}).

Or similar

[klebba]

observer: this.observeModel,

Does not throw even though this function does not exist

[klebba]

Here's an issue that seems like a bug:

      model: {
        type: Object,
        observer: this.observeModel,
      },
      currentElement: {
        observer: this.observerCurrentElement,
      },
      binding: {
        type: Boolean,
        internal: true,
        dependencies: ['currentElement', 'model'],
        resolver: this.computeBinding,
      },

The resolver function for binding is never called even though I'm able to confirm that both observers (this.observerModel and this.observerCurrentElement) are being called with the values I would expect. Note: I'm trying to port x-pages

[theengineear]

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

It will be a runtime error that will just keep reoccurring every time something asks for that value.

[theengineear]

If I remove the async it appears to work

Thanks, need to check for either AsyncFunction or Function.

EDIT: This is fixed now.

[theengineear]

observer: this.observeModel, Does not throw even though this function does not exist

Yah, I've been loose about undefined. I'll change it to a Reflect.has check instead of !== undefined.

EDIT: This is fixed now.

[theengineear]

The resolver function for binding is never called even though I'm able to confirm that both observers (this.observerModel and this.observerCurrentElement) are being called with the values I would expect. Note: I'm trying to port x-pages

@klebba — not sure if I'd consider it a bug, we decided to move observation after rendering and remove synchronicity. I think that might be where the trouble is coming from. What if we just untangled the observer callbacks so you don't have observers setting properties which themselves have observers?

class XPages extends XElement {
  static get properties() {
    return {
      current: {
        type: String,
        reflect: true,
      },
      items: {
        type: Object,
      },
      model: {
        type: Object,
      },
      update: {
        type: Object,
        internal: true,
        dependencies: ['current', 'items', 'model'],
        resolver: (current, items, model) => ({ current, items, model }),
        observer: this.observeUpdate,
      },
    };
  }

  static observeUpdate(host, { current, items, model }, oldValue) {
    const container = host.shadowRoot.getElementById('container');
    if (current && current === oldValue.current) {
      // same page, re-bind the model
      container.firstChild.model = model;
    } else if (current && current !== oldValue.current) {
      // new page, clean up old page and create next page
      container.firstChild && container.firstChild.remove();
      const element = document.createElement(items[current]);
      container.appendChild(Object.assign(element, { model }));
    } else {
      // no page, insert a slot so default content can be displayed
      container.firstChild && container.firstChild.remove();
      container.appendChild(document.createElement('slot'));
    }
  }

  static template(html) {
    return () => html`
      <style>
        :host,
        #container,
        #container > * {
          display: flex;
          flex-direction: column;
          flex-grow: 1;
        }
      </style>
      <div id="container"></div>
    `;
  }
}

[theengineear]

Would be nice to say got ${value} here as well

Yah, I haven't spent much time thinking about the messaging. I figured I'd start by at least making sure the things throw when we want them to throw.

EDIT: Error messages have been updated.

[klebba]

Thanks for all the fixes! Your x-pages refactor makes sense to me, but I still wonder why my callback never got called — maybe I'm missing something. Let's review together and I'll explain why I expected it to work

[theengineear]

but I still wonder why my callback never got called

Was it because you were using a resolver for a side effect and never looking at the actual value? Resolvers are lazily invoked, if you never ask for the return value, it never gets called. Moreover, they're memoized, we shouldn't depend on side effects in them.

[theengineear]

@klebba — that code coverage though.

# Coverage goal of 100% for http://0.0.0.0:8080/node_modules/@netflix/x-element/x-element.js met (got 100.00%).

I actually caught a couple bugs attaining that 100% code coverage mark! Totally worth the effort 👍

[klebba]

Wow, awesome!

[klebba]

What do you think about adding a lazy: false flag to force evaluation of a computed property?

[klebba]

The message Property "DemoView.properties.guardRemove" is internal (element authors may use "internal") might be more clear as Property "DemoView.properties.guardRemove" is internal (internal.${prop})

[klebba]

I hit an issue where my dependency set for a computed property includes an internal property; Property "XInspector.properties.myProp" is internal (element authors may use "internal"). — seems like this should work right?

[theengineear]

I hit an issue where my dependency set for a computed property includes an internal property; Property "XInspector.properties.myProp" is internal (element authors may use "internal"). — seems like this should work right?

It's hard for me to debug if I don't have an example. It shouldn't be an issue to include internal properties as dependencies for resolved properties. If you are seeing this error, then I've made a mistake for sure and I'll fix it.

I tried to repro, but I wasn't able to hit the error you're seeing.

[theengineear]

The message Property "DemoView.properties.guardRemove" is internal (element authors may use "internal") might be more clear as Property "DemoView.properties.guardRemove" is internal (internal.${prop})

Yah, I can reword. I'm just nervous about obscuring the fact that this is for authors only.

EDIT: Reworded.

[theengineear]

What do you think about adding a lazy: false flag to force evaluation of a computed property?

Can you provide an example of when you'd want this? I've thought about this before, but I can't think of any examples that don't feel a bit like an anti-pattern.

[klebba]

I figured out the issue with my internal pipeline; turns out it had nothing to do with the internal setting. The error message was a bit of a red herring. I had a name collision with a true DOM property — parentElement — and XElement freaked out as a result. Not certain what we can do here; would be a little annoying to keep a list of know properties that could collide with user defined properties. Any thoughts?

...

Thinking about this more; we should probably throw in certain cases when names are reserved for library use. For example we should not let a user attempt to define a property named internal

[theengineear]

Yep, we already keep a list of library-reserved properties. Try setting a property to internal. You should get a nice error.

As for shadowing in general, can we just lookup the property name after we've deleted it off the host during upgrade? I.e., if your prototype chain has that property already, throw an error.

Note that sometimes I resolve a title property, so in this case, I'm purposefully shadowing. Perhaps that's bad behavior though since you could always:

hostTitle: {
  observer: (host, value) => Object.assign(host, { title: value })
}

Then you have the concern of things that didn't shadow yesterday that shadow tomorrow. I.e., is it appropriate to throw on a developer for something that can break when Chrome updates? I'll try it because it's the most useful for developers to throw hard I think. Also, my guess is that these things don't actually update that often.

If we really wanted. We could perform a build script that injects the list based on the newest Chrome before running and have a test that basically asserts that this list doesn't change. Then, because you have to update the test (constituting a breaking change), a full version bump would be required. Feels maybe overkill at this stage though.

Here's what I used to grab all the property names:

const ALL_PROPERTY_NAMES = new Set();
let prototype = XElement.prototype;
while (prototype) {
  for (const propertyName of Object.getOwnPropertyNames(prototype)) {
    ALL_PROPERTY_NAMES.add(propertyName);
  }
  prototype = Object.getPrototypeOf(prototype);
}

I believe this is what we want. Note that non-enumerable names are included, but not properties that have symbols for names.

By the way, we currently just ignore symbols, but I think we should specifically throw an error if we detect one using Reflect.ownKeys in our static analysis step. We could consider allowing them later, but I can't think of a reason why you would need one and it makes attribute reflection pretty confusing! I'll do that as well.

^^ Same goes for non-enumerable, non-symbol property names. There's no need to do that and we shouldn't allow it.

[theengineear]

FYI out of ~400 elements checked, the only shadowed property names were the following (from 6 elements):

• id
• title

EDIT: BUT, if you remove any prepended _ characters, you see that actually 17 elements are effected and the list would be:

• id
• title
• style

Once the internal field is provided and the prepended _'s are no longer necessary.

[klebba]

Minor comment:

throw new Error(`Property "${path}" is resolved (resolved properties are read-only).`);

Would be more clear as:

throw new Error(`Property "${path}" is computed (computed properties are read-only).`);

[theengineear]

throw new Error(Property "${path}" is computed (computed properties are read-only).);

Hmm, didn't we want walk away from the language of "computed"? This would be the only place that we'd use that word.

[klebba]

My thinking here is that we still have computed properties, but the way we define them is to specify their dependencies and resolver function — a property definition which has these fields is considered computed — we could also call it derived if you want to disambiguate from old and new implementations

[theengineear]

Got it. Yah, instead of me updating this right now, maybe we can bike shed as a group. Ideally, the language used in the property definitions matches the language we see in the error messages and matches naming conventions in the code.

[klebba]

Sounds like a plan. I have an element that hooks into afterInitialRender -- do we still intend to include hooks like this?

Related: it seems like the render() method is no longer a public method available for override, breaking some of my integrations where I insert my code into this part of the element lifecycle

[klebba]

Total nit: I recommend we rename propertiesProxy to propertyProxy

[klebba]

I have an observer that I use when a value is first defined; e.g. when newValue is defined and oldvalue is undefined I kick off a subroutine. However the observer function is called many times in succession with the same values for newValue and oldValue, making it difficult to detect the edge where newValue is first populated. This seems to differ from the last implementation, is this expected?

[theengineear]

Total nit: I recommend we rename propertiesProxy to propertyProxy

🤔 but it's a proxy for the properties object. that doesn't make sense to me, but I'll change it if you want 👍

[theengineear]

Related: it seems like the render() method is no longer a public method available for override, breaking some of my integrations where I insert my code into this part of the element lifecycle

We can add it back. I'm just trying to make sure we need all of these things. Can you paste an example of this to help me understand? I've never seen this method used outside of a render guard, which is an anti-pattern.

[theengineear]

I have an element that hooks into afterInitialRender -- do we still intend to include hooks like this?

In previous conversations, we decided that many of these hooks bloated our codebase. Let's discuss your use-case and decide if it's strictly necessary. I would really like to error on the side of having a smaller interface wherever possible.

(in theory, i agree with you, but i want to challenge us to really think through the necessity of every bit of surface area we expose)

[theengineear]

I have an observer that I use when a value is first defined; e.g. when newValue is defined and oldvalue is undefined I kick off a subroutine. However the observer function is called many times in succession with the same values for newValue and oldValue, making it difficult to detect the edge where newValue is first populated. This seems to differ from the last implementation, is this expected?

I must have introduced a bug. I thought I had a test for exactly this case, but I'll dig in tonight. You should never get called with the same arguments in an observer. Sorry about this!

[theengineear]

@klebba , how would you feel about having the following:

renderCallback(initial) {
  console.log('this is the first time we rendered', initial);
}

A couple notes:

1. Note that there's no skipping a render anymore. Previously, you could just not call super.render and that could potentially cause all sorts of confusion since the DOM won't have been created which can be super confusing. Instead, you are forced to render something, even if it is nothing.
2. Instead of two callback functions, we just give you a boolean flag initial which indicates whether or not you've rendered for the first time ever.
3. You cannot render. The rationale for this is that it can again be a serious source of confusion. E.g., what happens if you manually call render before you've connected? The host won't have been initialized and it would be super confusing.
4. If we must have a way to cause a render, I'd rather expose invalidate again, which would simply enqueue a render in the future under-the-hood.

At any rate, I think the renderCallback (so named to match the conventions and flavors of other, native callbacks) may be enough for your needs.

[theengineear]

I must have introduced a bug. I thought I had a test for exactly this case, but I'll dig in tonight. You should never get called with the same arguments in an observer. Sorry about this!

I have a test case that specifically touches this functionality. Can you paste an example @klebba?

[theengineear]

Total nit: I recommend we rename propertiesProxy to propertyProxy

So, I did a bit of renaming such that internally, we have propertyMap and attributeMap and listenerMap. This allows us to deconflict referring to these objects as just properties and internal in our instance-side code. It's a bit of overloading, but I think that it ends up reading well.

In other words, propertiesProxy >> properties and internalPropertiesProxy >> internal.

[theengineear]

Related: it seems like the render() method is no longer a public

I did some refactoring here to to allow this again. As we discussed, you can break things if you misuse render right now (e.g., infinite loop because you set a property inside render). (I may still add a guard to prevent render from being called before the host is initialized).

[klebba]

Working through more integration; I wonder if it would be a good idea to make it possible to return the last references from a computed function. The use case is that I want to scan the incoming dependencies and detect if I should mint a new reference (e.g. another array) or just return the last one. Since the reference won't have changed, downstream dependency resolvers would not be called

[theengineear]

I wonder if it would be a good idea to make it possible to return the last references from a computed function

That's an interesting idea and could be pretty neat:

static get properties() {
  return {
    a: {},
    b: {},
    c: { dependencies: ['a', 'b'], resolver: this.resolveC },
  }
}

// Just pass the last value.
static resolveC(a, b, oldValue) {/* .. */}

// Pass everything as an object.
static resolveC(a, b, { args: [lastA, lastB], value }) {/* .. */}

[theengineear]

@charricknflx @codeStryke @erahhal — Here's that PR I was talking about. I think a good starting point might be to read through the top-level comment (description) in this PR. Another good kickoff point would be to peruse the SPEC.md file in this change set. Obviously, feel free to dig into whatever you want though! Perhaps we can kick off a discussion more fomally on the topic next week.

[klebba]

Caught an issue tonight; we can't use BigInt yet unless we want to drop support for Safari. I recommend we hold off on including it in the serializable types for now