Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

silverwind
Copy link

@silverwind silverwind commented Mar 3, 2024

  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
Copy link
Owner

43081j commented Mar 5, 2024

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
Copy link
Author

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
Copy link
Author

silverwind commented Mar 6, 2024

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
Copy link
Owner

43081j commented Mar 6, 2024

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
Copy link
Author

silverwind commented Mar 6, 2024

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
Copy link
Owner

43081j commented Mar 6, 2024

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
Copy link
Author

silverwind commented Mar 6, 2024

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 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.

@@ -52,3 +52,5 @@ class FooBarElement extends HTMLElement {
## When Not To Use It

If you are comfortable with the edge cases of DOM traversal directly in the `connectedCallback` then you can disable this rule.

If your element is appended to the DOM via `.innerHTML` or similar methods, you should disable this rule because in such a case, the children are are available during `connectedCallback` and no mutations events will fire.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add this because it's typically not possible to know what happens to your element, as it is up to the consumer how it is embedded.

For example if your element is appended to the DOM using append(), and you have disabled this rule, your code breaks. If your element is served from the source HTML, and browser decides to render, your element is connected but incomplete, you code breaks.

The point of this rule is to remind you that those cases exist, and to carefully think about how you want to approach them.

Copy link
Author

@silverwind silverwind Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, disabling the rule as a whole is definitely too crude. I think a eslint-disable-line would be suitable for cases where you know exactly how your element is going to be inserted. Maybe it should just recommend that.

Copy link
Author

@silverwind silverwind Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could at least hint somewhere in the rule text that depending on the renderer, children may be available or not and I think any universally usable component that deals with its children has to check for both immediately and delayed availability of children.

I just fear that people who blindly follow this rule will create components that can not work correctly when inserted via .innerHTML, which I think is very common with frameworks like React or Vue.

@keithamus
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants