dom

dom copied to clipboard

What problem are you trying to solve?

Calling mutationObserver.disconnect() while forgetting to handle any pending records is a common footgun.

What solutions exist today?

Having to write boilerplate like:

let records = mutationObserver.takeRecords();
if (records.length > 0) {
	callback(records);
}
mutationObserver.disconnect();

Every time you want to stop observing is quite repetitive, and it cannot be done with just a reference to the mutation observer, since it requires a reference to its callback too.

How would you solve it?

Add a dictionary argument to disconnect() with a flush option (name TBB) that does exactly this.

Anything else?

No response

as one that has been using MO forever (polyfills included) I never even thought the leaky records would be a problem but I fully agree with this issue concerns and I also need to likely update all my libraries heavily based on MO to guarantee something not observed anymore won't be processed after a disconnect happens.

Thanks for considering this bug and raising its importance too, if needed.

Definitely interesting and I think it'd be nice to have this as a feature of MutationObserver...from a topical assessment it doesn't seem like this would break anything. I agree that takeRecords() is quite clunky and could use some modernization.

I'm open to implementing something or helping out with the specs if this issue ready for those!

I actually wonder if this might be something we could simply fix. Code that follows the pattern in my first comment won't break, it will simply take the records before disconnect() is called. And the rest is already broken — I'd wager it's highly unlikely that code depends on pending records not being taken.

If not, another solution is to introduce a different method, and deprecate disconnect(). Though this is a conventional name at this point that other observers share too…

@annevk What do you think?

I'm not sure. @smaug---- and @rniwa are probably better equipped to judge the impact of such a change.

Adding option to disconnect seems like a sensible approach. Changing the behavior of disconnect at this point is likely going to hit some web compatibility issues (this API has been shipping for 10+ years) so I'd rather not.

the rest is already broken

The rest is not already broken. disconnect() was designed to stop observing anything, very explicit. No different to removeEventListener. (There might be an event queued in a task, and you won't get that event if removeEventListener is called).

If we add a dictionary to disconenct() for flush, I wonder if we want also explicit flush() which is like takeRecords() but calls the callback.

@rniwa

Adding option to disconnect seems like a sensible approach. Changing the behavior of disconnect at this point is likely going to hit some web compatibility issues (this API has been shipping for 10+ years) so I'd rather not.

We can't keep adding API surface for every fix. How would sites be depending on the behavior of pending records not being taken?

Anyhow, if the compat risk is deemed too high I think a new method might be better, so we can gradually steer people towards using that, rather than being stuck with a method where you need to pass an optional parameter in 99.9% of the time (like cloneNode(true)).

@smaug----

The rest is not already broken. disconnect() was designed to stop observing anything, very explicit. No different to removeEventListener. (There might be an event queued in a task, and you won't get that event if removeEventListener is called).

How frequently does that happen though? It sounds like an extremely rare race condition, whereas what I’m talking about here happens way more.

If we add a dictionary to disconenct() for flush, I wonder if we want also explicit flush() which is like takeRecords() but calls the callback.

That seems like a good idea!

:+1: for .flush() and dare I say .detach(element) or .unobserve(node) would be a nice extra things to the MO API.

edit if there's any doubt around why the flush() is desirable, people just bootstrap MO with an arrow function, they don't necessarily make that a scoped reference, and once they do that, there's no way to trigger that explicitly so that takeRecords() is currently a libraries/frameworks thing to do only, not the norm for developers. flush() that automatically triggers the registered callback as in thatCallback(mo.takeRecords()) would be a super easy, clean, and cool way to reason about the API.

edit 2 dare I say flush() would make takeRecords() almost useless ... but there are places where explicit records to pre-process might be desired.

@LeaVerou what do you envision this addition looking like? Is it better to have redundant options (like giving developers the choice between passing in a dictionary to takeRecords() to automate this process when disconnect() gets called or calling flush() manually)? Or is this something should we only implement in one way?

@smaug---- Do you know why disconnect() was designed this way? I'm curious if there's any case where a developer might want to use takeRecords() with disconnect(), then later in the codebase call observe() and disconnect() without takeRecords() using the same MutationObserver object. Seems like an unlikely scenario but if there's a use case for doing this I think the redundancy described above might be the best way to go about this feature addition.

The proposed disconnect option would also be useful to me — and a flush() method even moreso, for the reasons @WebReflection described. Every time I’ve ever used takeRecords(), it was specifically to do what flush() would do, and its absence presently necessitates keeping a common function reference around for both scenarios, which has seemed like awkward boilerplate.

(And yep: I’d also love an unobserve-specific-node-only method — though I imagine that would probably merit its own issue.)