-
Notifications
You must be signed in to change notification settings - Fork 295
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
Comments
This seems very reasonable to me. It would not be a burden implementation-wise. |
@smaug---- @annevk - thoughts? |
So disconnect() would do then what? Currently disconnect() clears the currently collected MutationRecords and it also removes transient observers. |
Why limited only to one target? This would be more useful |
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. 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. |
Right, clearing all queued records may be a problem (everything depends on the situation). Remembering to use |
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 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. |
I think removing only the records for the unobserved nodes makes sense.
That could work. As end user, I don't care if it is
I think that is confusing. If we If you still need records from the unobserved subtree, you can call |
Is this still something that's needed? It would help to have a clear proposal (ideally in OP) to point implementers to. |
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. |
Yes, makes code cleaner, and leads to less resource usage. |
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. |
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. |
This would be very useful. Let me give you an example I run into: In one specific scenario we have components that dispatch a register action when
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. |
@nielsvanmidden thank you, but this doesn't really address the questions from @smaug---- upthread (and the resulting discussion) as far as I can tell. |
Hope this becomes reality. |
@trusktr The problem is,
and
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. |
|
For example, consider this scenario.
The addition of |
|
So I think unobserve() semantics without queue filtering is the most promising behavior to implement. |
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 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
@ikeyan The records of the attribute changes for The key here is that |
@trusktr // 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"}] |
That seems simple! 👍 I think the API consistency between |
I encountered an infinite-loop scenario from improperly using
My current fix is recreate a I think I mistakenly used |
That's very true, the |
What do other |
@trusktr I recommend focusing on this:
|
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 :) |
That unobserve implementation loses all the queued records because of the disconnect() call. 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 Do we all agree to move with making that change? |
Sorry, but no, there's still no concrete proposal. |
I'll take a stab at that proposal. ProposalUse Cases
Changes
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. |
@RonaldZielaznicki looks reasonable. Do we need the callback given |
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? |
@annevk We'd need the callback or some other method to remove the records for an individual element. Otherwise a developer wishing to utilize
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.
Yes, we'd remove those transient observers.
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.
Assuming the above is correct, this is how I'd imagine the logic would work:
References:
|
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.
How does this work? Records may be there because of multiple targets. I mean something like this. |
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.
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.
Assuming the above is correct, this is how I'd imagine the logic would work:
|
Did you check my example. How should that work? The source code has a question. |
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 |
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.The text was updated successfully, but these errors were encountered: