Skip to main content

Bug Tracker

Side navigation

#12213 closed feature (migrated)

Opened August 07, 2012 04:30PM UTC

Closed October 20, 2014 11:50PM UTC

Add hook to cleanData to allow cleanup

Reported by: T.J. Crowder <tj@crowdersoftware.com> Owned by: gnarf
Priority: low Milestone: None
Component: manipulation Version: 1.8rc1
Keywords: 1.9-discuss Cc:
Blocked by: Blocking:
Description

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.

Attachments (0)
Change History (25)

Changed August 07, 2012 04:35PM UTC by scottgonzalez comment:1

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.

Changed August 07, 2012 04:45PM UTC by dmethvin comment:2

owner: → T.J. Crowder <tj@crowdersoftware.com>
status: newpending

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?

Changed August 07, 2012 06:20PM UTC by T.J. Crowder <tj@crowdersoftware.com> comment:3

status: pendingnew

Replying to [comment:1 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.

Changed August 07, 2012 06:23PM UTC by scottgonzalez comment:4

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.

Changed August 07, 2012 06:31PM UTC by T.J. Crowder <tj@crowdersoftware.com> comment:5

Replying to [comment:4 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 [comment:2 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.

Changed August 07, 2012 07:03PM UTC by Jock Murphy <jock@jockmurphy.com> comment:6

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.

Changed August 07, 2012 07:29PM UTC by T.J. Crowder <tj@crowdersoftware.com> comment:7

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.

Changed August 07, 2012 07:54PM UTC by dmethvin comment:8

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.

Changed August 07, 2012 10:17PM UTC by T.J. Crowder <tj@crowdersoftware.com> comment:9

Replying to [comment:8 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. :-)

Changed August 21, 2012 01:30AM UTC by dmethvin comment:10

component: unfiledmanipulation
priority: undecidedlow
status: newopen

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

Changed September 07, 2012 10:28AM UTC by oskar.ols@gmail.com comment:11

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.

Changed September 07, 2012 09:37PM UTC by Jock Murphy <jock@jockmurphy.com> comment:12

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.

Changed September 08, 2012 02:37PM UTC by dmethvin comment:13

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.

Changed September 09, 2012 01:11AM UTC by dmethvin comment:14

type: enhancementfeature

Bulk change from enhancement to feature.

Changed September 17, 2012 06:01PM UTC by gibson042 comment:15

keywords: → 1.9-discuss

Changed October 14, 2012 10:36PM UTC by mikesherov comment:16

+0, Whatever dmethvin thinks.

Changed October 14, 2012 11:36PM UTC by rwaldron comment:17

-1

Changed October 15, 2012 08:56AM UTC by anonymous comment:18

+1

Changed October 22, 2012 05:42PM UTC by gnarf comment:19

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

Changed October 22, 2012 05:51PM UTC by T.J. Crowder <tj@crowdersoftware.com> comment:20

Replying to [comment:19 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.

Changed October 29, 2012 05:18PM UTC by mikesherov comment:21

milestone: None1.9
owner: T.J. Crowder <tj@crowdersoftware.com>gnarf
status: openassigned

Changed November 14, 2012 07:35AM UTC by jzaefferer comment:22

@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?

Changed November 14, 2012 02:04PM UTC by dmethvin comment:23

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(){
      //  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.

Changed March 01, 2013 05:49PM UTC by dmethvin comment:24

milestone: 1.9None

Changed October 20, 2014 11:50PM UTC by m_gol comment:25

resolution: → migrated
status: assignedclosed