Skip to content
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

Fixed issue for undefined data values that breaks chaining #501

Closed
wants to merge 4 commits into from
Closed

Fixed issue for undefined data values that breaks chaining #501

wants to merge 4 commits into from

Conversation

jsoverson
Copy link

Added a check in $el.data(key,val) that looks to see whether or not a value was submitted rather than whether or not a value is undefined.

Looking for definededness causes the return value to be the previously stored data for the key rather than the calling element. This can break subsequent calls in a chain, eg

var foo = 1;
var bar;

$(element).data('foo', foo).data('bar',bar).appendTo(otherElement)

This chain would error out because the second attempt at setting data, for key 'bar', would try to return the original value for the key instead of attempting to set the value and returning the calling element.

You still can't set the data to undefined, that pre-existing concern already seems to be explicitly checked and prevented in jQuery.data(), this fix just ensures that the chain remains stable under what would likely be the most common expectation.

I think setting a value to undefined is a valid use case, though, and would be easily addressed by removing lines 102 and 104, but I don't know enough about the reasoning behind disallowing it to suggest this right now.

@rwaldron
Copy link
Member

Please cite a ticket for this issue, see: http://bugs.jquery.com Additionally, patches must conform to the jQuery Core Style Guide, see: http://docs.jquery.com/JQuery_Core_Style_Guidelines . Lastly, there will need to be proven test cases included with any patches.

@jsoverson
Copy link
Author

http://bugs.jquery.com/ticket/10266 was created for this issue.

http://bugs.jquery.com/ticket/9977 looks like it had previously referred to the issue but was closed prematurely due to incorrect assumptions about its cause.

Changed fix commit to be more in style with surroundings. Added unit test that fails with current source tree and passes with changes.

@rwaldron
Copy link
Member

This patch is invalid. Making commits via github's interface is a red flag indication that you have not tested this code against the browsers that jQuery supports. Furthermore, your unit test has never been run with this code (otherwise you'd know that it actually fails because you added you a 6th test to a unit that expects 5 tests - which will fail the unit). Lastly, your test does not conform to the jQuery Core Style Guide

@jsoverson
Copy link
Author

It's also indicative of someone trying to be helpful and submitting a patch even though he doesn't have the time to set up the keys necessary to make appropriate commits to github. As simple as I thought a one line patch would be, I apologize for missing one edit in the unit tests. It's been fixed.

I did get the unit tests running locally, did check out qunit and sizzle in order to get them to work, did test them in the three browsers I had access to (firefox, safari, chrome), and then committed the changes via the web out of simplicity.

I apologize for the missing line and appreciate you noticing it.

Referring to what part of the style guidelines the test is not conforming to would make its fix come a lot more quickly.

@timmywil
Copy link
Member

Per previous ticket discussions, this has been closed as wontfix.

@timmywil timmywil closed this Sep 20, 2011
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants