Skip to content

Data: Support object as second argument #235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,26 @@ var oldData = jQuery.data;
jQuery.data = function( elem, name, value ) {
var curData;

// Name can be an object, and each entry in the object is meant to be set as data
if ( name && typeof name === "object" && arguments.length === 2 ) {
curData = jQuery.hasData( elem ) && oldData.call( this, elem );
var sameKeys = {};
for ( var key in name ) {
if ( key !== jQuery.camelCase( key ) ) {
migrateWarn( "jQuery.data() always sets/gets camelCased names: " + key );
curData[ key ] = name[ key ];
} else {
sameKeys[ key ] = name[ key ];
}
}

oldData.call( this, elem, sameKeys );
Copy link
Member

Choose a reason for hiding this comment

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

Why not oldData.apply( this, arguments ) similarly to how what the other configuration is handled? You wouldn't have to record sameKeys at all then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sameKeys is not the same as the second argument, as it only contains keys that are in camelCase and thus does not need to be handled here.

The other oldData.apply is actually doing the same if it determines the key is already camel case.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, I was comparing it to the second part but there's an early return there if the non-camelCased version of the key was found. It's fine then. 👍


return name;
}

// If the name is transformed, look for the un-transformed name in the data object
if ( name && name !== jQuery.camelCase( name ) ) {
if ( name && typeof name === "string" && name !== jQuery.camelCase( name ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is necessary. We only support documented scenarios, while we want to add support for the object as the second parameter here at this point we know it's not an object so we're fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your reason, which is why I think it should goes straight to jQuery's own .data method, and will throw the same error as when jquery-migrate is not used.

Copy link
Member

Choose a reason for hiding this comment

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

@mgol I see @wjmao88 's point here, it's ensuring the behavior (and error location) is identical in both cases. Are you good with that?

curData = jQuery.hasData( elem ) && oldData.call( this, elem );
if ( curData && name in curData ) {
migrateWarn( "jQuery.data() always sets/gets camelCased names: " + name );
Expand Down
52 changes: 52 additions & 0 deletions test/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,55 @@ test( "jQuery.data() camelCased names", function( assert ) {
} );

} );

test( "jQuery.data() camelCased names (mass setter)", function( assert ) {
var sames = [
"datum",
"ropeAdope",
"Олег\u0007Michał",
"already-Big",
"number-2",
"unidash-"
],
diffs = [
"dat-data",
"hangy-dasher-",
"-dashy-hanger"
];

assert.expect( 11 );

var div = document.createElement( "div" );

// = sames.length + noWarning
expectNoWarning( "Data set as an object and get without warning via API", function() {
var testData = {};

sames.forEach( function( name, index ) {
testData[ name ] = index;
} );

jQuery.data( div, testData );

sames.forEach( function( name, index ) {
assert.equal( jQuery.data( div, name ), index, name + "=" + index );
} );
} );

// = diffs.length + warning
expectWarning( "Data set as an object and get without warning via API", function() {
var testData = {};

diffs.forEach( function( name, index ) {
testData[ name ] = index;
} );

jQuery.data( div, testData );

diffs.forEach( function( name, index ) {
assert.equal( jQuery.data( div, name ), index, name + "=" + index );
} );
} );

} );