43081j / eslint-plugin-wc

ESLint rules for Web Components
MIT License
95 stars 10 forks source link

docs: Improve `no-child-traversal-in-connectedcallback` #130

Open silverwind opened 7 months ago

silverwind commented 7 months ago
  1. Calling MutationObserver#observe without options is invalid and raises a TypeError.
  2. When custom elements are rendered with Vue, I noticed the children are available during connectedCallback and the suggested MutationObserver method does receive any mutation events. I assume Vue must use .innerHTML or similar method to append the component to the DOM.
43081j commented 7 months ago

do you have an example somewhere of the 2nd point?

sounds unusual so would be good to get our heads around it before we document it.

even if they add the outer nodes themselves, surely the custom element is responsible for its own children. are we talking about a situation where there's no shadow DOM? etc. think i just need to see an example

silverwind commented 7 months ago

Yes, I will try to make a jsfiddle to demonstrate. If it really is .innerHTML causing this, I will remove the reference to Vue specifically.

silverwind commented 7 months ago

Reproduced with these in Firefox, Chrome and Safari:

My conclusion is that a WC that wants to reliably access its children needs to do something like this to be compatible to both rendering methods:

connectedCallback() {
  if (this.children.length) 
    // access this.children
  } else {
    const observer = new MutationObserver(() => {
      observer.disconnect();
      // access this.children
    }).observe(this, {childList: true});
  } 
}
43081j commented 7 months ago

ok now i understand 👍

i don't think this is actually something to put in the reasons for not using the rule. i think its the example which is poor

in one of my projects, i have roughly the same pattern but with the initial call like you suggested:

constructor() {
  this._observer = new MutationObserver(...);
}

connectedCallback() {
  // initial call because the observer will observe from the next change
  this._onChildrenChanged();
  // observe so followup changes trigger too
  this._observer.observe(this, {childList: true});
}

i think we should just make the example clearer - calling the observer's callback as well as observing

silverwind commented 7 months ago

Can you share all related code so I see how you handle it?

BTW, I think the second example with the addEventListener('update') is also poor, because I don't know what this update event is and what triggers it. I think such examples should be fully self-contained to be useful.

Maybe @keithamus also wants to chime in.

43081j commented 7 months ago

basically the pattern we use in our projects are roughly this (off the top of my head so not exactly the code from our repos):

class FooElement extends HTMLElement {
  constructor() {
    this._observer = new MutationObserver((change) =>
      this._onChildrenChanged(change));
  }

  _onChildrenChanged(change?) {
    // do things now that child nodes changed
    // if `change` is nullish, treat it as if all children changed
  }

  connectedCallback() {
    this._onChildrenChanged(); // Call it immediately for any already-appended children

    this._observer.observe(this); // Observe the light DOM from now on
  }
}

the examples of "correct code" in the docs are meant to be simple though. so if we end up having to document a more complex example like this, im skeptical of if we should even keep it

it may just not be worth mentioning mutation observer, or we do a cut down version of this example

silverwind commented 7 months ago

Thanks, I see you are using the same basic concept I do, the only difference is that you call _onChildrenChanged unconditionally while I actually verify presence of at least one child, because in my case my component has one mandatory child element.

Your method would trigger _onChildrenChanged even when there was no actual change to children, for example when there are none.

BTW, as seen in https://github.com/WICG/webcomponents/issues/809, it is a popuplar request for the standard to include a lifecycle callback once children are available, but so far there has been no movement on it. Honstly I'm really surprised that such basic functionality is missing in the standard.

keithamus commented 7 months ago

BTW, as seen in WICG/webcomponents#809, it is a popuplar request for the standard to include a lifecycle callback once children are available, but so far there has been no movement on it. Honstly I'm really surprised that such basic functionality is missing in the standard.

The main reason is that people cannot decide how to correctly approach it. There is debate on whether or not elements deferring their logic until they see their closing tag is an anti-pattern. And for cases other than that, it seems there are multiple ways a component can consider doing setup: using a MutationObserver or deferring work until some event is fired.