Netflix / x-element

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

Simplify template error message once error causes are better-supported. #169

Closed theengineear closed 2 days ago

theengineear commented 9 months ago

See #128 for the original motivation here. Just opening this ticket to simplify some of our error logging to keep things as tidy as possible.

Current:

  render() {
    const { template, properties, renderRoot, render } = XElement.#hosts.get(this);
    const result = template(properties, this);
    try {
      render(renderRoot, result);
    } catch (error) {
      const pathString = XElement.#toPathString(this);
      const message = `${error.message} — Invalid template for "${this.constructor.name}" at path "${pathString}"`;
      throw new Error(message, { cause: error });
    }
  }

Proposed:

  render() {
    const { template, properties, renderRoot, render } = XElement.#hosts.get(this);
    const result = template(properties, this);
    try {
      render(renderRoot, result);
    } catch (error) {
      const pathString = XElement.#toPathString(this);
      const message = `Invalid template for "${this.constructor.name}" at path "${pathString}"`;
      throw new Error(message, { cause: error });
    }
  }

☝️ — See the difference? We ought to not have to literally repeat / embed the original error in our own messaging.

theengineear commented 9 months ago

Note that this is currently supported in Firefox. Here’s a link to the Chrome bug.

Screenshot 2024-02-13 at 1 15 37 PM
theengineear commented 4 weeks ago

This seems to work in all browsers now. BUT — we may want to consider that the cause there may or may not itself have an error associated. I.e., an error cause could be the original error, or it could be any compound object… I’m actually surprised / disappointed to learn this. It feels like a bad abstraction to me 🤷

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

theengineear commented 2 days ago

We’ve made the related changes here. Closing.