Allow chaining when passing undefined to any setter in jQuery

[jdsharp]

When calling .attr('attribute', undefined) will break the chain. Currently attr tests the value being set to determine whether to set or get the value. It should test arguments.length to determine the get/set.

[dmethvin]

Should setting an attribute to undefined do something special? It seems like it's an error, in which case breaking the chain is just a cascaded result of the original error. It would definitely be possible to check arguments.length but then what to do with the bad value?

[tauren]

Personally, I think that arguments.length should be checked to see if .attr('attribute',value) has a second argument. If the argument is undefined, it should be allowed and just ignored. The chain should not be broken.

I'm using JSON data that I don't control and currently have to wrap my jquery chain with an if(value) to prevent the chain from failing. Having this command not break the chain would simplify the code.

[Rick Waldron]

There is no attribute that accepts undefined. The docs do not cover passing undefined to attr(). Closing as invalid

[Xavi]

IMO, passing two arguments to $.fn.attr should never break the chain. This makes the API more consistent and provides the least surprise to the user.

As for the issue of what to do with the value, I would suggest throwing an exception. Breaking the chain is a very bad form of error reporting. First of all it doesn't also work. For example:

var url;
$(".foo").attr("src", url); // Doesn't throw exception (i.e. error isn't reported)

Second of all, breaking the chain leads to very bad error mesages:

var url;
// Throws "TypeError: Cannot call method 'addClass' of undefined"
$(".foo").attr("src", url).addClass("foo");

This error message is misleading and confusing. Throwing exception explaining that attributes can't be set to the value of undefined would be much more helpful, IMO.

[dmethvin]

Reopening for 1.8 discussion -- should we have uniform behavior for .method(undefined/null) ?

[gibson042]

​https://github.com/jquery/jquery/pull/606

[gibson042]

PR withdrawn because I misunderstood comment 5.

I would like to define a rule that providing any value argument places .attr etc. into a "set" mode with consistent (chainable) return values and overrideable default behavior to no-op if value is undefined. (.css does this explicitly right now).

Unless someone objects, I'll probably submit a new pull request by Monday.

[gibson042]

​https://github.com/jquery/jquery/pull/608

[Rick Waldron]

Please hold off on the pull requests. This has been marked for discussion ("1.8-discuss") not immediately patching.

[gibson042]

Please pardon my ignorance, but where does that discussion take place?

[Timmy Willison]

#10865 is a duplicate of this ticket.

[Timmy Willison]

#10874 is a duplicate of this ticket.

[Timmy Willison]

+0, I'm fine either way, but it should be consistent. Currently, irrc, only .css() allows chaining with undefined.

[ajpiano]

+0, If this doesn't break existing code - which I can't foresee it doing, because anyone who'd previously been passing undefined and intending for it to work as a setter would have run into this issue and worked around it already. On the other hand, what are we actually supposed to do when we get an undefined? Ignore it in CSS and attr? Set it to undefined in data? Skip it all the time?

[Rick Waldron]

-1, I'll change this to a "plus one" if/when the request can prove that it doesn't add a single a byte of new code or break an existing expectations - otherwise I strongly oppose.

[gibson042]

​https://github.com/jquery/jquery/pull/608

jquery.js: -271 bytes
jquery.min.js: -377 bytes

Executive summary:
Passing value = undefined to any combined getter/setter method (of which I found 12) will have no effect on the object but is always chainable. Previously, only .css operated in this fashion.

[Richard Gibson]

Fix #5571. Setters should treat undefined as a no-op and be chainable.

Changeset: 6c2a501de40a5f6b3ad382e2d309e5a10fce04d0

[davydotcom@…]

Aand this broke attr as a getter

i.e. $('selector').attr('data-source') returns array of elements (chain) instead of returning the value of data-source.

[dmethvin]

I'll bet you're using TinyMCE, which incorrectly patches jQuery.attr. They have a beta that fixes it. jQuery itself isn't causing the problem.

[anonymous]

Replying to davydotcom@…:

Aand this broke attr as a getter

i.e. $('selector').attr('data-source') returns array of elements (chain) instead of returning the value of data-source.

Related to another story and caused by TinyMCE. Whoops.

[cowboy]

It's unfortunate that this change was implemented in a patch release, as it changes the behavior of any existing code that happens to look like this, so that it can no longer behave as a getter:

jQuery.fn.href = function(href) {
  return this.prop("href", href);
};

Implementing a "shortcut method" can no longer be done in this manner, and now requires a much more verbose solution:

jQuery.fn.href = function(href) {
  if (arguments.length === 0) {
    return this.prop("href");
  } else {
    return this.prop("href", href);
  }
};

// Or

jQuery.fn.href = function(href) {
  return arguments.length === 0 ? this.prop("href") : this.prop("href", href);
};

// Or

jQuery.fn.href = function() {
  return this.prop.apply(this, ["href"].concat([].slice.call(arguments, 0)));
};

What used to be a trivial task now becomes much more daunting for the average jQuery user. Every example requires knowledge of arguments which is not something the average jQuery user will necessarily understand.

Is there anything that can be done to tweak this new "write more, do less" behavior so that it's a little more user-friendly in these cases?

[cowboy]

With the new "testing arguments.length" behavior, calling a method like prop with a value that might be null or undefined is very terse, but creating a "shortcut" method becomes much more complex.

// "Regular" call with a legitimate null/undefined value:
$("div").prop("href", value);

// "Shorcut" method:
jQuery.fn.href = function(href) {
  if (arguments.length === 0) {
    return this.prop("href");
  } else {
    return this.prop("href", href);
  }
};

With the old "testing value == null" behavior, a test must be done before calling prop if a value might be null or undefined (which makes chaining difficult), but creating a "shortcut" method is very terse.

// "Regular" call with a legitimate null/undefined value:
if (value != null) {
  $("div").prop("href", value);
}

// "Shorcut" method:
jQuery.fn.href = function(href) {
  return this.prop("href", href);
};

I apologize for just now noticing this, it slipped right past me.

[Rick Waldron]

@cowboy I recommend subscribing to ticket updates, that way you'll always be "in the know".

FWIW, I was, and remain, opposed to this change.

[dmethvin]

The .href() plugin was depending on a behavior that we never defined, and that varied from method to method. We had slowly been changing over to treat undefined/null values as chainable methods, but it wasn't consistent.

In retrospect, I should have waited until 1.8 to land this, but I still think it's the right way to handle the API. This way, if you see a syntactic call of .attr(name, value) you KNOW it's setter semantics and safely chainable, even if the value is undefined or null.

As I understood @rwaldron's original vote, the objection was to a size increase, which gibson042 addressed with a really nice size reduction that made our treatment of arguments the same throughout the library.

[gibson042]

I don't see much value in such shortcut methods, but if you want one to emulate old behavior with our now-uniform API then the best pattern I can think of is

jQuery.fn.href = function( href ) {
  return href !== undefined ?

    // force setter behavior
    this.prop( "href", href ) :

    // force getter behavior
    this.prop( "href" );
};