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

Conversation

wjmao88
Copy link
Contributor

@wjmao88 wjmao88 commented Oct 27, 2016

jQuery.data supports passing an object as a second argument and sets data for each entry in that object.

This warns the user about each violating non-camelCase key in the object.

This also adds a type check to avoid a current bug where jQuery.camelCase throws an error because a non-string is passed to it.

jQuery.data supports passing an object as a second argument and sets data for each entry in that object.

This warns the user about each violating non-camelCase key in the object.

This also adds a type check to avoid a current bug where jQuery.camelCase throws an error because a non-string is passed to it.
@mention-bot
Copy link

@wjmao88, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dmethvin to be a potential reviewer.

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 86.076% when pulling 19a2516 on wjmao88:data-name into 5acafbc on jquery:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 88.017% when pulling 6a4df76 on wjmao88:data-name into 5acafbc on jquery:master.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

There should be a space after //.

}
}

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. 👍

@@ -37,6 +37,38 @@ test( "jQuery.data() camelCased names", function( assert ) {
} );
} );

div = document.createElement( "div" );
Copy link
Member

Choose a reason for hiding this comment

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

Could you move all this code to a separate test? An example name: "jQuery.data() camelCased names (mass setter)".

@mgol mgol added this to the 3.1.0 milestone Nov 16, 2016
@mgol
Copy link
Member

mgol commented Nov 16, 2016

@wjmao88 Sorry for keeping you wait this long with the review. Will you have time to address my comments? If not, I can finish it for you.

@mgol
Copy link
Member

mgol commented Nov 16, 2016

This PR should fix issue #198.

@wjmao88
Copy link
Contributor Author

wjmao88 commented Nov 17, 2016

@mgol Yes, I'll be able to find time within a day or so.

// 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?

}
}

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.

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. 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 88.017% when pulling 5fa5460 on wjmao88:data-name into 1cb2093 on jquery:master.

@mgol
Copy link
Member

mgol commented Nov 21, 2016 via email

@mgol mgol closed this in 44e27cc Nov 23, 2016
@mgol
Copy link
Member

mgol commented Nov 23, 2016

@wjmao88 Landed, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants