Skip to main content

Bug Tracker

Side navigation

#5571 closed enhancement (fixed)

Opened November 30, 2009 05:12PM UTC

Closed December 06, 2011 10:21PM UTC

Last modified May 02, 2012 03:24PM UTC

Allow chaining when passing undefined to any setter in jQuery

Reported by: jdsharp Owned by:
Priority: low Milestone: 1.7.2
Component: attributes Version: 1.3.2
Keywords: Cc:
Blocked by: Blocking:
Description

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.

Attachments (0)
Change History (29)

Changed December 16, 2009 01:35AM UTC by dmethvin comment:1

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?

Changed June 26, 2010 06:02AM UTC by tauren comment:2

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.

Changed October 22, 2010 10:11PM UTC by rwaldron comment:3

component: coreattributes
priority: majorlow
resolution: → invalid
status: newclosed

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

Changed May 26, 2011 03:24AM UTC by Xavi comment:4

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:

#!js
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:

#!js
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.

Changed November 10, 2011 04:07PM UTC by dmethvin comment:5

keywords: → 1.8-discuss
milestone: 1.41.next
resolution: invalid
status: closedreopened

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

Changed November 10, 2011 04:07PM UTC by dmethvin comment:6

status: reopenedopen

Changed November 18, 2011 07:07AM UTC by gibson042 comment:7

Changed November 18, 2011 04:48PM UTC by gibson042 comment:8

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.

Changed November 18, 2011 07:51PM UTC by gibson042 comment:9

Changed November 18, 2011 08:02PM UTC by rwaldron comment:10

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

Changed November 20, 2011 11:10AM UTC by gibson042 comment:11

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

Changed November 20, 2011 10:40PM UTC by timmywil comment:12

summary: attr() invalid argument length checkAllow chaining when passing undefined to any setter in jQuery

Changed November 22, 2011 07:43PM UTC by timmywil comment:13

#10865 is a duplicate of this ticket.

Changed November 23, 2011 02:24PM UTC by timmywil comment:14

type: bugenhancement

Changed November 23, 2011 02:24PM UTC by timmywil comment:15

#10874 is a duplicate of this ticket.

Changed November 23, 2011 02:26PM UTC by timmywil comment:16

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

Changed November 23, 2011 03:47PM UTC by ajpiano comment:17

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

Changed November 23, 2011 05:12PM UTC by rwaldron comment:18

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

Changed November 30, 2011 10:11PM UTC by gibson042 comment:19

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.

Changed December 06, 2011 10:21PM UTC by Richard Gibson comment:20

resolution: → fixed
status: openclosed

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

Changeset: 6c2a501de40a5f6b3ad382e2d309e5a10fce04d0

Changed December 10, 2011 06:29PM UTC by dmethvin comment:21

keywords: 1.8-discuss
milestone: 1.next1.7.2

Changed April 04, 2012 12:27AM UTC by davydotcom@gmail.com comment:22

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.

Changed April 04, 2012 12:28AM UTC by dmethvin comment:23

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.

Changed April 04, 2012 12:35AM UTC by anonymous comment:24

Replying to [comment:22 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.

Changed May 02, 2012 05:34AM UTC by cowboy comment:25

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?

Changed May 02, 2012 05:57AM UTC by cowboy comment:26

_comment0: With the new "testing arguments.length" behavior, calling a method like `prop` with a value that might be null 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.1335938300857880

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.

Changed May 02, 2012 12:38PM UTC by rwaldron comment:27

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

Changed May 02, 2012 12:56PM UTC by dmethvin comment:28

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.

Changed May 02, 2012 03:24PM UTC by gibson042 comment:29

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" );
};