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

[discussion] Async nature of MutationObserver can cause room for error? #399

Closed
trusktr opened this issue Jan 16, 2017 · 4 comments
Closed

Comments

@trusktr
Copy link

trusktr commented Jan 16, 2017

A library that I'm working on creates components that contain HTMLElements. The user can run some code like this:

parent.removeChild(child)
parent.addChild(child)

Behind the scenes, the synchronous removeChild and addChild methods each trigger a MutationRecord to be recorded for the element referenced by parent. This means that at the next microtask parent element's MutationObserver's callback will be called with two MutationRecords: one for removedNodes and the other for addedNodes.

The problem is this: I wrote childConnectedCallback and childDisconnectedCallback methods for my components, and those methods are called by a MutationObserver whenever the observation callback sees items in addedNodes and removedNodes, respectively. In the above example, parent.removedChild(child) and parent.addChild(child) are synchronous in the same turn, so this causes two MutationRecords to be handled in a future microtick. So, after both methods are called, then childDisconnectedCallback and childConnectedCallback are fired for removedNodes and addedNodes, respectively, in the same order at the removeChild and addChild calls. Basically, the order of method calls is this:

// user code
parent.removeChild(child)
parent.addChild(child)

// library code triggered by MutationObserver callback
parent.childDisconnectedCallback(child) // for removedNodes
parent.childConnectedCallback(child) // for addedNodes

However, I wrote the childDisconnectedCallback and childConnectedCallback methods as if they were synchronous, not really considering the less common case when a child is removed and added back to the same parent in the same turn. It turns out that in my case my library gets into an error state due to the out-of-order calling of the methods.

Considering that MutationObserver callbacks are not synchronously fired, I can temporarily work around the problem by changing the user code to the following, which causes removedNodes and addedNodes MutationObserver callbacks to be interleaved with the removeChild and addChild calls respectively:

~async function() {
 parent.removeChild(child)
 await sleep(1000) // give time for MutationObserver callback to fire with only removeNodes
 parent.addChild(child)
 // A following MutationObserver callback fires with only addedNodes
}()

The method order is then:

// user code
parent.removeChild(child)
// library code triggered by MutationObserver callback
parent.childDisconnectedCallback(child) // for removedNodes

// user code
parent.addChild(child)
// library code triggered by a second MutationObserver callback
parent.childConnectedCallback(child) // for addedNodes

Do you get what I mean?

I suppose I am wondering if a synchronous API would be a nice alternative (f.e. a library author might not overlook async behavior like I did). One way in which there could be a synchronous API for this is with childConnectedCallback and childDisconnectedCallback for the Custom Element API.

@annevk
Copy link
Member

annevk commented Jan 16, 2017

The problem with a synchronous library is that you can then put the browser in a state of it not knowing what's going on. We used to have that and mutation observers are the replacement as it caused all kinds of problems.

With mutation observers the code that you write needs to be a little more involved. You get the changes from the API and you need to figure out if you just get the state from the tree instead or use the changesets in some way.

@trusktr
Copy link
Author

trusktr commented Jan 17, 2017

We used to have that and mutation observers are the replacement as it caused all kinds of problems.

I suppose you're talking about DOM mutation events?

If a feature like childConnectedCallback/childDisconnectedCallback triggers such methods in the same turn as connectedCallback/disconnectedCallback (respectively) of the child being connected only when those methods are defined, would that solve problems?

From the discussion that is linked in the MDN page, the DOM mutation event problems

(1) Verbose (fire too often)
(2) Slow (because of event propagation -- and because they prevent certain UA run-time optimizations)
(3) "Crashy" (have been the source of many crashers for UAs because script can tear up the world in any way it wishes while a DOM operation is underway).

could possibly be solved with the callbacks (as far as Custom Elements go), as in

(1) the callback methods only fire when they are defined
(2) no event propagation, they only fire on a parent custom element
(3) callbacks are called synchronously at the end of DOM operations.

The limitation of these new callbacks would be that they only exist on custom elements, so there's no way to arbitrarily watch some other node (f.e. a div element), and for that we can still use MutationObserver.

@annevk
Copy link
Member

annevk commented Jan 17, 2017

Yeah, notifying after the DOM operation completes is safe. Last time this came up though implementers were somewhat worried about performance if we gave the option to get after-operation notifications.

@annevk
Copy link
Member

annevk commented Jul 5, 2017

I'm going to close this. If you want to propose a synchronous API you could do so via a dedicated thread, though I don't think there would be a high chance of success. Custom elements is probably the way to go.

@annevk annevk closed this as completed Jul 5, 2017
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

2 participants