Skip to content
Permalink
Browse files
Ticket #6808. Changes data() so on plain objects, it uses a function … 
…to contain the cache ID to avoid it being JSON serialized.
  • Loading branch information
@jeresig
InfinitiesLoop authored and jeresig committed Jul 22, 2010
1 parent ef9fb80 commit 9faab0b
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 7 deletions.
29 src/data.js
View file
@@ -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.

Sign in to view

Copy link
@rkatic

rkatic Jul 23, 2010

Contributor

In case of a node, the copy is made immediately (good), but for other objects, the copy is made on (each!) reading operations!
Am I missing something?

This comment has been minimized.

Sign in to view

Copy link
@jeresig

jeresig Jul 23, 2010

Member

See - that's exactly what I said when I did the code review. Dave had an explanation but unfortunately it looks like it's gone now? I'll bring this to his attention again.

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jul 23, 2010

Member

Isn't the reason included into the commit message: "Ticket #6808. Changes data() so on plain objects, it uses a function to contain the cache ID to avoid it being JSON serialized."?

This comment has been minimized.

Sign in to view

Copy link
@jeresig

jeresig Jul 23, 2010

Member

@jaubourg: We're talking about the .extend() being inside of the function instead of outside of it - not the function itself existing. I commented on this fact in the original review of this patch but Dave Reed had an explanation (although that commit appears to be gone now). I've pinged him to get his feedback here.

This comment has been minimized.

Sign in to view

Copy link
@nje

nje Jul 23, 2010

Yes, sorry about that. I misunderstood the feedback. Fixed here:
http://github.com/nje/jquery/commit/f96062602e9b82c3b6c75b13174681864cc828cf
Sorry for the messy double commit. Change is to use the 'store' object as you suggested, and now declare it towards the top since it is used in 2 places and to avoid a redeclaration warning.

This comment has been minimized.

Sign in to view

Copy link
@jeresig

jeresig Jul 23, 2010

Member

Combined and landed fixes in 2084e01.

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jul 23, 2010

Member

@jeresig: my bad, didn't understand rkatic's comment properly ;)

This comment has been minimized.

Sign in to view

Copy link
@rkatic

rkatic Jul 23, 2010

Contributor

Probably I need to use an grammar checker... unfortunately nobody here uses to indicate my English errors ;(

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Jul 24, 2010

Member

@rkatic: oh no no no, the misunderstanding is on my side, your sentence made perfect sense.

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

}

thisCache = cache[ id ];
if ( !isNode ) {
thisCache = thisCache();
}

// Prevent overriding the named cache with undefined values
if ( data !== undefined ) {
@@ -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 ) {
7 test/unit/data.js
View file
@@ -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');
@@ -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" );
@@ -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." );
});

5 comments on commit 9faab0b

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 9faab0b Jul 28, 2010

Choose a reason for hiding this comment

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

Btw. Is there a reason to not simply using a function as the object cache and setting properties to that function?
Besides avoiding one function call, that would make the .data() function something simpler eliminating some isNode checks.

@InfinitiesLoop
Copy link

Choose a reason for hiding this comment

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

Great idea.. but that would make fields of the function look like data, like .length

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 9faab0b Jul 29, 2010

Choose a reason for hiding this comment

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

Fair reason. How about to store cache object as the "store" property of the function? At least that would avoid one function call.

@InfinitiesLoop
Copy link

Choose a reason for hiding this comment

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

To be precise it would replace a function call with a field reference. Net positive I'm sure, but not sure how much. I'd want to get an idea of the perf difference. Either way it's probably a negligible difference, only making a difference if a page is doing tons and tons of data() calls very quickly.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 9faab0b Jul 29, 2010

Choose a reason for hiding this comment

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

Well, in cases where JS is not operating on the DOM, function calls becomes much more significant, and since we are discussing about not-node cases, avoiding not beneficial function calls seams preferable to me.

Please sign in to comment.