Navigation Menu

Skip to content

Commit

Permalink
Ticket #6808. Changes data() so on plain objects, it uses a function …
Browse files Browse the repository at this point in the history
…to contain the cache ID to avoid it being JSON serialized.
  • Loading branch information
InfinitiesLoop committed Jul 20, 2010
1 parent 2f1aea7 commit 6be9747
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 7 deletions.
29 changes: 25 additions & 4 deletions src/data.js
Expand Up @@ -46,13 +46,30 @@ jQuery.extend({
// Avoid generating a new cache unless none exists and we
// want to manipulate it.
if ( typeof name === "object" ) {
cache[ id ] = jQuery.extend(true, {}, name);
if ( isNode ) {
cache[ id ] = jQuery.extend(true, {}, name);
} else {
cache[ id ] = function() {
return jQuery.extend(true, {}, name);

This comment has been minimized.

Copy link
@jeresig

jeresig Jul 21, 2010

It seems like this should only be done on the initial set, not on every call? Perhaps use a similar var store = ...; technique as below?

This comment has been minimized.

Copy link
@nje

nje Jul 22, 2010

Owner

Issue is we need to replace the entire set of data being returned by the function, not 1 value from it. So either the function always returns the same object, and we hang a 'data' off it which can be replaced here, or we replace the function with a new one that returns the new data. I'd think that usually data() will be used with a string key, in which case it would be more efficient not to have to deference '.data' off the return value of the function. Using data(obj) to replace all the data will involve replacing the function, but should be much more rare.

}

This comment has been minimized.

Copy link
@jeresig

jeresig Jul 21, 2010

Very minor - a missing ; here.

}

} else if ( !cache[ id ] ) {
cache[ id ] = {};
if ( isNode ) {
cache[ id ] = {};
} else {
var store = {};
cache[ id ] = function() {
return store;
}

This comment has been minimized.

Copy link
@jeresig

jeresig Jul 21, 2010

Missing ; here as well.

}

}

thisCache = cache[ id ];

This comment has been minimized.

Copy link
@jeresig

jeresig Jul 21, 2010

Perhaps these lines could just be turned into:

thisCache = isNode ? cache[ id ] : thisCache();

This comment has been minimized.

Copy link
@nje

nje Jul 22, 2010

Owner

Yeah.. it would be this though, which is not quite as nice, but still better:
thisCache = isNode? cache[ id ] : cache id ;

if ( !isNode ) {
thisCache = thisCache();
}

// Prevent overriding the named cache with undefined values
if ( data !== undefined ) {
Expand All @@ -71,8 +88,12 @@ jQuery.extend({
windowData :
elem;

var id = elem[ jQuery.expando ], cache = jQuery.cache,
isNode = elem.nodeType, thisCache = isNode ? cache[ id ] : id;
var isNode = elem.nodeType,
id = elem[ jQuery.expando ], cache = jQuery.cache;
if ( id && !isNode ) {
id = id();
}
var thisCache = cache[ id ];

// If we want to remove a specific section of the element's data
if ( name ) {
Expand Down
7 changes: 4 additions & 3 deletions test/unit/data.js
@@ -1,13 +1,14 @@
module("data");

test("expando", function(){
expect(6);
expect(7);

equals("expando" in jQuery, true, "jQuery is exposing the expando");

var obj = {};
jQuery.data(obj);
equals( jQuery.expando in obj, true, "jQuery.data adds an expando to the object" );
equals( typeof obj[jQuery.expando], "function", "jQuery.data adds an expando to the object as a function" );

obj = {};
jQuery.data(obj, 'test');
Expand All @@ -17,7 +18,7 @@ test("expando", function(){
jQuery.data(obj, "foo", "bar");
equals( jQuery.expando in obj, true, "jQuery.data added an expando to the object" );

var id = obj[jQuery.expando];
var id = obj[jQuery.expando]();
equals( id in jQuery.cache, false, "jQuery.data did not add an entry to jQuery.cache" );

equals( id.foo, "bar", "jQuery.data worked correctly" );
Expand Down Expand Up @@ -54,7 +55,7 @@ test("jQuery.data", function() {
jQuery.data( obj, "prop", true );

ok( obj[ jQuery.expando ], "Data is being stored on the object." );
ok( obj[ jQuery.expando ].prop, "Data is being stored on the object." );
ok( obj[ jQuery.expando ]().prop, "Data is being stored on the object." );

equals( jQuery.data( obj, "prop" ), true, "Make sure the right value is retrieved." );
});
Expand Down

1 comment on commit 6be9747

@nje
Copy link
Owner

@nje nje commented on 6be9747 Jul 22, 2010

Choose a reason for hiding this comment

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

Please sign in to comment.