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

Element.prototype.connectedCallback, et al. #629

Closed
trusktr opened this issue Mar 10, 2017 · 6 comments
Closed

Element.prototype.connectedCallback, et al. #629

trusktr opened this issue Mar 10, 2017 · 6 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Mar 10, 2017

Based on the extensible web manifesto, it seems that the following should work, but it doesn't:

Element.prototype.connectedCallback = function() {

  console.log('connected', this.tagName)

}

document.body.innerHTML = "<div><h1>hello</h1></div>"

// expected output:
// connected DIV
// connected H1

(fiddle).

and similarly with disconnectedCallback, attributeChangedCallback, etc.

Shouldn't we be able to do stuff like that, assuming APIs like the Custom Elements API help to explain native elements?

It'd be nice for library authors to be able to hook into the engine like that.

The current alternative way to hook into the engine like that is to use MutationObserver on all elements in the page, but the above solution

  • is much cleaner,
  • performs better due to creation of a single function rather than who-knows-how-many MutationObservers,
  • behaves in a more predictable fashion because it is synchronous rather than asynchronous,
  • works across ShadowDOM trees (it will be a pain, or impossible, to do it with MutationObservers),
  • is simply easier to work with than MutationObservers,
@trusktr trusktr changed the title Why doesn't Element.prototype.connectedCallback work? Element.prototype.connectedCallback Mar 10, 2017
@trusktr trusktr changed the title Element.prototype.connectedCallback Element.prototype.connectedCallback, et al. Mar 10, 2017
@trusktr
Copy link
Contributor Author

trusktr commented Mar 11, 2017

  • and is less error-prone.
    • For example, trying to create childConnectedCallback/childDisconnectedCallback is problematic with MutationObserver because MutationObserver is async.
      This means that if an element is added and removed to/from a few different parents, the MutationObserver will enqueue a bunch of addedNodes and removedNodes events for the next mutation callback for the same element.
      The end developer is likely to loop through all the addedNodes at once, followed by looping through all the removedNodes at once, which will obviously lead to the handling of these events in the wrong order.
      This further means that the element's parentNode (for example) will also not match with the what was actually the case during the time the events were recorded.
      These problems would not arise if we had synchronous mechanisms like Element.prototype.connectedCallback (or other similar synchronous APIs I may have mentioned in other issues).
      Implementing childConnectedCallback/childDisconnectedCallback on top of something like Element.prototype.connectedCallback would be simpler and less error-prone than with MutationObserver (it might be considered "monkey patching", but I think it's fine for a library author, if done with care).

@trusktr
Copy link
Contributor Author

trusktr commented Mar 11, 2017

This is how my base class currently implements childConnectedCallback/childDisconnectedCallback :

// inside the base class:
        observeChildren(this, this.childConnectedCallback, this.childDisconnectedCallback)

// outside the base class:

// NOTE: If a child is disconnected then connected to the same parent in the
// same turn, then the onConnect and onDisconnect callbacks won't be called
// because the DOM tree will be back in the exact state as before.
function observeChildren(ctx, onConnect, onDisconnect) {

    const observer = new MutationObserver(changes => {
        const weights = new Map

        for (const change of changes) {
            if (change.type != 'childList') continue

            for (const addedNode of change.addedNodes)
                weights.set(addedNode, (weights.get(addedNode) || 0) + 1)

            for (const removedNode of change.removedNodes)
                weights.set(removedNode, (weights.get(removedNode) || 0) - 1)
        }

        for (const [node, weight] of weights)
            if (weight > 0)
                onConnect.call(ctx, node)
            else if (weight < 0)
                onDisconnect.call(ctx, node)
    })

    observer.observe(ctx, { childList: true })
    return observer
}

@trusktr
Copy link
Contributor Author

trusktr commented Mar 11, 2017

If we're dealing only with custom elements, then putting connectedCallback on Element.prototype will work if the custom element calls super.connectedCallback.This means something like childConnectedCallback/childDisconnectedCallback can be implemented synchronously when the tree is entirely custom elements.

However, it won't work when the tree is mixed with native elements, because those never get connectedCallback/disconnectedCallback methods called.

There's probably other use cases for this.

Overall, the main point is that if those methods were called on native elements, it would be more inline with the Extensible Web Manifesto, it would explain native elements better, and it would mean that the above "monkey patching" works because the new APIs explain existing elements, as in "native elements are implemented with things like connectedCallbacks, and by extension I can modify the ones that are on their prototypes".

@trusktr
Copy link
Contributor Author

trusktr commented Mar 12, 2017

Here's childConnectedCallback implemented on Element.prototype.connectedCallback (compare with the above MutationObserver implementation):

Element.prototype.connectedCallback = function() {
  if (this.parentElement && this.parentElement.childConnectedCallback)
    this.parentElement.childConnectedCallback(this)
}

This is much simpler!

Also (at least in Chrome), the order of MutationRecords do not always fire in the same order as the things that trigger them. This sort of problem is completely avoided with the Element.prototype.connectedCallback implementation.

For reference, I discovered that using a single global MutationObserver rather than one per element fixes the problem of MutationRecord callbacks happening out of order. After changing my MutationObserver implementation to the following I solved a bug caused by out-of-order callbacks:

let childObservationHandlers = null
let childObserver = null
function observeChildren(ctx, onConnect, onDisconnect) {
    if (!childObservationHandlers) childObservationHandlers = new Map
    if (!childObserver) childObserver = createChildObserver()
    childObservationHandlers.set(ctx, {onConnect, onDisconnect})
    childObserver.observe(ctx, { childList: true })
    return true
}
function createChildObserver() {
    return new MutationObserver(async (changes) => {
        const weightsPerTarget = new Map

        console.log('changes', changes, performance.now())

        for (const change of changes) {
            if (change.type != 'childList') continue

            if (!weightsPerTarget.has(change.target))
                weightsPerTarget.set(change.target, new Map)

            const weights = weightsPerTarget.get(change.target)

            for (const addedNode of change.addedNodes)
                weights.set(addedNode, (weights.get(addedNode) || 0) + 1)

            for (const removedNode of change.removedNodes)
                weights.set(removedNode, (weights.get(removedNode) || 0) - 1)
        }

        for (const [target, weights] of weightsPerTarget) {
            const {onConnect, onDisconnect} = childObservationHandlers.get(target)

            for (const [node, weight] of weights) {
                if (weight > 0 && typeof onConnect == 'function')
                    onConnect.call(target, node)
                else if (weight < 0 && typeof onDisconnect == 'function')
                    onDisconnect.call(target, node)
            }
        }
    })
}

And... now it is even more complicated than the 3-line Element.prototype.connectedCallback version.

@rniwa
Copy link
Collaborator

rniwa commented Mar 14, 2017

I made that point during one of F2Fs and we specially reached a consensus not to do this.

@domenic
Copy link
Collaborator

domenic commented Aug 16, 2017

Closing per consensus.

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

4 participants