Add hook to cleanData to allow cleanup

[T.J. Crowder <[email protected]…>]

Just as jQuery itself needs to know when an element is cleaned up, plug-in code (and perhaps application/page code) sometimes needs this notification as well. The TinyMCE jQuery plug-in, for instance, monkey-patches jQuery to hook into empty, remove, etc. so it can clean up after itself. I was just doing a plug-in where each instance needs to respond to window resize, and try as I might, I can't come up with a way to do it that won't leave handler(s) lying around if the programmer using it doesn't call my "destroy" method -- and let's face it, that's pretty easy to fail to do.

Recommend adding a hook or event that plug-ins (and application/page code) can use, with the clearly-documented caveat that almost any other option should be used first because of the potential performance implications.

The performance cost in the normal case (when no one has hooked the event) looks trivially small: Looking at cleanData in the current dev source, it looks like it comes down to one extra property test just after ​line 742 in manipulation.js (that line number will rot), e.g.:

742: if ( data && data.events ) {
New:   if ( data.events.destroy) {
New:     jQuery(elem).trigger("destroy"); // Or a more efficient way, of course
New:   }

...which is pretty cheap. A quick test on my 2.4GHz machine with Firefox on 10,000 elements, 5,000 of which had data.events and one of which had data.events.destroy, the average time to clear the container was 124-126ms both with and without the above. The cost when someone has actually hooked the event, of course, will be higher but largely irrelevant; if they've hooked it, presumably they have a reason.

This would, of course, be a somewhat unusual event. My off-the-cuff is that it wouldn't bubble, and couldn't be cancelled. But there are other unusual events. :-)

I'm happy to get into the details of this and submit a pull request (with tests and documentation) if this suggestion has legs.

[scottgonzalez]

We should avoid adding more custom event types to jQuery. Using a callbacks list would be much better. FWIW, duck punching $.cleanData is really easy and we do this in jQuery UI.

[dmethvin]

You should be able to achieve the cleanup today with the special events API. If there is an event attached to the element with a teardown handler, that handler would be called by cleanData. This also doesn't bubble, which is good IMO.

​http://jsfiddle.net/WX69j/

$.event.special.sanitation = {
   teardown: function(){
      alert("I'm being repressed!");
   }
};
$("span").on("sanitation", $.noop);
$("div").html("");

Would that do the trick for this use case?

[T.J. Crowder <[email protected]…>]

Replying to scott.gonzalez:

We should avoid adding more custom event types to jQuery. Using a callbacks list would be much better.

I'm happy with it being a callback rather than an event, provided it's easy to hook and gets fired. I'm not seeing any real issue with a "destroy" event, though, provided we make sure it doesn't bubble. But a non-bubbling event with no default action sounds a lot like a callback. :-)

FWIW, duck punching $.cleanData is really easy and we do this in jQuery UI.

Yes, but in addition to being overkill (if you want the notification, you have to intercept every call to several API points), it's just sub-optimal IMV to be modifying the library rather than using it.

[scottgonzalez]

I'm not seeing any real issue with a "destroy" event...

The problem is that it will cause problems if the W3C ever decides to add a destroy event.

[T.J. Crowder <[email protected]…>]

Replying to scott.gonzalez:

The problem is that it will cause problems if the W3C ever decides to add a destroy event.

destroy was just an example. It can be jqdestroy or jqclean or whatever to avoid conflicts. (Similarly ready really should get renamed at some point...)

See also below (six lines of code), I lean toward an event but I'd be interested what an equivalent callback mechanism would look like.

Replying to dmethvin:

You should be able to achieve the cleanup today with the special events API.

First off: That's way cool!

​http://jsfiddle.net/WX69j/

$.event.special.sanitation = {
   teardown: function(){
      alert("I'm being repressed!");
   }
};

Would that do the trick for this use case?

It would, but then my plugin has to create its own special event (and it can only have one handler, unless you implement a callback list); jQuery UI has to create their own special event; TinyMCE has to implement their own special event... It just starts getting messy IMV.

The nice thing about using an event is it causes near-zero bloat, and it uses a familiar and easy paradigm. It inherits namespacing and all sorts of goodness.

An off-the-cuff, but functionally complete, implementation adds six lines of code (I haven't checked yet if there's a more efficient way to trigger the event): The three I posted earlier:

if ( data.events.destroy ) {
  jQuery(elem).trigger("destroy");
}

...and these when initializing jQuery.event.special:

special: {
  noBubble: true
}

Trivial implementation, virtually no overhead, and a paradigm any jQuery programmer can trivially understand.

[Jock Murphy <[email protected]…>]

Allow me to just say "here, here!" and voice my support for this, I have more than once felt the need for such a thing, and wanted a more automatic way to cleanup. The proposed solution in the message above seems ideal to me.

[T.J. Crowder <[email protected]…>]

​Here's a copy of 1.8rc1 with the six-line patch above (I'm not saying that implementation is the be-all, end-all), and ​here's a fiddle that generates 10,000 elements, 5,000 of which we hook the click event on (so that they have something in data.events), and one of which we hook the jqdestroy event on. Then we call html on the container element to destroy them all and time how long it takes, as well as watching to see that we get the jqdestroy event (and that the container doesn't). You can compare that with ​this version using an unmodified version of 1.8rc1, if you want to look at overheads.

[dmethvin]

FWIW, duck punching $.cleanData is really easy and we do this in jQuery UI.

However, it's not a public interface. I can't imagine that we could refactor to do without it, but then again I thought we'd always be saddled with $.attrFn too.

jQuery(elem).trigger("destroy");

We could use .triggerHandler which doesn't bubble.

The nice thing about using an event is it causes near-zero bloat, and it uses a familiar and easy paradigm. It inherits namespacing and all sorts of goodness.

It doesn't bloat the code but it's a pretty large hammer if all you need is a non-bubbling callback. Follow the codepath and you'll see what I mean. So much of that code is involved in implementing DOM semantics. I agree that web developers understand those semantics, but they're not really needed here.

The performance cost in the normal case (when no one has hooked the event) looks trivially small

Most likely because it's already horribly inefficient right now. :)

One of the things I wanted to look at in 2.0 was the possibility of eliminating jQuery.cache and attaching data directly to a property of each DOM element. On removal, if the subtree didn't have any special event remove/teardown handlers we could simply throw it away without walking it at all. So any feature that obligates us to walk the entire subtree seems like a bad idea right now until it't proven we can't shortcut that.

Really, $.cleanData is bad for the same reason that the DOMSubtreeModified event is bad and was abandoned in favor of MutationObserver. Perhaps it would be better to see if we (or a third party) could build the DOM cleanup functionality on top of that with a shim for when it's not available.

[T.J. Crowder <[email protected]…>]

Replying to dmethvin:

FWIW, duck punching $.cleanData is really easy and we do this in jQuery UI.

However, it's not a public interface.

Exactly.

jQuery(elem).trigger("destroy");

We could use .triggerHandler which doesn't bubble.

Great, we're down to three lines! :-)

Seriously, I knew there was a more efficient way. There may be even further one could go, although efficiency firing the event is less important IMV than efficiency when not firing it. Firing it should be the unusual case.

The performance cost in the normal case (when no one has hooked the event) looks trivially small

Most likely because it's already horribly inefficient right now. :)

Perhaps, but mostly it's trivial because in the vast majority of situations, it's not adding any overhead at all; in the sitaution where an element has events hooked on it, it's adding just one "does the property exist" check, which is very, very quick.

One of the things I wanted to look at in 2.0 was the possibility of eliminating jQuery.cache and attaching data directly to a property of each DOM element. On removal, if the subtree didn't have any special event remove/teardown handlers we could simply throw it away without walking it at all.

Well, if there are any special events with that behavior, this doesn't change anything; it just joins that list. Again, hooking this event should be rare, so if it's one of those special events, that's fine.

Really, $.cleanData is bad for the same reason that the DOMSubtreeModified event is bad and was abandoned in favor of MutationObserver. Perhaps it would be better to see if we (or a third party) could build the DOM cleanup functionality on top of that with a shim for when it's not available.

A quick review indicates that jQuery's been doing a form of cleanData for more than four and a half years, since ​v1.2.2 when cleanup showed up in remove (which used to be called by empty) starting with line 1,296 of ​jquery-1.2.2.js:

// Prevent memory leaks
jQuery( "*", this ).add(this).each(function(){
 jQuery.event.remove(this);
 jQuery.removeData(this);
});

As far as I'm aware, there's been no uproar about it -- and browsers and computers are only getting faster. Until/unless it actually shows up as a real performance problem, worrying about removing it is premature optimization.

If jQuery is able at some point to do away with cleanData, that's the time to worry about deprecating this functionality. I don't find it convincing as an argument against in the current reality, particularly not given what people are doing today to get this functionality. :-)

[dmethvin]

I'm not convinced we should do anything here but I'll mark the ticket open.

[[email protected]…]

Using a custom event seems like a horribly roundabout way of doing this, when all that is needed to handle it in a very straightforward manner is adding a single trigger.

[Jock Murphy <[email protected]…>]

I am sorry, but I disagree with both @dmethvin and @oskar. The problem is that the same code that adds an event (or the like) should have a clean and well defined way to clean itself up. If you basically require the cleanup to happen manually, or in the onUnload, then it will get missed.

What @T.J. is proposing IS the straightforward and elegant solution. I was frankly a bit shocked to learn jQuery didn't have a good solution to this problem.

[dmethvin]

The problem is that the same code that adds an event (or the like) should have a clean and well defined way to clean itself up.

Yes, that is what the special-events interface is for, if the standard cleanup is insufficient. I agree with TJ Crowder about the potential messiness of each event needing its own teardown handler when there are a lot of custom events, so it depends on use case. But the current teardown handler design is very low overhead.

The "event (or the like)" is what the majority of this discussion is about! Single page applications and MVCs can spend a lot of their time in cleanData already. TJ Crowder's timing test is only showing that the additional *test* is cheap. If someone writes code that depends on an expensively triggered destroy event and makes a popular plugin, we'll start getting reports like #7992 again. So in that aspect I agree with oskar.

I was frankly a bit shocked

Sorry to shock, Jock.

[dmethvin]

Bulk change from enhancement to feature.

[mikesherov]

+0, Whatever dmethvin thinks.

[Rick Waldron]

-1

[anonymous]

+1

[gnarf]

+0, Having found myself wanting something like this, and then figuring out that teardown can actually accomplish it, I'm leaning towards "meh" on this feature.

[T.J. Crowder <[email protected]…>]

Replying to gnarf:

...and then figuring out that teardown can actually accomplish it...

The teardown solution leaves a lot to be desired IMV, not least multiple autonomous handlers, convoluted client code (hooking an event you'll never receive), and of course the completely avoidable duplication (jQuery UI duck punches it, TinyMCE monkey-patches it, my stuff adds a teardown, your stuff adds a teardown...).

jQuery isn't going to stop needing this cleanup any time soon, it just makes sense to open it up to code using the library as well, esp. with the amazingly trivial cost.

[jzaefferer]

@dmethvin: what would the cleanData monkey-punching in jQuery UI look like using the special event teardown? See ​https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.widget.js#L15

Also, can you elaborate on MutationObserver and how that relates to this discussion? The $.cleanData "hook" is synchronous, triggering the destroy event while the node is still in the DOM. MutationObserver will collect events and therefore trigger the callback after the node is detached. Could that become a problem in practice?

[dmethvin]

what would the cleanData monkey-punching in jQuery UI look like

You mean using special events as I proposed above? The cleanData punch could be removed entirely. There would just be a single special event handler in the widget factory for an event like uisanitation:

jQuery.event.special.uisanitation = {
   teardown: function(){
      // `this` is the DOM element; get the widget ref from that?
      getWidgetFromElement(this).destroy();
   }
};

Then instead of attaching a "remove" event in the widget create you'd just attach an empty uisanitation event to trigger the teardown and cleanData's own code would take care of the rest:

this._on( true, this.element, {
  uisanitation: jQuery.noop
  }
});

MutationObserver will collect events and therefore trigger the callback after the node is detached. Could that become a problem in practice?

If the destroyer expected synchronous behavior. What are "remove" events generally used for? They're not part of the public interface for widgets? The stuff done by the default handler looks like it would be safe to do on a detached element and perhaps even better to do that way, since changing an attached element might cause reflows.

[m_gol]

Migrated to ​https://github.com/jquery/jquery/issues/1736