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

MutationObserver callbacks happen in bizarre order. #1105

Closed
trusktr opened this issue Aug 26, 2022 · 9 comments
Closed

MutationObserver callbacks happen in bizarre order. #1105

trusktr opened this issue Aug 26, 2022 · 9 comments

Comments

@trusktr
Copy link

trusktr commented Aug 26, 2022

Trying to port from DOM Mutation Events, or from parent dis/connectedCallbacks to a MutationObserver pattern that initializes or cleans up code based on when children are connected then disconnected, is nearly impossible without immense effort.

I've been using MutationObserver for years now, and am discovering bugs in certain cases that I didn't notice before (I should have written better tests! But also MutationObserver is really complicated and cumbersome).

The Problem

In the following example, the connected and disconnected events fire in the wrong order, which leave connected children erroneously cleaned up:

https://codepen.io/trusktr/pen/oNqrNXE/3aad3bb7315877d00c7c42e3d77fed5a?editors=1010

In this one, the behavior is a bit different, but still not as desired:

https://codepen.io/trusktr/pen/MWVMWKe/091e6f303bd773f2754304fb1c9bff30?editors=1000

With standard connectedCallback and disconnectedCallback, they always fire in this order per each custom element when an element disconnected and re-connected:

disconnectedCallback()
connectedCallback()

However, with this naive MutationObserver usage, the order (in the above two examples) when a child is removed then re-connected is

child connected
child removed

which is obviously going to cause unexpected behavior.


DOM Mutation Events are much much easier to use. MutationObserver is incredibly difficult to work with. (wrt observing children)

Possible Solutions:

  1. I believe that all the performance issues of Mutation Events could be fixed,
  2. Or a new synchronous alternative to Mutation Events could be created that would be no worse than today's connectedCallback and disconnectedCallback methods, which happen to be synchronous (synchronous is simple, and people can opt into deferred batching themselves when it really matters).

There's nothing about an event pattern that makes it inherently bad. It just happens to be that the way Mutation Events worked was not ideal and browsers implemented them differently with bugs. That's not to say we can't come up with an event pattern that works well.

@trusktr
Copy link
Author

trusktr commented Aug 27, 2022

Userland Solution

Currently, MOs run all at once per target, which is highly irrepresentative of the order in which mutations happened in the DOM.

The end user has two options, both of which get more complicated:

  1. Use a single MutationObserver root node of the current tree.
  2. Implement some additional tracking across all MutationObservers on individual targets

Here is an example of option 2 in userland:

https://codepen.io/trusktr/pen/ExEBxmw/401b4869c60721e0fca18840036cbdee?editors=0010

Possible Ideal Solutions (in the engine)

1 MutationObserver

I think the ideal solution would be for the browser engine to intelligently split MO changesets into chunks, and call them in the correct order (by calling MO callbacks for a single target one or more times) within the MO microtask, such that we can guarantee that each for loop in any callback is always iterating in the correct order relative to the whole tree in which the MutationObservers are observing.

This would eliminate the need for end users to write the complicated code required for the userland solutions.

2 Mutation Events

Fix DOM Mutation Events (make a new one with different names perhaps, or a new API that is similar to events).


Finally, compare how drastically complicated this MO code,

const connected = new Set 
const disconnected = new Set 
let scheduled = false

class XEl extends HTMLElement {
	connectedCallback() {
		this.o = new MutationObserver(changes => {
			for (const change of changes) {
				console.log('--- change', change.target)
				for (const child of change.addedNodes) {
					console.log('track added child', child)
					connected.add(child)
				}
				for (const child of change.removedNodes) {
					console.log('track removed child', child)
					disconnected.add(child)
				}
			}

			if (!scheduled) {
				scheduled = true
				queueMicrotask(() => {
					console.log('--------------- MICROTASK MO')
					scheduled = false
					const allNodes = new Set([...connected, ...disconnected])
					for (const child of allNodes) {
						if (child.parentElement) {
							if (disconnected.has(child)) console.log('child removed:', child)
							if (connected.has(child)) console.log('child added:', child)
						} else {
							if (connected.has(child)) console.log('child added:', child)
							if (disconnected.has(child)) console.log('child removed:', child)
						}
					}
					connected.clear()
					disconnected.clear()
				})
			}
		})

		this.o.observe(this, {childList: true})
	}

	disconnectedCallback() {
		this.o.disconnect()
	}
}

customElements.define('x-el', XEl)

setTimeout(() => {
	queueMicrotask(() => console.log('--------------- MICROTASK before'))
	const t = three
	t.remove()
	one.append(t)
	queueMicrotask(() => console.log('--------------- MICROTASK after'))
}, 1000)

is compared to the following DOM Mutation Events code:

https://codepen.io/trusktr/pen/ExEBxdE/586de910e2a61470aa96a00edfef9ccf?editors=0010

class XEl extends HTMLElement {	
	lastConnected = new Set
	
	childConnected = (event) => {
		if (event.target.parentElement !== this) return
		this.lastConnected.add(event.target)
		console.log('child added:', this, event.target)
	}

	childDisconnected = (event) => {
		if (!this.lastConnected.has(event.target)) return
		this.lastConnected.delete(event.target)
		console.log('child removed:', this, event.target)
	}

	connectedCallback() {
		for (const child of this.children) this.lastConnected.add(child)

		this.addEventListener('DOMNodeInserted', this.childConnected)
		this.addEventListener('DOMNodeRemoved', this.childDisconnected)
	}

	disconnectedCallback() {
		this.removeEventListener('DOMNodeInserted', this.childConnected)
		this.removeEventListener('DOMNodeRemoved', this.childDisconnected)
	}
}

customElements.define('x-el', XEl)

setTimeout(() => {
	queueMicrotask(() => console.log('--------------- MICROTASK before'))
	const t = three
	t.remove()
	one.append(t)
	queueMicrotask(() => console.log('--------------- MICROTASK after'))
}, 1000)

And that's not even simple enough.

The MutationObserver API is too complicated. We could have made something with a much simpler API like Mutation Events, while still making the API deferred-batched to run reactions in a microtask.

In fact we still can, make a better API, and we can deprecate MutationObserver. It is really error prone.

@trusktr
Copy link
Author

trusktr commented Aug 27, 2022

It gets more complicated:

My userland MutationObserver solution still has a big problem actually!

The current solution does not track targets, so if someone synchronously disconnects then reconnects a node to multiple parents, the solution currently does no know which element to call a method on (f.e. target.childConnectedCallback(child)). That current solution only ensures the order is correct.



I'm hoping to show just how complicated MutationObserver is, namely with regards to adding/removing nodes.

With regards to one-off events, for example any time text content or attributes change where we only care about the current value, maybe the last value (but not in which order nodes were added and removed and to whom the nodes finally belong), the API is fine and very sequential.

ResizeObserver and IntersectionObserver are both fine, because they represent values of time.

It is really childList that is the issue here, trying to ensure that initialization and cleanup logic is in the correct order.

Even the slotchange event API is much simpler, just like DOM Mutation Events are.



You may ask: "why don't you just use dis/connectedCallback?" The answer is that <div> tags don't have those callbacks, for example. One cannot simply replace all native elements with custom elements.

The following is not possible, as much as I wish it was:

div.connectedCallback = () => {...}
// or
const original = HTMLDivElement.prototype.connectedCallback
HTMLDivElement.prototype.connectedCallback = function() {
  original.call(this)
  // ...
}

It doesn't work because browser doesn't read those methods off of instances like idiomatic JavaScript would, even custom elements:

customElements.define('x-x', class extends HTMLElement {})
const x = document.createElement('x-x')
x.connectedCallback = () => console.log('connected')
document.body.append(x) // does not run connectedCallback

@trusktr
Copy link
Author

trusktr commented Aug 27, 2022

Maybe there's actually a bug in the browser implementation (or the spec)? Regarding these two lines:

	t.remove() // queue mutation for two
	one.append(t) // queue mutation for one

Shouldn't the reaction for #two run before the reaction for #one, because the t.remove() call modifies the children of #two so at that point the MO for #two should be queued. Then the one.append(t) line should queue the MO for #one.

So why, in this case, is #two's MO firing after #one's?


Regardless of that, fixing that still won't fix the overall issue. For example, suppose the nodes gets moved around to several parents, as in this example:

https://codepen.io/trusktr/pen/OJveVbv/d7f4cc48f8fa3b7462ae8043157ba05d?editors=0010

As you can see in the output, all reactions for a #one ran first. Then all reactions for #two ran next. They were not interleaved like dis/connectedCallbacks would be.

What I suggested above is that all reactions should be grouped such that, when naively iterating on them in the callbacks, all reactions will be iterated in the order in which they happened, across all MOs. In that example, it would mean that #one's MO callbacks and #two's MO callbacks would alternate executing, with a up to two change records (add/remove child) passed to each callback execution.

@trusktr
Copy link
Author

trusktr commented Aug 27, 2022

The most difficult question to answer using MutationObserver is:

Which parent was this child remove from?

As soon as there are more than two or more parents with MOs, which causes reactions to not be interleaved in the same order as the code that modified the DOM, it is impossible to answer this question (unless we move to a single root node MO approach).

Or if there's a bug, maybe we should be able to rely on the first MO that runs being the source of truth for a removed node's parent.

Without being able to determine this one piece of information, it is impossible to make a choice on which parent to run cleanup code on before running initialization code on a child's new parent.


But that only solved half of the issue:

If all reactions across all MOs of a batch of MOs were interleaved in the same order as DOM manipulations, not only would the answer be simple (the very first removedNodes reaction has the initial parent the child has been remove from), but all other code in between would be correctly interleaved, producing expected results for all reactions.

@trusktr
Copy link
Author

trusktr commented Aug 27, 2022

Firefox also runs one's MO before two.

@annevk
Copy link
Member

annevk commented Aug 29, 2022

I'm sorry @trusktr but this is not actionable. Usage questions should go to Stack Overflow. If you think you have identified a bug in the standard please file an issue with a succinct description of the problem.

@annevk annevk closed this as completed Aug 29, 2022
@WebReflection
Copy link

@trusktr the solution is to loop removedNodes before looping addedNodes and everything is fine. I have element-notifier reliably working like this forever (including polyfills for Custom Elements)

@trusktr
Copy link
Author

trusktr commented Aug 30, 2022

@annevk seems like a bug in the spec, because browsers behave the same, or browsers decidedly not following the spec in the exact same way.

The examples above clearly show the problem.

@WebReflection

the solution is to loop removedNodes before looping addedNodes and everything is fine

That doesn't work when you also need to tell a parent element which child was disconnected as a net result, and in that case requires net result counting too (not just reordering my reactions in the above examples).

If you do the following, synchronously, where all parents are custom elements with MOs.

  • remove a from parent A
  • add a to parent B
  • remove a from parent B
  • add a to parent C

Then the net result is I want to tell A that a was removed, and tell C that I added a, but the problem is that the something like following order of MutationObserver reactions can happen:

  1. MO for B runs, a was both added and removed, net result is do nothing
  2. MO for C runs, Net result is a added to C, run child connected logic for C
  3. MO for A runs, Net result is a removed from A, run child disconnected logic for A

The error is that 3 needs to run before 2 for the result to be correct.

Now, you mentioned simply running all removed reactions after added actions.

By counting all the addeds and removeds, in the end we are left with either one of these:

  • net result is removed from one element, added to another
  • net result is added to an element (no removeds)

This is possible to implement by using a counter for each MO on each custom element, then ignoring all net reactions, and finally, always running removeds before addeds like you suggested.

But! This means further code complication, and it will break:

  • now suppose we remove the parent elements also, in some arbitrary order,
  • and imagine we connect some child elements to them before reconnecting parents to other parents.
  • The MOs will have been disconnected during that time and will not track child connections, and new MO will be created on reconnect of parents after they have new children.

This means we must now also run MO-alternative logic in custom element connectedCallbacks to handle those added/removed child cases, and the code will get really ugly really fast as you can tell from what I'm describing already being a mess to understand, even if we can actually do it.


The main idea here is that, it would be amazing if there were a synchronous element observation API similar to Mutation Events, but without its problems.

I'm really only pointing out above that MOs are extremely over complicated when it comes to observing connected and disconnected children.

@trusktr trusktr changed the title MutationObserver callbacks happen in wrong order. MutationObserver callbacks happen in bizarre order. Aug 30, 2022
@trusktr
Copy link
Author

trusktr commented Aug 30, 2022

please file an issue with a succinct description of the problem

@annevk did you even look at the first two examples in the OP? Those are succinct and clearly show the problem. I'm not asking for usage help. I'm describing a problem with how MO works.

Can you please show some effort from your side towards the effort I put into making and describing those problematic examples, rather than seemingly glaze over and close the issue?

I desire a level of respect from people who manage the APIs I have to live with when I've in fact put effort into describing real-world problems with APIs you are supposed to curate and improve.

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

No branches or pull requests

3 participants