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 comment:1
Changed June 26, 2010 06:02AM UTC by 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 comment:3
component: | core → attributes |
---|---|
priority: | major → low |
resolution: | → invalid |
status: | new → closed |
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 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 comment:5
keywords: | → 1.8-discuss |
---|---|
milestone: | 1.4 → 1.next |
resolution: | invalid |
status: | closed → reopened |
Reopening for 1.8 discussion -- should we have uniform behavior for .method(undefined/null)
?
Changed November 10, 2011 04:07PM UTC by comment:6
status: | reopened → open |
---|
Changed November 18, 2011 07:07AM UTC by comment:7
Changed November 18, 2011 04:48PM UTC by 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 comment:9
Changed November 18, 2011 08:02PM UTC by 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 comment:11
Please pardon my ignorance, but where does that discussion take place?
Changed November 20, 2011 10:40PM UTC by comment:12
summary: | attr() invalid argument length check → Allow chaining when passing undefined to any setter in jQuery |
---|
Changed November 22, 2011 07:43PM UTC by comment:13
#10865 is a duplicate of this ticket.
Changed November 23, 2011 02:24PM UTC by comment:14
type: | bug → enhancement |
---|
Changed November 23, 2011 02:24PM UTC by comment:15
#10874 is a duplicate of this ticket.
Changed November 23, 2011 02:26PM UTC by 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 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 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 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 comment:20
resolution: | → fixed |
---|---|
status: | open → closed |
Fix #5571. Setters should treat undefined
as a no-op and be chainable.
Changeset: 6c2a501de40a5f6b3ad382e2d309e5a10fce04d0
Changed December 10, 2011 06:29PM UTC by comment:21
keywords: | 1.8-discuss |
---|---|
milestone: | 1.next → 1.7.2 |
Changed April 04, 2012 12:27AM UTC by 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 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 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 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 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 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 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 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" ); };
Should setting an attribute to
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 but then what to do with the bad value?