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.

Sign up for GitHub

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

Data: Support object as second argument #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
43832ae
Data: Support object as second argument
wjmao88 Oct 27, 2016
73ffd58
Fix single quote to double quote
wjmao88 Oct 27, 2016
19a2516
Fix white space
wjmao88 Oct 27, 2016
96c5817
Data: Add test case for setting entire object
wjmao88 Oct 27, 2016
b29c94f
Data: Store non-camel data separately with its non-camel key
wjmao88 Oct 27, 2016
4151a22
Data: use two separate test cases for sames and diffs
wjmao88 Oct 27, 2016
6a4df76
Data: update test metadata
wjmao88 Oct 27, 2016
5fa5460
Data: update format and separte single and mass set test
wjmao88 Nov 18, 2016
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
View file
Open in desktop
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
View file
Open in desktop
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 );
} );
} );

} );