Skip to content

Commit

Permalink
Data: always camelCase keys in .data()
Browse files Browse the repository at this point in the history
- This effectively implements our "Embrace HTML5" option
- Related: http://goo.gl/GcQAtn

Fixes gh-2257
  • Loading branch information
timmywil committed May 4, 2015
1 parent 2862a07 commit 0e79098
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 62 deletions.
34 changes: 7 additions & 27 deletions src/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,32 +111,25 @@ jQuery.fn.extend({
}

return access( this, function( value ) {
var data, camelKey;
var data;

// The calling jQuery object (element matches) is not empty
// (and therefore has an element appears at this[ 0 ]) and the
// `value` parameter was not undefined. An empty jQuery object
// will result in `undefined` for elem = this[ 0 ] which will
// throw an exception if an attempt to read a data cache is made.
if ( elem && value === undefined ) {
// Attempt to get data from the cache
// with the key as-is
data = dataUser.get( elem, key );
if ( data !== undefined ) {
return data;
}

camelKey = jQuery.camelCase( key );
// Attempt to get data from the cache
// with the key camelized
data = dataUser.get( elem, camelKey );
// The key will always be camelCased in Data
data = dataUser.get( elem, key );
if ( data !== undefined ) {
return data;
}

// Attempt to "discover" the data in
// HTML5 custom data-* attrs
data = dataAttr( elem, camelKey, undefined );
data = dataAttr( elem, key );
if ( data !== undefined ) {
return data;
}
Expand All @@ -146,23 +139,10 @@ jQuery.fn.extend({
}

// Set the data...
camelKey = jQuery.camelCase( key );
this.each(function() {
// First, attempt to store a copy or reference of any
// data that might've been store with a camelCased key.
var data = dataUser.get( this, camelKey );

// For HTML5 data-* attribute interop, we have to
// store property names with dashes in a camelCase form.
// This might not apply to all properties...*
dataUser.set( this, camelKey, value );

// *... In the case of properties that might _actually_
// have dashes, we need to also store a copy of that
// unchanged property.
if ( key.indexOf("-") > -1 && data !== undefined ) {
dataUser.set( this, key, value );
}

// We always store the camelCased key
dataUser.set( this, key, value );
});
}, null, value, arguments.length > 1, null, true );
},
Expand Down
57 changes: 27 additions & 30 deletions src/data/Data.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Data.prototype = {
return owner[ this.expando ];
},
cache: function( owner, initial ) {

// We can accept data for non-element nodes in modern browsers,
// but we should not, see #8335.
// Always return an empty object.
Expand All @@ -57,14 +58,16 @@ Data.prototype = {
cache = this.cache( owner );

// Handle: [ owner, key, value ] args
// Always use camelCase key (gh-2257)
if ( typeof data === "string" ) {
cache[ data ] = value;
cache[ jQuery.camelCase( data ) ] = value;

// Handle: [ owner, { properties } ] args
} else {

// Copy the properties one-by-one to the cache object
for ( prop in data ) {
cache[ prop ] = data[ prop ];
cache[ jQuery.camelCase( prop ) ] = data[ prop ];
}
}
return cache;
Expand All @@ -73,10 +76,13 @@ Data.prototype = {
var cache = this.cache( owner );

return key === undefined ?
cache : cache[ key ];
cache :

// Always use camelCase key (gh-2257)
cache[ jQuery.camelCase( key ) ];
},
access: function( owner, key, value ) {
var stored;

// In cases where either:
//
// 1. No key was specified
Expand All @@ -89,12 +95,9 @@ Data.prototype = {
// 2. The data stored at the key
//
if ( key === undefined ||
((key && typeof key === "string") && value === undefined) ) {
( ( key && typeof key === "string" ) && value === undefined ) ) {

This comment has been minimized.

Copy link
@rwaldron

rwaldron May 4, 2015

Member

Did the style guide change to require space on all parenthesized expressions? Used to be that nested parenthesized expressions didn't require additional padding.

This comment has been minimized.

Copy link
@timmywil

timmywil May 4, 2015

Author Member

To my dismay, yes. At some point, we'll let jscs do a sweep and make all of the styles consistent.

This comment has been minimized.

Copy link
@timmywil

timmywil May 4, 2015

Author Member

I did this here just to make my editor stop showing yellow.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
@dmethvin

dmethvin May 4, 2015

Member

Yes, still unsure of the spaciness vs the old guide. See also jquery-archive/PEP#185 which seems to be going less spacey for some reason.


stored = this.get( owner, key );

return stored !== undefined ?
stored : this.get( owner, jQuery.camelCase(key) );
return this.get( owner, key );
}

// [*]When the key is not a string, or both a key and value
Expand All @@ -110,7 +113,7 @@ Data.prototype = {
return value !== undefined ? value : key;
},
remove: function( owner, key ) {
var i, name, camel,
var i,
cache = owner[ this.expando ];

if ( cache === undefined ) {
Expand All @@ -121,33 +124,27 @@ Data.prototype = {
this.register( owner );

} else {

// Support array or space separated string of keys
if ( jQuery.isArray( key ) ) {
// If "name" is an array of keys...
// When data is initially created, via ("key", "val") signature,
// keys will be converted to camelCase.
// Since there is no way to tell _how_ a key was added, remove
// both plain key and camelCase key. #12786
// This will only penalize the array argument path.
name = key.concat( key.map( jQuery.camelCase ) );

// If key is an array of keys...
// We always set camelCase keys, so remove that.
key = key.map( jQuery.camelCase );
} else {
camel = jQuery.camelCase( key );
// Try the string as a key before any manipulation
if ( key in cache ) {
name = [ key, camel ];
} else {
// If a key with the spaces exists, use it.
// Otherwise, create an array by matching non-whitespace
name = camel;
name = name in cache ?
[ name ] : ( name.match( rnotwhite ) || [] );
}
key = jQuery.camelCase( key );

// If a key with the spaces exists, use it.
// Otherwise, create an array by matching non-whitespace
key = key in cache ?
[ key ] :
( key.match( rnotwhite ) || [] );
}

i = name.length;
i = key.length;

while ( i-- ) {
delete cache[ name[ i ] ];
delete cache[ key[ i ] ];
}
}
},
Expand Down
36 changes: 31 additions & 5 deletions test/unit/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,34 @@ test(".data should not miss attr() set data-* with hyphenated property names", f
deepEqual( b.data("long-param"), { a: 2 }, "data with property long-param was found, 2" );
});

test(".data always sets data with the camelCased key (gh-2257)", function() {
expect( 18 );

var div = jQuery("<div>").appendTo("#qunit-fixture"),
datas = {
"non-empty": "a string",
"empty-string": "",
"one-value": 1,
"zero-value": 0,
"an-array": [],
"an-object": {},
"bool-true": true,
"bool-false": false,

// JSHint enforces double quotes,
// but JSON strings need double quotes to parse
// so we need escaped double quotes here
"some-json": "{ \"foo\": \"bar\" }"
};

jQuery.each( datas, function( key, val ) {
div.data( key, val );
var allData = div.data();
equal( allData[ key ], undefined, ".data does not store with hyphenated keys" );
equal( allData[ jQuery.camelCase( key ) ], val, ".data stores the camelCased key" );
});
});

test(".data supports interoperable hyphenated/camelCase get/set of properties with arbitrary non-null|NaN|undefined values", function() {

var div = jQuery("<div/>", { id: "hyphened" }).appendTo("#qunit-fixture"),
Expand Down Expand Up @@ -679,19 +707,18 @@ test(".data supports interoperable removal of properties SET TWICE #13850", func
});
});

test( ".removeData supports removal of hyphenated properties via array (#12786)", function() {
test( ".removeData supports removal of hyphenated properties via array (#12786, gh-2257)", function() {
expect( 4 );

var div, plain, compare;

div = jQuery("<div>").appendTo("#qunit-fixture");
plain = jQuery({});

// When data is batch assigned (via plain object), the properties
// are not camel cased as they are with (property, value) calls
// Properties should always be camelCased
compare = {
// From batch assignment .data({ "a-a": 1 })
"a-a": 1,
"aA": 1,
// From property, value assignment .data( "b-b", 1 )
"bB": 1
};
Expand All @@ -706,7 +733,6 @@ test( ".removeData supports removal of hyphenated properties via array (#12786)"
div.removeData([ "a-a", "b-b" ]);
plain.removeData([ "a-a", "b-b" ]);

// NOTE: Timo's proposal for "propEqual" (or similar) would be nice here
deepEqual( div.data(), {}, "Data is empty. (div)" );
deepEqual( plain.data(), {}, "Data is empty. (plain)" );
});
Expand Down

1 comment on commit 0e79098

@markelog
Copy link
Member

Choose a reason for hiding this comment

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

/cc @rwaldron

Please sign in to comment.