Skip to content
Permalink
Browse files

Pull data-* attributes into .data(). Original code by Andrée Hasson a…

…nd Paul Irish. Fixes #6921.
  • Loading branch information...
jeresig committed Sep 20, 2010
1 parent 747ba7d commit 20673d7e5836dda504b66730b528a8dae9787493
Showing with 85 additions and 1 deletion.
  1. +17 −1 src/data.js
  2. +68 −0 test/unit/data.js
@@ -1,6 +1,7 @@
(function( jQuery ) {

var windowData = {};
var windowData = {},
rnum = /^-?[0-9.]$/;

This comment has been minimized.

Copy link
@jfirebaugh

jfirebaugh Sep 21, 2010

Maybe translate the JSON grammar for number to tighten this up?

/^-?(0|[1-9][0-9]*)(\.[0-9]+)?([eE][+-]?[0-9]+)?$/ (untested)

jQuery.extend({
cache: {},
@@ -142,8 +143,23 @@ jQuery.fn.extend({
if ( value === undefined ) {
var data = this.triggerHandler("getData" + parts[1] + "!", [parts[0]]);

// Try to fetch any internally stored data first
if ( data === undefined && this.length ) {
data = jQuery.data( this[0], key );

// If nothing was found internally, try to fetch any
// data from the HTML5 data-* attribute
if ( data === undefined && this[0].nodeType === 1 ) {
data = this[0].getAttribute( "data-" + key );

This comment has been minimized.

Copy link
@kangax

kangax Sep 20, 2010

Not using dataset when available?

This comment has been minimized.

Copy link
@jeresig

jeresig Sep 21, 2010

Author Member

dataset appears to camelCase the names - which we don't want in this case (we would want to preserve the names as-is, like we do with CSS properties (e.g. font-size) and DOM attributes (e.g. colspan)). Thus if we wanted to use dataset we would have to check all the incoming names for hyphens, and potentially de-hyphenate them, which is really un-needed overhead. If we were looping over all the possible data-* attributes we would definitely be using dataset.


if ( data != null ) {
data = data === "true" ? true :
data === "false" ? false :
data === "null" ? null :
rnum.test( data ) ? parseFloat( data ) :

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Sep 21, 2010

You can safely test if a string is number-like using !isNaN(str) which covers all js number formats and won't be fooled by strings like "1..2"

This comment has been minimized.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Sep 21, 2010

Interesting results, certainly not what I expected. However, the test isn't fair, the regex (in the test) would fail to spot "6" as a number, and also "1e3", ".5" etc.

However, even given the results on that page, the difference between a single execution is very small and this function isn't likely to be called frequently enough to make a noticeable difference. I'd still go with isNaN which is less code and more accurate.

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Sep 21, 2010

Member

Also in IE6 and IE8, the isNaN test is a lot faster. Makes sense to opt for the slowest engine, the others are ten times faster anyway.

This comment has been minimized.

Copy link
@mdumic

mdumic Sep 21, 2010

@jzaefferer: The test is not fair as rnum is incomplete. Should be like this: /^[+-]?(?:\d+\.?|\d*\.\d+)(?:e[+-]?\d+)?$/i
Take a look at the test: http://jsperf.com/proper-rnum-vs-isnan
It matches anything JS parser considers a number, with the exception of leading and trailing spaces (which would make regex even longer. Imho isNaN is the way to go.

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Sep 21, 2010

Member

Yeah, longer regex makes the IE runtime a lot worse.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Sep 21, 2010

I've submitted a pull request to change this to isNaN and make the tests stricter http://github.com/jakearchibald/jquery/commit/23d59dd6011cd93bfa7f2395a595303ce71bfc95

This comment has been minimized.

Copy link
@jeresig

jeresig Sep 21, 2010

Author Member

@jakearchibald: Oof, too slow ;) 8ebb9b2 and e0b2430

This comment has been minimized.

Copy link
@jakearchibald

This comment has been minimized.

Copy link
@ghost

ghost Sep 21, 2010

One strange consideration with isNaN .... I worked on an app that used Flickr's webservice via YQL. It would return strings like: "1231312e431231" which would be interpreted as Infinity in type guessing that used isNaN. To fix this I added:

! isNaN(data) && (+data) !== Infinity

This comment has been minimized.

Copy link
@kangax

kangax Sep 21, 2010

@jupiterjs That's actually what native isFinite does (also taking -Infinity into consideration, contrary to the example above)

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Sep 21, 2010

Oh cool, didn't know that. I guess it comes down to what's more expected: Returning "1231312e431231" as a string despite it being in the form of a valid number, or returning Infinity, which is a number but not accurate.

This comment has been minimized.

Copy link
@ghost

ghost Sep 21, 2010

Yeah, I think in that case, I'd rather have the string anyway. Infinity is probably just as likely to cause errors in code using the converted value as the value as a string, but at least the string gives you additional info you might use.

data;
}
}
}

return data === undefined && parts[1] ?
@@ -157,6 +157,74 @@ test(".data(String) and .data(String, Object)", function() {
$elem.removeData();
});

test("data-* attributes", function() {
expect(19);
var div = jQuery("<div>"),
child = jQuery("<div data-myobj='old data' data-ignored=\"DOM\"></div>");

equals( div.data("attr"), undefined, "Check for non-existing data-attr attribute" );

div.attr("data-attr", "exists");
equals( div.data("attr"), "exists", "Check for existing data-attr attribute" );

div.data("attr", "internal").attr("data-attr", "external");
equals( div.data("attr"), "internal", "Check for .data('attr') precedence (internal > external data-* attribute)" );

child.appendTo('#main');
equals( child.data("myobj"), "old data", "Value accessed from data-* attribute");

child.data("myobj", "replaced");
equals( child.data("myobj"), "replaced", "Original data overwritten");

child.data("ignored", "cache");
equals( child.data("ignored"), "cache", "Cached data used before DOM data-* fallback");

child
.attr("data-true", "true")
.attr("data-false", "false")
.attr("data-five", "5")
.attr("data-null", "null")
.attr("data-string", "test");

equals( child.data('true'), true, "Primitive true read from attribute");
equals( child.data('false'), false, "Primitive false read from attribute");
equals( child.data('five'), 5, "Primitive number read from attribute");
equals( child.data('null'), null, "Primitive null read from attribute");
equals( child.data('string'), "test", "Typical string read from attribute");

child.remove();

// tests from metadata plugin
function testData(index, elem) {
switch (index) {
case 0:
equals(jQuery(elem).data("foo"), "bar", "Check foo property");
equals(jQuery(elem).data("bar"), "baz", "Check baz property");
break;
case 1:
equals(jQuery(elem).data("test"), "bar", "Check test property");
equals(jQuery(elem).data("bar"), "baz", "Check bar property");
break;
case 2:
equals(jQuery(elem).data("zoooo"), "bar", "Check zoooo property");
equals(jQuery(elem).data("bar"), '{"test":"baz"}', "Check bar property");
break;
case 3:
equals(jQuery(elem).data("number"), true, "Check number property");
equals(jQuery(elem).data("stuff"), "[2,8]", "Check stuff property");
break;
default:
ok(false, ["Assertion failed on index ", index, ", with data ", data].join(''));
}
}

var metadata = '<ol><li class="test test2" data-foo="bar" data-bar="baz" data-arr="[1,2]">Some stuff</li><li class="test test2" data-test="bar" data-bar="baz">Some stuff</li><li class="test test2" data-zoooo="bar" data-bar=\'{"test":"baz"}\'>Some stuff</li><li class="test test2" data-number=true data-stuff="[2,8]">Some stuff</li></ol>',
elem = jQuery(metadata).appendTo('#main');

elem.find("li").each(testData);
elem.remove();
});

test(".data(Object)", function() {
expect(2);

18 comments on commit 20673d7

@akahn

This comment has been minimized.

Copy link

replied Sep 20, 2010

w00t!

@yeco

This comment has been minimized.

Copy link

replied Sep 20, 2010

Nice!

@gercheq

This comment has been minimized.

Copy link

replied Sep 20, 2010

great update! thanks..

@jasonwebster

This comment has been minimized.

Copy link

replied Sep 20, 2010

Shouldn't /^-?[0-9.]$/ be /^-?[0-9\.]+$/ ?

@jeresig

This comment has been minimized.

Copy link
Member Author

replied Sep 21, 2010

@jason - Derp, yep - forgot the + - but the escaping \ isn't needed.

@irae

This comment has been minimized.

Copy link
Contributor

replied Sep 21, 2010

totally makes sense! thanks for the addition!

@jfirebaugh

This comment has been minimized.

Copy link

replied Sep 21, 2010

No parsing of JSON into an object?

@jeremyckahn

This comment has been minimized.

Copy link

replied Sep 21, 2010

I basically understand how this is working, and it looks great! However, there is one thing I'm unclear on. The regular expression "rnum" seems to only match strings that start with numbers between -9 and 9 inclusive, "." and "-.". Why is that type of string important to differentiate from other strings for this functionality?

@gf3

This comment has been minimized.

Copy link
Contributor

replied Sep 21, 2010

@jfirebaugh: I don't think jQuery should have to guess what type of data is stored in the attribute as a string?

@jeremyckahn: It determines if the attribute looks like a number and converts it accordingly. E.g. "-1.27" → -1.27.

@jeremyckahn

This comment has been minimized.

Copy link

replied Sep 21, 2010

@gf3: Thank you for clearing that up, that makes sense. However, isn't that potentially dangerous? It seems that the string "-1.27_not_a_number" would be parsed as a float.

@jeresig

This comment has been minimized.

Copy link
Member Author

replied Sep 21, 2010

@jfirebaugh: That was certainly an option - but the performance overhead of implementing that would've been excessive. It would've resulted in a complete parse of every found attribute - there's no way that it would've been fast enough. Instead I opted to cover some of the common cases (true, false, null, and numbers).

@jfirebaugh

This comment has been minimized.

Copy link

replied Sep 21, 2010

Detection of JSON was part of the original proposal and patch by Paul Irish, and is necessary if this is intended to be a full replacement for the metadata plugin.

The cool thing about how the metadata plugin works is how it integrates with jQuery UI widgets. For example, if I'm using the jQuery button widget and I want to give my buttons different icons, I can do the following:

<button data-button='{"icons": {"primary": "ui-icon-plus"}}'>New</button>
<button data-button='{"icons": {"primary": "ui-icon-trash"}}'>Delete</button>

$('button').button();

And both buttons automatically get the right icon. In other words, I can supply all the widget options in the data attribute, serialized as JSON, and make only a single call to instantiate everything.

@jfirebaugh

This comment has been minimized.

Copy link

replied Sep 21, 2010

@jeresig: Could we try the JSON parse only if the value starts with '{' and ends with '}'?

@gf3

This comment has been minimized.

Copy link
Contributor

replied Sep 21, 2010

@jfirebaugh: In other other words you can supply presentation data in functionality data in information data. Hmmm....

@jfirebaugh

This comment has been minimized.

Copy link

replied Sep 21, 2010

@jeresig

This comment has been minimized.

Copy link
Member Author

replied Sep 21, 2010

@jeremyckahn: The RegExp isn't that permissive, it'll reject strings like "1.27foo" because it has a non-number in it. The RegExp isn't perfect though - it'll try to handle things like "1..2" and "-." so I may want to tweak it some.

@jfirebaugh: That might be a possibility, I can check in to it. As it was originally implemented by Paul + co., though, it was blindly running parseJSON on every attribute, which just wasn't good.

@jeremyckahn

This comment has been minimized.

Copy link

replied Sep 21, 2010

@jeresig: That makes sense. Thanks for the explanation!

@mdumic

This comment has been minimized.

Copy link

replied Sep 21, 2010

@jfirebaugh, @jeresig: Proper JSON can be an array too. I.e. starting with '[' and ending with ']', so rbrace should be adjusted accordingly: rbrace = /^(?:\{.*\}|\[.*\])$/;

Please sign in to comment.
You can’t perform that action at this time.