Skip to content

Commit

Permalink
Widget: Hook into jQuery.cleanData to auto-destroy widgets. Fixes #60…
Browse files Browse the repository at this point in the history 
…08 - Widget: auto-destroy is broken in jQuery 1.4.
  • Loading branch information
@scottgonzalez
scottgonzalez committed Sep 3, 2010
1 parent e5f3bfc commit 0a0a39f
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 14 deletions.
4 changes: 3 additions & 1 deletion tests/unit/widget/widget.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ <h2 id="qunit-userAgent"></h2>
<div id="main" style="position: absolute; top: -10000px; left: -10000px;">

<div id="widget-wrapper">
<div id="widget"></div>
<div id="widget">
<div>...</div>
</div>
</div>

</div>
Expand Down
67 changes: 67 additions & 0 deletions tests/unit/widget/widget_core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,4 +409,71 @@ test( "._trigger() - provide event and ui", function() {
.testWidget( "testEvent" );
});

test( "auto-destroy - .remove()", function() {
expect( 1 );
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( true, "destroyed from .remove()" );
}
});
$( "#widget" ).testWidget().remove();
});

test( "auto-destroy - .remove() on parent", function() {
expect( 1 );
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( true, "destroyed from .remove() on parent" );
}
});
$( "#widget" ).testWidget().parent().remove();
});

test( "auto-destroy - .remove() on child", function() {
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( false, "destroyed from .remove() on child" );
}
});
$( "#widget" ).testWidget().children().remove();
// http://github.com/jquery/qunit/pull/34
$.ui.testWidget.prototype.destroy = $.noop;
});

test( "auto-destroy - .empty()", function() {
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( false, "destroyed from .empty()" );
}
});
$( "#widget" ).testWidget().empty();
// http://github.com/jquery/qunit/pull/34
$.ui.testWidget.prototype.destroy = $.noop;
});

test( "auto-destroy - .empty() on parent", function() {
expect( 1 );
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( true, "destroyed from .empty() on parent" );
}
});
$( "#widget" ).testWidget().parent().empty();
});

test( "auto-destroy - .detach()", function() {
$.widget( "ui.testWidget", {
_create: function() {},
destroy: function() {
ok( false, "destroyed from .detach()" );
}
});
$( "#widget" ).testWidget().detach();
});

})( jQuery );
36 changes: 23 additions & 13 deletions ui/jquery.ui.widget.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,30 @@
*/
(function( $, undefined ) {

var _remove = $.fn.remove;

$.fn.remove = function( selector, keepData ) {
return this.each(function() {
if ( !keepData ) {
if ( !selector || $.filter( selector, [ this ] ).length ) {
$( "*", this ).add( [ this ] ).each(function() {
$( this ).triggerHandler( "remove" );
});
}
// jQuery 1.4+
if ( $.cleanData ) {
var _cleanData = $.cleanData;
$.cleanData = function( elems ) {
for ( var i = 0, elem; (elem = elems[i]) != null; i++ ) {
$( elem ).triggerHandler( "remove" );
}
return _remove.call( $(this), selector, keepData );
});
};
_cleanData( elems );

This comment has been minimized.

Sign in to view

Copy link
@boraMan

boraMan Jan 17, 2013

There's an additional (internal) argument 'acceptData' in jQuery.cleanData - why's that not passed here?
(seems to be a perf.opt. as it just prevents some duplicate calls to jQuery.acceptData)

This comment has been minimized.

Sign in to view

Copy link
@scottgonzalez

scottgonzalez Jan 17, 2013

Author Member

Notice the word internal that you used? Also, it doesn't exist in all versions.

This comment has been minimized.

Sign in to view

Copy link
@boraMan

boraMan Jan 17, 2013

Absolutely - but doesn't that also apply to the whole .cleanData itself?
(Did you already think about using $.event.special instead of hooking into cleanData and changing some of its behavior?)

This comment has been minimized.

Sign in to view

Copy link
@scottgonzalez

scottgonzalez Jan 17, 2013

Author Member

Think about it? yes? Actually try it? no. Would you like to send a pull request along with performance tests?

This comment has been minimized.

Sign in to view

Copy link
@boraMan

boraMan Jan 17, 2013

Ok, I'll give it a try. Could you give me a hint how to submit performance tests? (sorry - I'm new in here...)
};
} else {
var _remove = $.fn.remove;
$.fn.remove = function( selector, keepData ) {
return this.each(function() {
if ( !keepData ) {
if ( !selector || $.filter( selector, [ this ] ).length ) {
$( "*", this ).add( [ this ] ).each(function() {
$( this ).triggerHandler( "remove" );
});
}
}
return _remove.call( $(this), selector, keepData );
});
};
}

$.widget = function( name, base, prototype ) {
var namespace = name.split( "." )[ 0 ],
Expand Down

2 comments on commit 0a0a39f

@ehynds
Copy link

@ehynds ehynds commented on 0a0a39f Sep 3, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why destroy widgets on detach?
@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, the test makes sure that destroy isn't called on .detach().
Please sign in to comment.