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

[idea] childConnectedCallback and childDisconnectedCallback #550

Closed
trusktr opened this issue Aug 22, 2016 · 33 comments
Closed

[idea] childConnectedCallback and childDisconnectedCallback #550

trusktr opened this issue Aug 22, 2016 · 33 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Aug 22, 2016

I think having such methods would make it much easier than using MutationObserver for doing things when children are connected to a custom element.

Some reasons:

  • Suppose we wish to use connectedCallback to initiate a mutation observer, and disconnectedCallback to disconnect the observer, rather than initiating it in createdCallback or constructor. This has the side-effect that by the time connectedCallback is fired that the custom element may already have had children added to it, so the mutation observer won't catch those.

  • Using MutationObserver requires writing at least two nested for loops, which is much more verbose than simply placing logic inside a childConnectedCallback(child) method. f.e. compare

    connectedCallback() { // or perhaps in createdCallback or constructor?
        const observer = new MutationObserver(changes => {
            for (let change of changes) {
    
                for (let node of change.addedNodes) {
                    if (node instanceof MotorHTMLNode)
                        this.imperativeCounterpart.addChild(node.imperativeCounterpart)
                }
    
                for (let node of change.removedNodes) {
                    if (node instanceof MotorHTMLNode)
                        this.imperativeCounterpart.removeChild(node.imperativeCounterpart)
                }
    
            }
        })
        observer.observe(this, {
            childList: true
        })
    }

    vs

    childConnectedCallback(node) {
        this.imperativeCounterpart.addChild(node.imperativeCounterpart)
    }
    childDisconnectedCallback(node) {
        this.imperativeCounterpart.removeChild(node.imperativeCounterpart)
    }
  • The API is just easier, which is a good thing for people learning the Custom Element API.

On a similar note, it may be nice to have element-specific methods. For example (and as a possible alternative to my distributedCallback idea), HTMLSlotElement could have a childDistributedCallback(child) method, and it wouldbe possible to make a Custom Element that extends from HTMLSlotElement and to use it in place of the default <slot> element.

class MySlot extends HTMLSlotElement {
  childDistributedCallback(child) {
    console.log('Child distributed into this slot:', child)
  }
}

customElements.define('my-slot', MySlot)
<some-shadow-tree>
  <my-slot>
  </my-slot>
</some-shadow-tree>

I know we'll be able to use slotchange events, but the methods might be easier to work with.

@rniwa
Copy link
Collaborator

rniwa commented Aug 22, 2016

In v0 API, there was childrenChangedCallback at one point. I think it got removed due to performance reasons.

@WebReflection
Copy link

WebReflection commented Aug 22, 2016

it's also an overlap with MutationObserver that can be easily simplified through user-land libarries or generic Custom Element class that implements observable changes in its constructor (or add/removes them once connected/disconnected)

@rniwa
Copy link
Collaborator

rniwa commented Aug 22, 2016

MutationObserver doesn't get invoked until the end of microtask whereas custom element callbacks get invoked almost immediately so it isn't quite polyfillable.

@WebReflection
Copy link

well, it could but that's undesired, hence my suggestion to use MutationObserver instead (even if not quite the same, I guess it's fine for 99% of use cases)

@trusktr
Copy link
Contributor Author

trusktr commented Aug 24, 2016

MutationObserver doesn't get invoked until the end of microtask whereas custom element callbacks get invoked almost immediately so it isn't quite polyfillable.

well, it could but that's undesired, hence my suggestion to use MutationObserver instead (even if not quite the same, I guess it's fine for 99% of use cases)

Actually, I'm running into hitches due to this, which makes it difficult to coordinate some of my connected/disconnected logic with my polyfill.

For reference, my polyfill looks like the following in my WebComponent base class (so notreally a "polyfill"), with code omitted for brevity:

// some code omitted for brevity...

export default
function WebComponentMixin(elementClass) {
    if (!elementClass) elementClass = HTMLElement

    // some code omitted for brevity...

    class WebComponent extends elementClass {

        // constructor() is used in v1 Custom Elements instead of
        // createdCallback() as in v0.
        constructor() {
            super()

            // some code omitted for brevity...

            this.createdCallback()
        }

        createdCallback() {
            // some code omitted for brevity...

            const observer = new MutationObserver(changes => {
                for (let change of changes) {
                    if (change.type != 'childList') continue

                    for (let node of change.addedNodes)
                        this.childConnectedCallback(node)

                    for (let node of change.removedNodes)
                        this.childDisconnectedCallback(node)
                }
            })
            observer.observe(this, { childList: true })
        }

        // Subclasses can implement these.
        childConnectedCallback(child) {}
        childDisconnectedCallback(child) {}

        // some code omitted for brevity...
    }

    // some code omitted for brevity...

    return WebComponent
}

Then, subclasses simply implement the childConnectedCallback and childDisconnectedCallback methods. But, as mentioned, timing issues happen because the observation does not happen in coordination with the connected/disconnected methods, forcing me in some situations to use promises and await to coordinate things in a somewhat ugly and possibly error-prone manner (in my subclasses).

@trusktr
Copy link
Contributor Author

trusktr commented Aug 24, 2016

Updated my previous comment, meant to say

timing issues happen because the observation does not happen in coordination with the connected/disconnected methods

@trusktr
Copy link
Contributor Author

trusktr commented Aug 24, 2016

IMO, it'd be nice that (if this feature existed) that childConnectedCallback and childDisconnectedCallback would be guaranteed to fire before the connectedCallback/disconnectedCallback of the children.

This would allow parents to set up certain things (for example pass a message or state to a child), then allow for the child to react to the message or state in connectedCallback without requiring async coordination or polling. Specifically, a child could detect the absence of a message or state in its connectedCallback in order to easily detect an invalid-parent state.

Similar with distributedCallback and detecting the invalid-slot-parent state.

Currently, my library just silently fails, which is not ideal, and I definitely don't want to introduce polling, and introducing async coordination with promises complicates things a ton, even if using await for cleaner syntax.

@trusktr
Copy link
Contributor Author

trusktr commented Sep 1, 2016

Idea from #559 (comment): callbacks for different types of changes: childUpgradedCallback, childConnectedCallback, childDisconnectedCallback, childAttributeChangedCallback, etc.

childUpgradedCallback could be especially helpful when elements that are already children of a custom element parent are upgraded in the future, as described in #559.

@caridy
Copy link

caridy commented Feb 20, 2018

I agree with @WebReflection here, MutationObserves is good enough for this very edge case. If you have many components with many children that need to be observed, you should probably look for alternative solutions, and question the viability of your current design when using WC APIs.

I don't think asking for child*Callback is realistic, specially because of its perf characteristics, which will probably force us to do something similar to the observedAttributes, which itself is very confusing and complicated.

@annevk
Copy link
Collaborator

annevk commented Mar 5, 2018

There's no interest in tackling callbacks for children alone. This would be considered if someone came up with an imperative API for slotting (note: thus far we haven't been able to do so).

@annevk annevk closed this as completed Mar 5, 2018
@robdodson
Copy link

robdodson commented Mar 14, 2018

I also just ran into this issue and put together an example. In my example, the MutationObserver only handles the case where children are added, hopefully it's self evident how to handle the removal case.

I process any existing children in my connectedCallback, and from then on rely on the MutationObserver for any changes.

Would the folks on this thread mind taking a look and let me know if there are any issues with this approach? https://stackblitz.com/edit/custom-element-mo?file=index.html

@calebdwilliams
Copy link

@robdodson That's more or less what I've started doing, but you need to disconnect on disconnectedCallback. So far I haven't had any issues.

@robdodson
Copy link

@calebdwilliams good point, I'll add that!

@franktopel
Copy link

@trusktr
Copy link
Contributor Author

trusktr commented Jan 28, 2019

parsedCallback is definitely useful for certain conditions, and fires only one time after the element is parsed. We can use it to look at children at the point in time when it is called, but we would still need to handle children added (or removed) at any point in the future, which parsedCallback doesn't cover.

By the way, at least for custom elements, here's an easy way to implement childConnectedCallback. Just make sure your custom element class calls super.connectedCallback().

What's nice is that this works regardless of parsing or code load order! During parsing, when a child connectedCallback fires, the parent will have already been upgraded and thus it will have the childConnectedCallback ready to be called, and we don't have to use any async deferral tricks/hacks, no MutationObserver.

(Any caveats I missed?)

@WebReflection
Copy link

we would still need to handle children added (or removed) at any point in the future

you just need MutationObserver for that

@trusktr
Copy link
Contributor Author

trusktr commented Jan 28, 2019

you just need MutationObserver for that

That's part of the problem because it introduces unwanted async behavior to the element and other complexities. The async nature of it means user code will not work unless they defer their logic too, so it can get messy. It's just not simple.

Simple would mean it is called synchronously like connectedCallback is (f.e. during parsing).

@WebReflection
Copy link

custom elements are async by specifications, indeed driven by callbacks, so I'm not sure what's the issue that you are trying to solve.

childConnected, in any case, doesn't solve the most important issue: when it is safe to setup the component. Until we have that, everything else looks superfluous to me 🤷‍♂️

@trusktr
Copy link
Contributor Author

trusktr commented Jan 28, 2019

Maybe "synchronous" isn't the right term, but from my experience, no macro tasks fire during parsing, and no animation frames either (at least during parsing of the body, though some seem to fire during parsing of the head). So once we're in parsing of the body (including custom elements), then it is almost like "synchronous" in the sense that there's no possible way to defer until after parsing except with setTimeout, so Promise.resolve() is not useful (this is in actual practice from all that I currently observe, not from reading the spec).

See, here's an example:

screen shot 2019-01-27 at 9 40 03 pm

(Note I am hovering on this.children.length and it is 0 although it does have children.

Thus because it is impossible to use microtask-based deferral to wait for children to be ready, and because MutationObserver will not fire during parsing, and because animation frames do not fire during this process, we have no choice but to use a macrotask (setTimeout) in order to defer to a future point in time when children will be ready.

And as an ugly side effect of this, anything in a <script> tag after the custom elements will have been evaluated before the next macrotask that we deferred to, thus the script-tag code also needs to use a macrotask deferral to fire code after children have been handled.

After doing the macrotask deferral in the custom element code, we then need to manually handle children once (because remember MutationObserver does not fire during parsing, and will never fire after parsing unless children have changed, which in most cases they haven't and we need to handle the existing children).

So, because of this, parsing is effectively "synchronous": connectedCallbacks all fire in preorder tree order during parsing "synchronously" because even the custom element author can not use microtask deferral to handle children, only macrotask deferral.

Macrotasks don't fire until after parsing (from my observations). So, you see:

screen shot 2019-01-27 at 10 05 00 pm

This is a huge pain.


In my custom elements, APIs and state are ready after both connectedCallbacks and childConnectedCallbacks have fired, only childConnectedCallback being reliable because at that point parentElement and children are both upgraded and thus parent and children can rely on each other's APIs.

So if end users of the APIs need to reply on APIs and state that only exist after both connectedCallbacks and childConnectedCallbacks of the custom elements have fired, then they need to introduce an unintuitive setTimeout call in their own code.

Do you see the problem now?


For example, suppose we have elements that create state in both connectedCallback and childConnectedCallback below. Then if initial calls to childConnectedCallback are async (due to using macrotask deferral and considering that MutationObserver is not an option during parsing), then we have to do the following to access all state properly as an end user of the elements:

<some-element>
  <other-element>
  </other-element>
</some-element>
<script>
  // `childConnectedCallback` has not fired yet. And guess what: `connectedCallback` has
  // to defer in order to manually fire `childConnectedCallback` in a future macrotask,
  // because guess what: `MutationObserver` does NOT fire on initially connected children!
  // So guess even more what! Because the elements must defer with a macrotask, then we
  // (speaking as the end user of the custom elements) need to to defer with a macrotask:

  const el = document.querySelector('some-element')

  console.log(el.someStateCreatedInChildConnectedCallback) // undefined

  Promise.resolve().then(() => {
    console.log(el.someStateCreatedInChildConnectedCallback) // STILL undefined, WAT?
  })

  setTimeout(() => {
    console.log(el.someStateCreatedInChildConnectedCallback) // FINALLY! Ugh.
  })
</script>

It's plain and simple: It'd be great to have a "synchronous" (in the sense I described above with regard to parsing) childConnectedCallback.

An easy way to implement it is with the monkey patching I linked to, or perhaps a class-factory mixin.

@trusktr
Copy link
Contributor Author

trusktr commented Jan 28, 2019

Apparently, because children can not be observed during parsing with Promise.resolve as I show above, and all macrotasks fire after parsing, this means that the parsing mechanism has some other sort of task which is between microtask and macrotasks. It seems as though the parser fires connectedCallbacks between each queue if microtask during parsing, and it is therefore impossible to defer with microtasks to detect children.

I don't know if this is part of spec though, but IMO definitely not ideal because it shows some behavior that isn't a normal part of what people expect from JavaScript.

@WebReflection
Copy link

I don't know if this is part of spec though, but IMO definitely not ideal because it shows some behavior that isn't a normal part of what people expect from JavaScript.

which is exactly my point since about ever: the most important piece of the puzzle to grant robust delivery of custom elements is not in the specifications and nobody knows, not even browser vendors, when an element can be trustfully initialized.

Yes, we can always add nodes after that, but the point is that nobody knows when it's even possible to do so.

We need to solve this part of the puzzle, or anything dealing with <div> and built-in extends would always feel like a better choice than this funny unknown that is custom elements upgrade.

As summary, we need an upgradedCallback or we can stop bothering with CE, IMO.

@rniwa
Copy link
Collaborator

rniwa commented Jan 29, 2019

As I've numerously stated in various threads & issues, the right approach is for the child to report its insertion to the parent. Waiting for all children to be parsed in order to initialize is never a good idiom because the HTML parser may pause for seconds at a time when there is a network delay.

We used to have code like that in our builtin element implementations and we still do in some cases (due to historical reasons) but almost all such code end up causing bugs. It's simply not the right pattern by which to write an element.

@trusktr
Copy link
Contributor Author

trusktr commented Jan 29, 2019

the right approach is for the child to report its insertion to the parent

That's basically the idea in the example of #785 (comment).

parser may pause for seconds at a time when there is a network delay.

Ah, that explains even more, so it's something stuck somewhere between microtasks and macrotasks.

It's simply not the right pattern by which to write an element.

It can be fine if the relationship between parent and child is part of the API. For example: select > option, tr > td, etc.

In my cases, I have one example where a parent lays out children, but the children need to be a certain type:

            <!-- lays out nodes using Apple-style VFL -->
            <i-autolayout-node
                id="layout"
                position="0 0 0"
                align=" 0.5 0.5 0"
                mount-point=" 0.5 0.5 0"
                visual-format="
                    V:|-[child1(child3)]-[child3]-|
                    V:|-[child2(child4)]-[child4]-|
                    V:[child5(child4)]-|
                    |-[child1(child2)]-[child2]-|
                    |-[child3(child4,child5)]-[child4]-[child5]-|
                "
                style="background: rgba(0,0,0,0.3)"
            >

                <i-dom-plane size="1 1 0" color="deeppink" class="child1">This is a paragraph of text to show that it reflows when the size of the layout changes size so that the awesomeness can be observed in its fullness.</i-dom-plane>
                <i-dom-plane size="1 1 0" color="deeppink" class="child2">This is a paragraph of text to show that it reflows when the size of the layout changes size so that the awesomeness can be observed in its fullness.</i-dom-plane>
                <i-dom-plane size="1 1 0" color="deeppink" class="child3">This is a paragraph of text to show that it reflows when the size of the layout changes size so that the awesomeness can be observed in its fullness.</i-dom-plane>
                <i-dom-plane size="1 1 0" color="deeppink" class="child4">This is a paragraph of text to show that it reflows when the size of the layout changes size so that the awesomeness can be observed in its fullness.</i-dom-plane>
                <i-dom-plane size="1 1 0" color="deeppink" class="child5">This is a paragraph of text to show that it reflows when the size of the layout changes size so that the awesomeness can be observed in its fullness.</i-dom-plane>

            </i-autolayout-node>

@trusktr
Copy link
Contributor Author

trusktr commented Jan 29, 2019

Another issue is, that children can not report to their parents if they are in a shadow root. However, the parent can easily either look at its direct children, or look at children in its root.

@trusktr
Copy link
Contributor Author

trusktr commented Jan 29, 2019

I'm thinking another approach can be to emit an event with composed:true so it goes up past the root. An issue with this is how to distinguish the event being emitted from a child in the light tree, or a child of the shadow root. Let me experiment with it...

@platosha
Copy link

@trusktr with events you could use the event.composedPath() result to distinguish between different event origins.

BTW, have you explored using the slotchange event for your use case? Any reasons against using slotchange?

@trusktr
Copy link
Contributor Author

trusktr commented Feb 1, 2019

I made a pen based on the MDN example:

https://codepen.io/trusktr/pen/66d328c9378e850f6419086d0624ead7

Anywho, turns out this is only good in Custom Elements, as we can at most enforce out own elements emit the events.

Originally I mentioned:

MutationObserver will not fire during parsing

@rniwa pointed out that if the MutationObserver is created in connectedCallback, then it doesn't fire for children during parsing. The observer has to be created in the constructor.

I wanted to create an observer in connectedCallback, then clean it up (.disconnect() it) in disconnectedCallback, so as to keep things cleaned up when done using them.

I eventually figured the pattern to conditionally create the observer either in the constructor or in connectedCallback as needed, while cleaning up in disconnectedCallback. Here's that example.

Then I asked about being able to check if (isParsing) {} or similar, which I haven't investigated yet to see if it is useful in making the previous example cleaner.

@WebReflection
Copy link

if the MutationObserver is created in connectedCallback, then it doesn't fire for children during parsing.

nobody said that is the place indeed and you should add the observer within the constructor.

The connected/disconnected can be either a flag or a dumb this.ownerDocument.contains(this) that is an evergreen synchronous check for connected nodes

@trusktr
Copy link
Contributor Author

trusktr commented Feb 2, 2019

this.ownerDocument.contains(this)

I just gave it a try, and it seems to return false on elements in a shadow root. @rniwa's isConnected polyfill traverses up to check shadow hosts.

@trusktr
Copy link
Contributor Author

trusktr commented Feb 2, 2019

nobody said that is the place indeed and you should add the observer within the constructor.

Yes, but connectedCallback and disconnectedCallback seemed like the right way to create it and reciprocally destroy it. It was obvious or intuitive that making an observer in constructor would work, while making one in connectedCallback wouldn't.

So in the example I'm making the observer from both constructor and connectedCallback depending on conditions, so that disconnectedCallback can clean it up and if the node is reconnected it can regain the functionality in connectedCallback instead of constructor.

This is all under the assumption that a MutationObserver should be cleaned up by calling its disconnect() method. Can we expect things to be garbage collected when the node is not used anymore, without calling observer.disconnect()?

@rniwa
Copy link
Collaborator

rniwa commented Feb 3, 2019

If a browser were to keep a MutationObserver around after all its observed nodes is no longer visible from JS, e.g. mark-and-sweep GC would not mark it, then that's simply a browser implementation bug.

@WebReflection
Copy link

WebReflection commented Feb 3, 2019

I just gave it a try, and it seems to return false on elements in a shadow root

Yeah, I don't use Shadow DOM much so I haven't thought about it. The @rniwa solution looks way better but I wonder if that should be a WHATWG proposal instead of just a comment in some discussion, 'cause it's very handy and easily badly replaced (see my suggestion, as example).

a MutationObserver should be cleaned up

I think as @rniwa suggested it'd be unnecessary.

Same goes for listeners, the constructor is the place to add these or remove these, while reaction can be different based on the node state.

Adding and removing too much on each connect/disconnect is not better, IMO, just slower and, in this case, simply over complicated for no gain.

@trusktr
Copy link
Contributor Author

trusktr commented Apr 12, 2019

Adding and removing too much on each connect/disconnect is not better, IMO, just slower and, in this case, simply over complicated for no gain.

Could be true sometimes, but in order to implement something like a childConnected/DisconnectedCallback feature which is inactive when the tree is not in a document (similar to connected/disconnectedCallback, it requires handling in connected/disconnected.

Well, maybe we can just return early is not is connected, but then the MO would be firing all the time still. Would that be better?

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

No branches or pull requests

9 participants