Regression: jQuery 2.0.0b2 bugs with $.fn.data handling parameters with dashes - next part

[m_gol]

A followup to bug #13548. The fix didnt't handle the situation where we use .data(key) between setting the attribute and setting the data. Currently the following still behaves incorrectly:

$('body').attr('data-long-param', 'test');
$('body').data('long-param');
$('body').data('long-param', {a: 2});
$('body').data('long-param');

[m_gol]

I think the key is the data_user.set( elem, key, data ) invocation where key should be changed to camelKey. I reported the pull request and added a new test against that: ​https://github.com/jquery/jquery/pull/1189 Bare with me, this is my first pull request on to jQuery so I might not know all the procedures...

[Rick Waldron]

Before jumping into another patch, can you provide a fiddle that illustrates the problem behaviour? The new data system was written in conformance with all of our pre-existing expectations for the documented API, I want to make sure we're not pushing into "new turf"—which can happen if an API was "accidentally" allowing some use case

[m_gol]

Sure, here it is: ​http://jsfiddle.net/m_gol/6Rqwj/9/

[m_gol]

As for where I stumbled upon this problem - in the codebase I'm talking about there's a jQuery plugin which is initialized in the following way (I mean, the code is much bigger, I extracted relevant parts):

function Tabs(element) {
	var that = this;
	this.$element = $(element);
	this.$element.find('.action-switch-tab').each(function (i, link) {
		var $link = $(link);
		var targetSelector = $link.data('tabs-target');
		$link.data('tabs-target', that.$element.find(targetSelector));
	});
}

After that, I expect to have a copy of the target jQuery object in $link.data('tabs-target'). However, just reading this data using this parameter causes the code to create a 'tabs-target' data field which causes the tabsTarget field to not even be read.

Sure, I could write var targetSelector = $link.attr('data-tabs-target') instead but current code is more flexible - we can have elements set for this plugin via JS and it works even if there was no data-tabs-target attribute.

[m_gol]

Anyway, regardless of what's expected of the data API and what's not, without using a very hacky code (which I'd argue it's not in this fiddle and the example from my previous comment), there shouldn't be a situation where setting the data and reading it later returns the initial result, not the last one, am I right?

[Rick Waldron]

Looks like it works correctly: ​http://jsfiddle.net/rwaldron/HDmdq/

[m_gol]

For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.

[Rick Waldron]

jQuery edge

This is the code on github.

Anyway, regardless of what's expected of the data API and what's not, without using a very hacky code (which I'd argue it's not in this fiddle and the example from my previous comment), there shouldn't be a situation where setting the data and reading it later returns the initial result, not the last one, am I right?

We only read the data-* attributes once, but I was certain we had all the edge cases covered.

[Rick Waldron]

For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.

I just realized the confusion: the patch I landed won't be in jQuery 2.0.0b2, it will be in 2.0.0b3, to test against the latest code: ​http://code.jquery.com/jquery-git2.js

[m_gol]

jQuery edge points to 1.9.2pre, not 2.0.0pre.

[m_gol]

With 2.0.0pre your test case returns false: ​http://jsfiddle.net/m_gol/HDmdq/6/

[m_gol]

My latest test case updated to point to jquery-git2.js: ​http://jsfiddle.net/m_gol/6Rqwj/13/

Note that there are both long-param and longParam keys in the printed object and .data('long-param') picks up the former. My pull request fixes that.

It also makes the code more consistent. In current jQuery master if you invoke $object.data(), the HTML5 data-* attributes are set using the following code:

if ( name.indexOf( "data-" ) === 0 ) {
	name = jQuery.camelCase( name.substring(5) );
	dataAttr( elem, name, data[ name ] );
}

(lines 235-238 of src/data.js). However, if you use $object.data('long-param'), it's invoked in this way:

data = dataAttr( elem, key, undefined );

Changing it from key to camelKey makes it consistent with the first code snippet, thus my pull request.

[Rick Waldron]

jQuery edge points to 1.9.2pre, not 2.0.0pre.

Right, I completely forgot that there is a special jquery-git2.js—also explains why the bug wasn't present. I'll take a look at this today, thanks for the report and the follow up

[m_gol]

Replying to rwaldron:

jQuery edge points to 1.9.2pre, not 2.0.0pre.

Right, I completely forgot that there is a special jquery-git2.js—also explains why the bug wasn't present.

Maybe it's worth to ping jsFiddle guys to provide the "jQuery 2 edge" option, it's needed for tests like that.

I'll take a look at this today, thanks for the report and the follow up

Great, thanks!

[Michał Gołębiowski]

Fixes #13550. .data should not miss attr() set data-* with hyphenated property names. Closes gh-1189

Changeset: 761b96c3016e09613f356bd6070be04edec74d0f