Consider inverting order of initialization / listener-setup.

theengineear commented 1 year ago

Because the initial render of an x-element is synchronous, events dispatched by children in their first render or observe methods will not be heard by parent listeners!

One solution would be to literally swap these lines in x-element.js:

  connectedCallback() {
    XElement.#initializeHost(this);
    XElement.#addListeners(this); // ^^ If we move this line up, it should fix the root issue.
  }

The obvious risk here is that we could introduce a race condition where a listener is expecting some DOM to exist before initialization is complete. I believe that shouldn't be possible though since the tree should be resolved depth-first — i.e., all the DOM should arrive just in time in this case.

Here's a repro html file (scroll to the bottom for the printout):

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <title>Listener Race Condition</title>
  </head>
  <body>
    <script type="module">
      import XElement from 'https://unpkg.com/@netflix/x-element@1.0.0-rc.50/x-element.js';
      class ParentElement extends XElement {
        static get listeners() {
          return {
            error: (host, event) => {
              console.log(`parent element caught "${event.error.message}"`);
            }
          };
        }
        static template(html) {
          return (properties, host) => {
            console.log('rendering parent element');
            // This is just one way to log out when the listener is added.
            host.shadowRoot.addEventListener = (...args) => {
              console.log(`listening for "${args[0]}" on parent element's shadow root`);
              host.addEventListener.call(host.shadowRoot, ...args);
            }
            return html`<child-element></child-element>`;
          }
        }
      }
      customElements.define('parent-element', ParentElement);

      class ChildElement extends XElement {
        static template(html) {
          return (properties, host) => {
            console.log('rendering child element');
            console.log('dispatching error from child element');
            host.dispatchError(new Error('Did you catch that?'));
            return html`<div>child component</div>`;
          }
        }
      }
      customElements.define('child-element', ChildElement);

      // Render everything synchronously.
      console.log('listening for "error" on document')
      document.addEventListener('error', event => {
        console.log(`document caught "${event.error.message}"`);
      });
      console.log('injecting parent element');
      document.body.append(document.createElement('parent-element'));
    </script>
  </body>
</html>

If you run that, you should see the following logs:

listening for "error" on document
injecting parent element
rendering parent element
rendering child element
dispatching error from child element
document caught "Did you catch that?"
listening for "error" on parent element's shadow root

☝️ — See how listening for "error" on parent element's shadow root happens too late?

theengineear commented 1 year ago

FYI @charricknflx — I documented this issue that you came across and came up with what I think is a pretty straightforward repro.

So far, I haven't figured out what could go wrong. Additionally, I swapped the lines locally and ran our test suite — all checks still pass. There might be some very good reason not to set up listeners ahead of the initial render… but it hasn't dawned on me yet 😅

theengineear commented 1 year ago

FYI @klebba — Not sure if this is something that you've ever come across. It's a pretty edge-y edge-case, but feels like it warrants attention.

klebba commented 1 year ago

Nice find and great context! Thanks @theengineear — I haven't come across this situation myself but my first thought is that it seems correct to ensure no events are missed. Out of curiosity how did we encounter the edge case? Either way, I would support making the change

theengineear commented 1 year ago

my first thought is that it seems correct to ensure no events are missed

Yep. I'm leaning in the same direction on this one. Seems like the right fundamental position to ensure this for listeners setup through the system.

Out of curiosity how did we encounter the edge case?

In development, Chris had setup a validation system that works roughly like this:

A <context-menu> component is passed a JSON object which declares the setup for the entire menu.
Items in the context menu (e.g., <context-menu-item> are each passed their individual chunk of properties.
Each item individually validates their properties in the observe callback and fires an event.
The top-level <context-menu> listens to all the events, adds a path for context, and re-dispatches a top-level event.

It's hard, but possible to configure / render this DOM tree synchronously (if you have the demo set up just right) and there's an edge case where the top-level listeners miss the lower-level events.

And by "just right" above, note how in the top-level description's code snippet, we don't manually add document.body.append(document.createElement('parent-element')); inside the script file. Again, it's an edge case, but I do think we should try and support it for consistency.

klebba commented 1 year ago

I see — that's interesting. Programmatically dispatched events (rather than user initiated) are typically avoidable in my experience, but again I wouldn't use that as a reason not to make this change

theengineear commented 1 year ago

Closed by #126.

Netflix / x-element

Consider inverting order of initialization / listener-setup. #125