Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

DRY out removeData/cleanData #838

Closed
wants to merge 3 commits into from

3 participants

@gibson042
Collaborator

There was a great deal of duplicated code between jQuery.removeData and jQuery.cleanData, along with divergence which probably should not have happened. Let's sync them up.

Sizes - compared to master @ 3c86547

    255980      (-119)  dist/jquery.js
     93000      (-146)  dist/jquery.min.js
     33467       (-35)  dist/jquery.min.js.gz
src/data.js
((12 lines not shown))
// don't care
// Ensure that `cache` is not a window object #10080
- if ( jQuery.support.deleteExpando || !cache.setInterval ) {
+ else if ( jQuery.support.deleteExpando || !jQuery.isWindow( cache ) ) {
@rwaldron Collaborator

For the sake of readability, can you re-format this so there is no break-up from the if to the else if - thanks

@gibson042 Collaborator

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmethvin dmethvin closed this in f8baea8
@mescoda mescoda referenced this pull request from a commit in mescoda/jquery
@gibson042 gibson042 DRY out removeData/cleanData, closes gh-838. 2f0d175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 26, 2012
  1. @gibson042

    DRY out removeData/cleanData

    gibson042 authored
Commits on Jun 27, 2012
  1. @gibson042

    style guide adherence

    gibson042 authored
Commits on Jul 2, 2012
  1. @gibson042

    small performance boost

    gibson042 authored
This page is out of date. Refresh to see the latest.
Showing with 63 additions and 74 deletions.
  1. +14 −38 src/data.js
  2. +49 −36 src/manipulation.js
View
52 src/data.js
@@ -128,16 +128,11 @@ jQuery.extend({
var thisCache, i, l,
- // Reference to internal data cache key
- internalKey = jQuery.expando,
-
isNode = elem.nodeType,
// See jQuery.data for more information
cache = isNode ? jQuery.cache : elem,
-
- // See jQuery.data for more information
- id = isNode ? elem[ internalKey ] : internalKey;
+ id = isNode ? elem[ jQuery.expando ] : jQuery.expando;
// If there is already no cache entry for this object, there is no
// purpose in continuing
@@ -164,7 +159,7 @@ jQuery.extend({
if ( name in thisCache ) {
name = [ name ];
} else {
- name = name.split( " " );
+ name = name.split(" ");
}
}
}
@@ -187,37 +182,23 @@ jQuery.extend({
// Don't destroy the parent cache unless the internal data object
// had been the only thing left in it
- if ( !isEmptyDataObject(cache[ id ]) ) {
+ if ( !isEmptyDataObject( cache[ id ] ) ) {
return;
}
}
- // Browsers that fail expando deletion also refuse to delete expandos on
- // the window, but it will allow it on all other JS objects; other browsers
- // don't care
- // Ensure that `cache` is not a window object #10080
- if ( jQuery.support.deleteExpando || !cache.setInterval ) {
+ // Destroy the cache
+ if ( isNode ) {
+ jQuery.cleanData( [ elem ], true );
+
+ // Use delete when supported for expandos or `cache` is not a window per isWindow (#10080)
+ } else if ( jQuery.support.deleteExpando || cache != cache.window ) {
delete cache[ id ];
+
+ // When all else fails, null
} else {
cache[ id ] = null;
}
-
- // We destroyed the cache and need to eliminate the expando on the node to avoid
- // false lookups in the cache for entries that no longer exist
- if ( isNode ) {
- jQuery.deletedIds.push( id );
-
- // IE does not allow us to delete expando properties from nodes,
- // nor does it have a removeAttribute function on Document nodes;
- // we must handle all of these cases
- if ( jQuery.support.deleteExpando ) {
- delete elem[ internalKey ];
- } else if ( elem.removeAttribute ) {
- elem.removeAttribute( internalKey );
- } else {
- elem[ internalKey ] = null;
- }
- }
},
// For internal use only.
@@ -227,15 +208,10 @@ jQuery.extend({
// A method for determining if a DOM node can handle the data expando
acceptData: function( elem ) {
- if ( elem.nodeName ) {
- var match = jQuery.noData[ elem.nodeName.toLowerCase() ];
-
- if ( match ) {
- return !(match === true || elem.getAttribute("classid") !== match);
- }
- }
+ var noData = elem.nodeName && jQuery.noData[ elem.nodeName.toLowerCase() ];
- return true;
+ // nodes accept data unless otherwise specified; rejection can be conditional
+ return !noData || noData !== true && elem.getAttribute("classid") === noData;
}
});
View
85 src/manipulation.js
@@ -347,14 +347,18 @@ jQuery.fn.extend({
if ( scripts.length ) {
jQuery.each( scripts, function( i, elem ) {
if ( elem.src ) {
- jQuery.ajax ? jQuery.ajax({
- url: elem.src,
- type: "GET",
- dataType: "script",
- async: false,
- global: false,
- throws: true
- }) : jQuery.error( "no ajax" );
+ if ( jQuery.ajax ) {
+ jQuery.ajax({
+ url: elem.src,
+ type: "GET",
+ dataType: "script",
+ async: false,
+ global: false,
+ throws: true
+ });
+ } else {
+ jQuery.error("no ajax");
+ }
} else {
jQuery.globalEval( ( elem.text || elem.textContent || elem.innerHTML || "" ).replace( rcleanScript, "" ) );
}
@@ -760,45 +764,54 @@ jQuery.extend({
return ret;
},
- cleanData: function( elems ) {
- var data, id,
+ cleanData: function( elems, /* internal */ acceptData ) {
+ var data, id, elem, type,
+ i = 0,
+ internalKey = jQuery.expando,
cache = jQuery.cache,
- special = jQuery.event.special,
- deleteExpando = jQuery.support.deleteExpando;
+ deleteExpando = jQuery.support.deleteExpando,
+ special = jQuery.event.special;
- for ( var i = 0, elem; (elem = elems[i]) != null; i++ ) {
- if ( elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()] ) {
- continue;
- }
+ for ( ; (elem = elems[i]) != null; i++ ) {
- id = elem[ jQuery.expando ];
+ if ( acceptData || jQuery.acceptData( elem ) ) {
- if ( id ) {
- data = cache[ id ];
+ id = elem[ internalKey ];
+ data = id && cache[ id ];
- if ( data && data.events ) {
- for ( var type in data.events ) {
- if ( special[ type ] ) {
- jQuery.event.remove( elem, type );
+ if ( data ) {
+ if ( data.events ) {
+ for ( type in data.events ) {
+ if ( special[ type ] ) {
+ jQuery.event.remove( elem, type );
- // This is a shortcut to avoid jQuery.event.remove's overhead
- } else {
- jQuery.removeEvent( elem, type, data.handle );
+ // This is a shortcut to avoid jQuery.event.remove's overhead
+ } else {
+ jQuery.removeEvent( elem, type, data.handle );
+ }
}
}
- }
- // Remove cache only if jQuery.event.remove was not removed it before
- if ( cache[ id ] ) {
- if ( deleteExpando ) {
- delete elem[ jQuery.expando ];
+ // Remove cache only if it was not already removed by jQuery.event.remove
+ if ( cache[ id ] ) {
- } else if ( elem.removeAttribute ) {
- elem.removeAttribute( jQuery.expando );
- }
+ delete cache[ id ];
+
+ // IE does not allow us to delete expando properties from nodes,
+ // nor does it have a removeAttribute function on Document nodes;
+ // we must handle all of these cases
+ if ( deleteExpando ) {
+ delete elem[ internalKey ];
- jQuery.deletedIds.push( id );
- delete cache[ id ];
+ } else if ( elem.removeAttribute ) {
+ elem.removeAttribute( internalKey );
+
+ } else {
+ elem[ internalKey ] = null;
+ }
+
+ jQuery.deletedIds.push( id );
+ }
}
}
}
Something went wrong with that request. Please try again.