Re: [w3c/webcomponents] connectedCallback timing when the document parser creates custom elements (#551)

@franktopel things I'd do differently:

 1. use a `WeakSet` for the already parsed bit
 2. I wouldn't cache the parents. Nodes can be easily moved around, caching parents wouldn't give you much performance boost but it might hide shenanigans
 3. the `setup` should be instead the `connectedCallback` of the `HTMLBaseElement` class, and the class should also have a childrenAvailableCallback no-op (or actually ignore everything on `comnnectedCallback` if `"childrenAvailableCallback" in this` is false)
 4. the method still doesn't solve the empty custom elements case, which is not edge at all, so that if the document is loading but there won't ever be children, the mutation observer will leak forever and no `childrenAvailableCallback` will ever be called
 5. I usually clean up before invoking "the unknown" so the mutation observer disconnect should be the first thing to do, so that if there is an error in the `childrenAvailableCallback` callback it doesn't backfire forever.

Accordingly, this is how I'd go, or what makes sense to propose as standard.

```js
const HTMLParsedElement = (() => {
  const DCL = 'DOMContentLoaded';
  const init = new WeakSet;
  const isParsed = el => {
    do {
      if (el.nextSibling)
        return true;
    } while (el = el.parentNode);
    return false;
  };
  const cleanUp = (el, observer, onDCL) => {
    observer.disconnect();
    el.ownerDocument.removeEventListener(DCL, onDCL);
    parsedCallback(el);
  };
  const parsedCallback = el => el.parsedCallback();
  return class HTMLParsedElement extends HTMLElement {
    connectedCallback() {
      if ('parsedCallback' in this && !init.has(this)) {
        init.add(this);
        if (document.readyState === 'complete' || isParsed(this))
          // ensure an order via a micro-task so that
          // parsedCallback is always after connectedCallback
          Promise.resolve(this).then(parsedCallback);
        else {
          // the need a DOMContentLoaded case
          const onDCL = () => cleanUp(this, observer, onDCL);
          this.ownerDocument.addEventListener(DCL, onDCL);
          // the early case still good to setup one
          const observer = new MutationObserver((changes) => {
            changes.some(record => {
              if (record.addedNodes.length) {
                cleanUp(this, observer, onDCL);
                return true;
              }
            });
          });
          // we are interested in the element nextSibling so
          // lets observe its parent instead of its own nodes
          observer.observe(this.parentNode, {childList: true});
        }
      }
    }
  };
})();
```

Observing the `parentNode` still might hide shenanigans but at least is its only direct container and not the whole document so it's IMO most likely a more reliable approach.

Eventually, the observer could be configured as `{childList: true, subtree: true}` and the `if` should check `if (isParsed(this))` instead of checking `record.addedNodes.length`.



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/webcomponents/issues/551#issuecomment-431309659