Bug Tracker

Ticket #10944 (closed bug: fixed)

Opened 3 years ago

Last modified 2 years ago

$.ajax does not always return an object implementing the Promise interface

Reported by: fastfasterfastest Owned by:
Priority: low Milestone: 1.8
Component: ajax Version: 1.7.1
Keywords: 1.8-discuss Cc:
Blocking: Blocked by:

Description (last modified by rwaldron) (diff)

The documentation of $.ajax states "The jqXHR objects returned by $.ajax() as of jQuery 1.5 implement the Promise interface...". This is not true in 1.7.1. As a consequence one cannot (safely) use the Promise programming model on the object returned by $.ajax.

If you use a beforeSend callback and the callback returns false, then $.ajax does not return an object that implements the Promise interface - $.ajax instead returns a boolean.

E.g.,  http://jsfiddle.net/67DJr/

$.ajax( '/echo/html' , { beforeSend: function(){ return confirm('Access resource?'); } })
    .done(function() { alert("success"); })
    .fail(function() { alert("error"); })
    .always(function() { alert("complete"); });

The above code causes a javascript error if user clicks Cancel.

Per documentation, $.ajax should always return an object that implements the Promise interface. In the case of beforeSend returning false, and thus cancelling or preventing the call from being made, it seems to make sense for $.ajax to return a Promise object that has been rejected.

Change History

comment:1 Changed 3 years ago by sindresorhus

  • Priority changed from undecided to low
  • Status changed from new to open
  • Component changed from unfiled to ajax

comment:2 Changed 3 years ago by fastfasterfastest

Rather than returning a Promise object that has been rejected, it may make more sense to return a Promise object that is in "pending" state and that will never be resolved nor rejected.

comment:3 Changed 3 years ago by jaubourg

  • Keywords 1.8-discuss added

This is a backward compatibility issue. 1.4.x already returned false when beforeSend returned false.

I agree returning a rejected promise would be better, especially given $.when( false ) would give a resolved promise and cannot be used to "save the day" in this situation.

However, such a change would break existing code (though I dunno how many people rely on the existing behaviour -- our unit tests do at least).

I'll tag this as open for discussion for 1.8.

Last edited 3 years ago by jaubourg (previous) (diff)

comment:4 follow-up: ↓ 5 Changed 3 years ago by fastfasterfastest

One issue with returning a rejected promise is handling of errors.

Currently, the "fail functions" of an ajax promise will be called only if the ajax call errored. If $.ajax returns a rejected promise when beforeSend returns false, then those "fail functions" would be called as well - and they would (likely) need to determine when/why they got called (e.g. to present some information to the user), was it because beforeSend returned false or was it because the $.ajax call failed.

Also, if a rejected promise is returned when beforeSend returns false, then the "fail functions" of the ajax promise will be called, but the error callback in the ajax settings will not be called. I.e.:

$.ajax( '/echo/html' , {
    beforeSend: function(){ return confirm('Access resource?'); },
    error: function() { alert("will not be called if beforeSend returns false"); }
})
.fail(function() { alert("will be called if beforeSend returns false });

That would be a little "inconsistent" with the current behavior where the error callback in the ajax settings and the promise's fail functions will either all be called, or none will be called, depending on whether the ajax call succeeds.

Therefore, I think it may be better to return a promise that will never be resolved nor rejected if beforeSend returns false.

(Another possible alternative would be to no longer be able to cancel the call by beforeSend returning false, i.e. beforeSend's return value would be ignored.)

comment:5 in reply to: ↑ 4 Changed 3 years ago by jaubourg

Well, how useful would a pending Promise be? Do we really want to force users to check the Promise's state in order to determine what happened?

I'm not that shocked by the inconsistency of having fail callbacks added later on called whereas ones given in the error options were not. I'd rather see this as a feature.

Ignoring the return value of beforeSend is clearly not an option here.

And btw, same goes when aborting in a prefilter.

comment:6 Changed 3 years ago by dmethvin

Seems like a rejected promise would be the way to go, although I think we would want to document some way to determine it was rejected due to the beforeSend callback returning false. I agree a forever-pending state is not good.

comment:7 Changed 3 years ago by jzaefferer

  • Description modified (diff)

+1, Could be considered a bug.

comment:8 Changed 3 years ago by jaubourg

+1, but we will break existing code with this.

comment:9 Changed 3 years ago by dmethvin

  • Description modified (diff)

+1, to a rejected Promise. We'll need to document the change.

comment:10 Changed 3 years ago by fastfasterfastest

Assuming a rejected promise will be returned:

  • Should $.ajax error callback be called or not?

If it will be called, then that would be another backward compatibility issue since the error callback is currently not called when beforeSend returns false.

  • With what values should the promise be rejected with?

It would (probably) be useful to reject it with some value to allow the fail functions to determine if the rejection was due to beforeSend or whether the rejection was due to something that happened during the http request. Currently an $.ajax promise is rejected with three parameters: jqXHR, textStatus, errorThrown. Perhaps it should be rejected with a particular textStatus value when beforeSend returns false. E.g. "beforesend" or "requestcancelled" (since a prefilter apparently might cause the call to be not-made as well). Perhaps the currently used "abort" may cover it - I am not 100% sure under what circumstances "abort" is used today.

comment:11 Changed 3 years ago by mikesherov

  • Description modified (diff)

+1, we need to keep our promises when we say it in the docs. pun intended.

comment:12 Changed 3 years ago by timmywil

+1

comment:13 Changed 3 years ago by rwaldron

  • Description modified (diff)

+1

comment:14 Changed 3 years ago by doom777

+1, caused me an hour of headache + need to put special

var ret = jQuery.get(url);
if(ret === false)
 handleErr();
else
 ret.success(handleSuccess).error(handleErr)

everwhere in the code until you fix it

comment:15 Changed 2 years ago by jaubourg

#9203 is a duplicate of this ticket.

comment:16 Changed 2 years ago by anonymous

When is this going to get fixed already? Deferred objects have a clear success/fail architecture, and those who use methods that return deferred objects, rely on that. If beforeSend returns false, then the request failed and should deferred.reject Classic case, I can't comprehend why someone would want .ajax() to return false sometimes.

comment:17 Changed 2 years ago by rwaldron

I think there is some concern that remains unresolved:

jaubourg +1, but we will break existing code with this.

comment:18 Changed 2 years ago by jaubourg

Well, it is in 1.8pre.

Current implementation doesn't fire success, error and complete handlers (to be backward compatible). However newly added handlers using done/fail/complete methods will be called subsequently. Status of the error is "cancelled".

Last edited 2 years ago by jaubourg (previous) (diff)

comment:19 Changed 2 years ago by jaubourg

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

$.ajax now always returns an object implementing the Promise interface. Fixes #10944. Unit tests amended. For back-compat, in case of an early abort, callbacks passed in the options are not called (while subsequent callbacks attached to the returned Promise are). For early abort triggered by returning false in beforeSend, statusText is "canceled".

Changeset: 395612bb152660226b93f7f096cd0146fc06991e

comment:20 Changed 2 years ago by dmethvin

  • Milestone changed from None to 1.8
Note: See TracTickets for help on using tickets.