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

Disconnect single target instead of all #126

Open
zbraniecki opened this issue Dec 9, 2015 · 40 comments
Open

Disconnect single target instead of all #126

zbraniecki opened this issue Dec 9, 2015 · 40 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@zbraniecki
Copy link

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
Copy link
Member

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

@zbraniecki
Copy link
Author

@smaug---- @annevk - thoughts?

@smaug----
Copy link
Collaborator

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
Copy link
Contributor

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----
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link

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.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Aug 23, 2016
@annevk annevk marked this as a duplicate of #482 Jul 27, 2017
@trusktr
Copy link

trusktr commented Jul 28, 2017

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
Copy link
Member

annevk commented Mar 13, 2018

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

@justinfagnani
Copy link

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
Copy link

trusktr commented Mar 13, 2018

Is this still something that's needed?

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

@trusktr
Copy link

trusktr commented Mar 13, 2018

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
Copy link
Member

annevk commented Mar 17, 2018

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
Copy link

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
Copy link
Member

annevk commented Nov 13, 2018

@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
Copy link

trusktr commented Dec 6, 2018

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. 👍

@annevk annevk added the addition/proposal New features or enhancements label Apr 11, 2019
@ikeyan
Copy link

ikeyan commented May 21, 2019

@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
Copy link

ikeyan commented May 21, 2019

  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
Copy link

ikeyan commented May 21, 2019

  1. 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
Copy link

ikeyan commented May 22, 2019

  1. 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
Copy link

ikeyan commented May 22, 2019

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

@trusktr
Copy link

trusktr commented May 16, 2020

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 #126 (comment), 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
Copy link

ikeyan commented May 20, 2020

@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
Copy link

trusktr commented Oct 29, 2020

That seems simple! 👍

I think the _queue would be a Set too right?

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

@pengzhengyi
Copy link

pengzhengyi commented Apr 16, 2021

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
Copy link

trusktr commented Sep 10, 2021

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
Copy link

trusktr commented Sep 10, 2021

@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
Copy link
Member

annevk commented Sep 10, 2021

@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
Copy link

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----
Copy link
Collaborator

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, #126 (comment) is still a very valid comment here :)

@trusktr
Copy link

trusktr commented Sep 11, 2022

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, #126 (comment) is still a very valid comment here :)

It seems that the same behavior should apply then: drop the records and we're done. A takeRecords(element) call can precede that, if the end user so needs.

Do we all agree to move with making that change?

@annevk
Copy link
Member

annevk commented Sep 15, 2022

Sorry, but no, there's still no concrete proposal.

@RonaldZielaznicki
Copy link

RonaldZielaznicki commented Mar 30, 2023

I'll take a stab at that proposal.

Proposal

Use Cases

Changes

  • Add MutationObserver.prototype.unobserve(target, callback)
    • target: Node
    • callback: optional MutationCallback
    • stops new MutationRecords for target from queuing
    • removes any MutationRecords for target from queue
    • if callback defined, calls callback with MutationRecords removed from queue

Tried to keep in line with Resize.prototype.unobserve while balancing the need to do something with any MutationRecords in the current queue in a way that is intuitive to web developers.

@annevk
Copy link
Member

annevk commented Mar 31, 2023

@RonaldZielaznicki looks reasonable. Do we need the callback given takeRecords() exists?

@smaug----
Copy link
Collaborator

How does that proposal work with transient observers? Say, one has registered a node and observe changes to its subtrees and nodes in subtree moves around so there are transient observers. Are those transient observers removed? What if there are transient observers because of several different nodes passed earlier to observe(), which all transient observers are removed then?

@RonaldZielaznicki
Copy link

RonaldZielaznicki commented Mar 31, 2023

RonaldZielaznicki looks reasonable. Do we need the callback given takeRecords() exists?

@annevk We'd need the callback or some other method to remove the records for an individual element. Otherwise a developer wishing to utilize unobserve would need to handle the records for every other element that is observed when they remove a single element, which puts us back into the original issue with disconnect that unobserve is trying to solve.


@smaug----

How does that proposal work with transient observers?

Ideally, similar to a call to disconnect. But, disconnect's specification does not reference transient observers. I'm assuming it removes transient observers, but I'm relying on a comment you made back in 2015. So that could have changed.

Say, one has registered a node and observe changes to its subtrees and nodes in subtree moves around so there are transient observers. Are those transient observers removed?

Yes, we'd remove those transient observers.

What if there are transient observers because of several different nodes passed earlier to observe(), which all transient observers are removed then?

This question is confusing to me because it makes me think we've different understandings of the specification. I want to make sure our understanding aligns. So, please let me know if any of the following given assumptions are incorrect.

  • the transient registered observers are in a Node's registered observer list; rather than the MutationObserver 1
  • given each transient registered observer has a source registered observer2

Assuming the above is correct, this is how I'd imagine the logic would work:

  • If the transient registered observer's source is the MutationObserver from which unobserve was called, then we'd remove the transient registered observers from a Node's registered observer list

References:

  1. Registered Observer List - "Each node has a registered observer list (a list of zero or more registered observers), which is initially empty. "
  2. transient registered observer - "A transient registered observer is a registered observer that also consists of a source (a registered observer)."

@smaug----
Copy link
Collaborator

I was just asking about transient observers, not saying it wouldn't work. But I need to still think how the proposal would behave in that case.

  • removes any MutationRecords for target from queue

How does this work? Records may be there because of multiple targets. I mean something like this.

@RonaldZielaznicki
Copy link

@smaug----

I thought I answered that in my previous comment. Did you need me to expand upon it?

This bit specifically is what I thought answered your question.


What if there are transient observers because of several different nodes passed earlier to observe(), which all transient observers are removed then?

This question is confusing to me because it makes me think we've different understandings of the specification. I want to make sure our understanding aligns. So, please let me know if any of the following given assumptions are incorrect.

  • the transient registered observers are in a Node's registered observer list; rather than the MutationObserver
  • given each transient registered observer has a source registered observer

Assuming the above is correct, this is how I'd imagine the logic would work:

  • If the transient registered observer's source is the MutationObserver from which unobserve was called, then we'd remove the transient registered observers from a Node's registered observer list

@smaug----
Copy link
Collaborator

Did you check my example. How should that work? The source code has a question.

@scorbiclife
Copy link

scorbiclife commented Aug 24, 2023

Hi, I came to ask about what happens to the queue when you observe the same node again. Because I'd like to know the semantics of this which can be used now as far as I can see:

m.observe(e, { attribute: true, attributeFilter: [] });

Edit: Oh, the standard is actually much friendlier than I expected. So it has no effect on the mutation queue.

MDN Docs shows a usage of takeRecords. Likewise, would running takeRecords and setting a new dummy observer on the same node be a valid way to emulate unobserve or is there some edge case that I didn't think about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests