Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13529 closed bug (notabug)

$.ajax() Deferred does not pass json data

Reported by: martijn@… Owned by: martijn@…
Priority: high Milestone: None
Component: ajax Version: 1.9.1
Keywords: Cc:
Blocked by: Blocking:

Description

When calling a (proper) json handler that returns data (obviously), I'm doing that using the $.ajax() method, like this:

$.ajax({
  cache: false,
  data: { foo: "bar" },
  dataType: "json",
  type: "POST",
  url: "some-url-that-returns-something.json",
  success: function(data) {
    //Yay json data!
  }
}).complete(function(data) {
  //Omg my json data's not here!
});

For illustration purposes, I'm using both the success callback of the $.ajax function, and the complete function of the $.Deferred interface. In the real world, I prefer to use the latter only.

But in the latter case, the json data is no longer accessible... It's not in this, not in any of the arguments, not in any properties of any of the arguments. Yesss, there's data.responseText, but that's just a string and not json data. Yesss, I can parse it yet again, but it's already been parsed (and potentionally gone through a converter if supplied) so doing it again is pointless.

So, I say this is a bug. Which browser doesn't matter, but I tried in Firefox 19.

Change History (10)

comment:1 Changed 7 years ago by dmethvin

Owner: set to martijn@…
Status: newpending

Are you saying there is a bug in that it's not working as documented? Can you point to the documentation that says the .complete() method on a jQXHR should be passed the data as its first argument?

comment:2 in reply to:  1 Changed 7 years ago by jaubourg

Replying to dmethvin:

Are you saying there is a bug in that it's not working as documented? Can you point to the documentation that says the .complete() method on a jQXHR should be passed the data as its first argument?

In case of a success, it should, it is unit tested afaik. What I'd like to see is an actual test case.

Also, to the OP, why don't you use .done rather than .complete?

comment:3 Changed 7 years ago by jaubourg

@dmethvin, scrap that, I mixed things with .always :/

comment:4 Changed 7 years ago by timmywil

Component: unfiledajax
Priority: undecidedhigh
Version: git1.9.1

Actually, this hit me in real code as well. I'd be for passing the parsed JSON to complete. I ended up re-parsing. Btw, I don't think the OP is saying the docs aren't clear, just that the current behavior is neither intuitive or efficient(since the JSON has already been parsed and correctly passed to success) in terms of dealing with JSON responses in the complete callback. I gotta go with the OP on this one.

Last edited 7 years ago by timmywil (previous) (diff)

comment:5 Changed 7 years ago by timmywil

Well, now that I think more about the implementation, we may not be able to guarantee the JSON would always be there since the complete callback is also called on failure (when there may not be any JSON to parse). So to make parameters consistent, it may have to stay inefficient, amd admittedly confusing.

Still, the data has been parsed so it would be reasonable to assume the parsed data would be available when you hit always. We _could_ perhaps tack on data as a 3rd argument when available and leave it up to the user to validate callback arguments. I'd be down for that, but if arguments are not validated and JSON is expected, complete callbacks will throw an error whenever there is a failure that does not provide JSON. In my opinion, a reasonable risk taken for the sake of convenience and overall performance, but others may not agree.

comment:6 Changed 7 years ago by dmethvin

There are a few things going on here, all tangled into one "bug report" that isn't a bug but more a series of documented but unfortunate coincidences. I think this is all working as documented:

1) The .complete() method of jQXHR is deprecated, and the .always() method of $.Deferred takes its place.

2) The creator of a $.Deferred() gets the opportunity to define what arguments are passed to .done() and .fail() handlers. They do not have to be identical and in fact often are not.

3) As a result of (2) the .always() handler cannot be guaranteed it will get a consistent set of arguments. The $.Deferred() creator must ensure it by design.

4) To simplify migration from the old-style callbacks and because there was no good reason to change it, we used the same argument lists for the .done() and .fail() methods in 1.5. At this point the trap was set.

5) In 1.6 we added the .always() method, whose added handlers are always called and passed the arg list of the .resolve() or .reject() call. So we get this signature:

jqXHR.always(function(data|jqXHR, textStatus, jqXHR|errorThrown) { });

6) The .always() method isn't guaranteed to have the converted data, either the HTTP request or data conversion may have failed. But if it succeeded, the data should be the first argument. You will need to figure out whether it is the data you wanted or a jQXHR by looking at the third arg to see if it is a string.

7) Yeah.

Error case: http://jsfiddle.net/dmethvin/WVrxw/1/

Success case: http://jsfiddle.net/dmethvin/WVrxw/2/

comment:7 Changed 7 years ago by jaubourg

We're over-thinking this... a lot. The OP clearly should use .done because his intention is to get the parsed data that is only present in case of a success. Using .complete is bad practice here, plain and simple.

Now, we discussed the order of arguments for success and error handlers ad nauseum before: we just cannot change them without breaking every single app in the known universe.

Also, as a side note, you can make sure the json data is on the jqXHR instance by doing so:

$.ajaxSetup({
  responseFields: {
    "json": "responseJSON"
  }
});

Now, everytime you request json successfully, the jqXHR instance will have the responseJSON field set properly.

Also, if we finally merge PR1164, this is fixed because the responseJSON field is added to the jqXHR instance there and it will be there for the error case if the data is proper json to boot.

comment:8 Changed 7 years ago by timmywil

Resolution: notabug
Status: pendingclosed

I'm not sure why I was assuming the old complete was just an alias for always. That wouldn't work anyway, considering the difference in arguments. Thank you jaubuorg for making it clear yet again that you have it covered. I need to read all the ajax docs again. Forgive my ignorance.

As a side note, I'm in favor of landing 1164.

comment:9 in reply to:  8 Changed 7 years ago by jaubourg

Replying to timmywil:

I'm not sure why I was assuming the old complete was just an alias for always. That wouldn't work anyway, considering the difference in arguments. Thank you jaubuorg for making it clear yet again that you have it covered. I need to read all the ajax docs again. Forgive my ignorance.

Forgive the docs... I don't remember if responseFields is even mentionned. Also, Dave did a tremendous job at summarizing the madness we got ourselves into with those signatures ;)

As a side note, I'm in favor of landing 1164.

Yeah, I really gotta get to it. It's just that I'm switching tasks like crazy these days.

comment:10 Changed 7 years ago by anonymous

Just shows how confusing $.Deferred can be. There are like 4 or 5 ways to get a "done" signal (complete, always, done, then, and probably more), and they're all ever so slightly different. I will try to remember to stop using complete. It's not (no longer?) in the docs, so I guess that's a good thing.

Still it's weird that the parsed JSON is nowhere to be found in this function. I will stick to that conclusion, no matter how deprecated complete is.

Note: See TracTickets for help on using tickets.