Disconnect single target instead of all

[zbraniecki]

Current specification of MutationObserver allows users to connect an observer to multiple target root nodes, but disconnect always acts on all of them.

It makes it hard to write systems that modify targets in reaction to mutations because they need to disconnect the target on which the mutation happened, modify, and reconnect.

A simple solution would be to add a single parameter to disconnect with targetNode.

[travisleithead]

This seems very reasonable to me. It would not be a burden implementation-wise.

[zbraniecki]

@smaug---- @annevk - thoughts?

[smaug----]

So disconnect() would do then what? Currently disconnect() clears the currently collected MutationRecords and it also removes transient observers. I don't quite understand what disconnect(someNode) would do to the record queue?

[ArkadiuszMichalski]

Why limited only to one target? This would be more useful disconnect(Node... nodes), so without passing argument act like what we have now (remove all registered observers), for one or more nodes check if they are exist in context object’s list of nodes and remove registered observer for that nodes. Record queue for both case probably should be cleared.

[smaug----]

So even if MutationObserver keeps observing nodeA after calling disconnect(nodeB), the records which were created because of observing nodeA, are just lost. Feels a bit odd to me, and error prone. One needs to remember to call takeRecords to not have stuff lost.

For the original use case would something like suppress(node)/unsuppress(node) work? It wouldn't affect to the queued records, but would just prevent creating new ones for the particular registered observer or relevant transient observers.

[ArkadiuszMichalski]

Right, clearing all queued records may be a problem (everything depends on the situation). Remembering to use MutationObserver.takeRecords() before disconnecting single node, if necessary, is inconvenient. But suppress(node)/unsuppress(node) looks interesting.

[justinfagnani]

To add a different use case, for which I don't think suppress would work, in the Custom Elements v1 polyfill, I want to be able to observe the main document and the roots of several disconnected DOM trees. If any of the observed roots are added to another, then I want to unobserve the root of the child subtree because the subtree is now being observed by the parent subtree's observer.

I would propose adding an unobserve() method to parallel observe(), rather than change disconnect().

As for the record queue, I'd still need records from the unobserved subtree, so would prefer they remain, but could do a takeRecords() if necessary.

[trusktr]

Record queue for both case probably should be cleared.

I think removing only the records for the unobserved nodes makes sense.

I would propose adding an unobserve() method to parallel observe(), rather than change disconnect().

That could work. As end user, I don't care if it is unobserve(node) or disconnect(node), as long as they work the same (stop observing the node, and any following observer callback will not contain records for that node). unobserve sounds better as a reciprocal to observe.

As for the record queue, I'd still need records from the unobserved subtree, so would prefer they remain

I think that is confusing. If we unobserve(target), no future records should be callbacked for target, to keep things simple.

If you still need records from the unobserved subtree, you can call unobserve(node) after you've gotten those records (f.e. in the next observer callback).

[annevk]

Is this still something that's needed? It would help to have a clear proposal (ideally in OP) to point implementers to.

[justinfagnani]

From the Web Components world, I wouldn't be surprised if there are a few use cases that come up from needing to observe multiple shadow roots or ancestor trees as they're being distributed to slots, or with multiple custom element instances observing their light DOM and unobserving on disconnection.

[trusktr]

Is this still something that's needed?

Yes, makes code cleaner, and leads to less resource usage.

[trusktr]

Right now I use a new MO on each element, with the same callback function, so that I can remove each observer as needed. Seems like I could save memory usage if it were only a single MO.

[annevk]

Reading through the thread again it seems the main problem is that it's still unclear what the semantics should be. E.g., @justinfagnani argues the records need to be kept and @trusktr argues just those for the node in question need to be removed (it's unclear to me how that'd work, we don't store sufficient information for that today).

If someone can figure out a proposal here in terms of concrete changes to the specification it might be easier to obtain implementer interest.

[nielsvanmidden]

This would be very useful. Let me give you an example I run into: Currently I'm building a web application with React and Redux. This application is able to run from both the server and the client. But since the server isn't supporting any browser APIs we use middleware. This middleware connects React components to JavaScript services (singletons) that enhance those components with the use of browser API's. This middleware is applied to the JavaScript client bundle only.

In one specific scenario we have components that dispatch a register action when componentDidMount() (React lifecycle API) is invoked. And whenever this action is dispatched it starts observing the particular node for mutations (subscribeNode()).

class ServiceCards {
  constructor() {
    this.subscribeNode = this.subscribeNode.bind(this);
    this.observer = new MutationObserver(mutationsToObserve);
  }

  subscribeNode(node) {
    node.addEventListener('transitionend', this.onTransitionEnd);
    this.observer.observe(node, mutationOptions);
  }

  onTransitionEnd(ev) {
    if (this.classList.contains('is-selected') && ev.propertyName === 'width') {
      this.classList.remove('is-selected');
    }
  }
}

Because React is in control when a component is either mounted or unmounted, disconnect doesn't mean so much to me. Also if a single component gets unmounted, I don't want the observer to disconnect. I'd rather dispatch an action that is able to unsubscribe a single node here.

[annevk]

@nielsvanmidden thank you, but this doesn't really address the questions from @smaug---- upthread (and the resulting discussion) as far as I can tell.

[trusktr]

Hope this becomes reality. unobserve(target) as reciprocal to observe(target) would be great, leaving disconnect as is. Calling unobserve(target) would cancel any future callbacks (and records) for that target, and leave future callbacks and records in place for other targets, as @justinfagnani proposed. 👍

[ikeyan]

@trusktr The problem is,

callbacks (and records) for that target

and

future callbacks and records in place for other targets

can have nodes in common. For example, if a node A is an ancestor of a node B like in this HTML,

<body>
<div> = A
  <ul>
    <li></li> = B
  </div>
</div>
</body>

and if you observe both A and B, how unobserve() should behave?

observer.observe(A, {subtree: true, childList: true});
observer.observe(B, {attributes: true});
// Now if someone add a class attribute to B,
// observer has a queued item {target: B, attributeName: "class"}
observer.unobserve(B);

I think three behaviors are mentioned in this issue.

[ikeyan]

1. disconnect() semantics: the method should discard all mutations in the queue (@ArkadiuszMichalski mentioned here)

• I think this is error prone and surprising behavior

[ikeyan]

2. unobserve() with queue filtering semantics: the method should behave as if there was no observe() call(s) on the node. (@trusktr mentioned here)

• mutations in the queue should be filtered
  • if no other observe() calls on other targets cover the mutation, remove it.
    • otherwise, leave as it is.
  • the problem is, mutations can change nodes' hierarchy, and thus the correct cancellation is actually impossible. (@annevk)

For example, consider this scenario.

• observer.observe(A, {subtree: true, attributes: true})
• observer.observe(B, {attributes: true})
• mutation queued: add a class attribute to node B
• (not observed) move node B from <ul> to <body>
• observer.unobserve(B)

The addition of class attribute to node B was covered by node A at that time, but not when unobserve() is called, and since node additions/removals were not observed, you cannot reconstruct the past hierarchy. So, if you implement the queue filtering, you can only use the current hierarchy (wrong result), or you have to remember the node hierarchy at every mutation just for this purpose (memory consuming).

[ikeyan]

3. unobserve() semantics without queue filtering: same as 3. but no queue filtering ( @justinfagnani mentioned here, @smaug---- mentioned here)

• mutations already in the queue are left as they are and are eventually processed by the callback function.
• since MutationObserver is asynchronous, I think this is acceptable and efficient solution.

[ikeyan]

So I think unobserve() semantics without queue filtering is the most promising behavior to implement.

[trusktr]

Wouldn't it be weird to if someone unobserves something, and then for some reason the callback still fires later? Wouldn't this cause confusion for end users of the API?

As @justinfagnani mentioned in https://github.com/whatwg/dom/issues/126#issuecomment-228455689, there could be a separate takeRecords() API for users that really need this.

For example,

observer.observe(node)

// ... then later ...

observer.takeRecords() // take the records that may already be queued
// ... do anything with records belonging to `node` ...
observer.unobserve(node) // will not fire a future callback with records that would've had target = node

observer.observe(A, {subtree: true, childList: true});
observer.observe(B, {attributes: true});
// Now if someone add a class attribute to B,
// observer has a queued item {target: B, attributeName: "class"}
observer.unobserve(B);

and if you observe both A and B, how unobserve() should behave?

@ikeyan The records of the attribute changes for B will not be included in the future callback. However, the records for the target A will still be included, even if those include B. It makes intuitive sense: that's how it would work if you never observed B in the first place.

The key here is that unobserve(B) will unobserve the specific behavior defined when we called observe(B). If B happens to be a node in the childList+subtree observation behavior of some another node A, then unobserving B should not have any impact on the observation defined on target node A with observe(A).

[ikeyan]

@trusktr So, the records should know which nodes they are for? Then, I also think it's a sound and simpler behavior.

// node A is a descendant of node B
observer.observe(A, {subtree: true, attributes: true});
observer.observe(B, {attributes: true});
// mutation queued: add a class attribute to node B
observer._queue = [{for: new Set([A, B]), message: "a class attribute added to B"}]
observer.unobserve(B);
// queue filtering. remove B from `for` attribute, and if `for` gets empty, the mutation record is thrown away
observer._queue = [{for: new Set([A]), message: "a class attribute added to B"}]

[trusktr]

That seems simple! :+1:

I think the _queue would be a Set too right?

API consistency between MutationObserver, ResizeObserver, IntersectionObserver, and any other Observer would be ideal.

[pengzhengyi]

I encountered an infinite-loop scenario from improperly using disconnect (assuming it works like unobserve for all targets in observer's node list)

1. receiving a mutation record at an observed target (inside MutationObserver's callback)
2. stop observing this target by calling disconnect (unexpected: the target still remains in the mutation observer's node list)
3. handle the mutation record (action here will manipulate the DOM and will trigger another mutation record if this target is still observed)
4. re-observing the previously observed target -- call observe on this target (the target now have two occurrences in the mutation observer's node list)
5. this already-handled-once mutation record is dispatched again to newly added target due to live editing of node list
6. infinite loop

My current fix is recreate a MutaionObserver and restore it to desired state whenever I need to re-observe a target.

I think I mistakenly used disconnect as unobserve for all, which might be another reason why a separate unobserve method is beneficial for inexperienced programmer like me.

[trusktr]

which might be another reason why a separate unobserve method is beneficial for inexperienced programmer like me.

That's very true, the unobserve idea simply makes intuitive sense: it is something any developer would assume (a construction API has a reciprocal desctruction API, and disconnect is not that so it isn't as intuitive especially considering the other API doesn't exist which may lead to some people thinking for a moment that disconnect might be the exactly-reciprocal option to use before discovering there isn't one).

[trusktr]

@annevk

Reading through the thread again it seems the main problem is that it's still unclear what the semantics should be

1. @ikeyan described the semantics regarding what happens to the records queue here, which seems very reasonable: namely using a Set to track which elements a record applies to, and removing that record only if the Set is empty.
2. Based on @ArkadiuszMichalski's comment, having unobserve(...nodes: Node[]) (using TypeScript syntax here) would be useful.
   • Would it be more worth accepting only a single arg for consistency with other *Observer APIs (f.e. ResizeObserver.unobserve)? Or would we be able to add var args to those other already-existing APIs?
3. @justinfagnani mentioned that the suggested sync takeRecords() method would be useful when we want to unobserve a particular node but still need access to the pending records before removing the node. I agree now because those lost records can result in effects unexpectedly not happening and leading to bugs (f.e. some mechanisms not being cleaned up upon node removal).
   • Based on the possibility of bugs due to effects that didn't run, the idea that unobserve() would stop queuing new records for the unobserved element but still fire the existing records in the upcoming task seems very reasonable. I.e. "stop observing the node now, but I still want to know what already happened before I stopped observing". After some thought, I now believe that this being default behavior would be ideal as it would lead to less bugs.
   • The takeRecords is one way to give people the choice on whether they want to observe the changes that already happened after they call unobserve. However, this means that the default behavior would be the potentially buggy removal of not-yet-handled observations after calling unobserve.
   • There are cases when we don't need to handle existing observations when we decide to unobserve: we might decide we don't care what will happen or what already happened to a node in cases where effects don't need to clean anything up or where effects don't create garbage that is hung and will leak.
   • Based on this, I think it would be better to default to the less-bug-prone behavior (which is that unobserve(node) stops observing for changes at the point the method is called, but still notifies of already-happened changes in the upcoming tick), and either
     • the user can call takeRecords() and do nothing with the result to opt into ignoring the already-happened observations,
     • or there can be an option that can be passed to unobserve, for example unobserve(node, {dropRecords: true}), but this would preclude the var args idea unobserve(...nodes) from being done in an ideal way.

What do other *Observer APIs currently do when unobserveing a single node?

[annevk]

@trusktr I recommend focusing on this:

If someone can figure out a proposal here in terms of concrete changes to the specification it might be easier to obtain implementer interest.

[samboylett]

I've been using this as a work around:

class MutationObserverUnobservable extends MutationObserver {
  private observerTargets: Array<{
    target: Node;
    options?: MutationObserverInit;
  }> = [];

  observe(target: Node, options?: MutationObserverInit): void {
    this.observerTargets.push({ target, options });

    return super.observe(target, options);
  }

  unobserve(target: Node): void {
    const newObserverTargets = this.observerTargets.filter(
      (ot) => ot.target !== target
    );
    this.observerTargets = [];
    this.disconnect();
    newObserverTargets.forEach((ot) => {
      this.observe(ot.target, ot.options);
    });
  }
}

I was pretty surprised the *Observer apis didn't all follow a similar interface. It would be nice to see unobserve in the spec :)

[smaug----]

That unobserve implementation loses all the queued records because of the disconnect() call. See https://dom.spec.whatwg.org/#dom-mutationobserver-disconnect step 2

Anyhow, https://github.com/whatwg/dom/issues/126#issuecomment-916703706 is still a very valid comment here :)