Bug Tracker

Modify

Ticket #5571 (closed enhancement: fixed)

Opened 4 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

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.

Change History

comment:1 Changed 4 years ago by 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?

comment:2 Changed 4 years ago by 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.

comment:3 Changed 3 years ago by rwaldron

  • Priority changed from major to low
  • Resolution set to invalid
  • Status changed from new to closed
  • Component changed from core to attributes

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

comment:4 Changed 3 years ago by 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.

comment:5 Changed 2 years ago by dmethvin

  • Keywords 1.8-discuss added
  • Status changed from closed to reopened
  • Resolution invalid deleted
  • Milestone changed from 1.4 to 1.next

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

comment:6 Changed 2 years ago by dmethvin

  • Status changed from reopened to open

comment:8 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by rwaldron

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

comment:11 Changed 2 years ago by gibson042

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

comment:12 Changed 2 years ago by timmywil

  • Summary changed from attr() invalid argument length check to Allow chaining when passing undefined to any setter in jQuery

comment:13 Changed 2 years ago by timmywil

#10865 is a duplicate of this ticket.

comment:14 Changed 2 years ago by timmywil

  • Type changed from bug to enhancement

comment:15 Changed 2 years ago by timmywil

#10874 is a duplicate of this ticket.

comment:16 Changed 2 years ago by timmywil

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

comment:17 Changed 2 years ago by 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?

comment:18 Changed 2 years ago by rwaldron

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

comment:19 Changed 2 years ago by 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.

comment:20 Changed 2 years ago by Richard Gibson

  • Status changed from open to closed
  • Resolution set to fixed

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

Changeset: 6c2a501de40a5f6b3ad382e2d309e5a10fce04d0

comment:21 Changed 2 years ago by dmethvin

  • Keywords 1.8-discuss removed
  • Milestone changed from 1.next to 1.7.2

comment:22 follow-up: ↓ 24 Changed 2 years ago by 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.

comment:23 Changed 2 years ago by 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.

comment:24 in reply to: ↑ 22 Changed 2 years ago by 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.

comment:25 Changed 2 years ago by 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?

comment:26 Changed 2 years ago by cowboy

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.

Version 0, edited 2 years ago by cowboy (next)

comment:27 Changed 2 years ago by rwaldron

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

comment:28 Changed 2 years ago by 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.

comment:29 Changed 2 years ago by 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" );
};

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.