Bug Tracker

Ticket #13292 (closed bug: fixed)

Opened 2 years ago

Last modified 11 months ago

$.ajax with 1.9.0 doesn't call anymore success function in case of 204

Reported by: olamy@… Owned by: jaubourg
Priority: blocker Milestone: 1.9.1
Component: ajax Version: 1.9.0
Keywords: Cc: jaubourg
Blocking: Blocked by:

Description

Using $.ajax success function. In case of 204 (No content) success function is not called anymore

Change History

comment:1 Changed 2 years ago by anonymous

workaround is to add

statusCode: {

204: function() {

}

}

But backward comp breaks and AFAIK nothing in the release notes.

comment:2 Changed 2 years ago by dmethvin

  • Cc jaubourg added

@jaubourg can you take a look? Seems like it should be calling the success callback.

comment:3 Changed 2 years ago by anonymous

comment:4 follow-up: ↓ 7 Changed 2 years ago by anonymous

comment:5 follow-up: ↓ 6 Changed 2 years ago by jaubourg

Could we have a test case that doesn't use mockjax? Because the test case in #13261 doesn't fail if and when you remove the contentType option. It doesn't make any sense since the option is request-related not response-related. So I really think something's fishy here.

The mockjax repo says "The current version of Mockjax has been tested with jQuery 1.3.2 through 1.7.0 with QUnit unit tests, residing in /test." While it's more than likely that it has been tested with earlier versions, it wouldn't hurt to check.

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 2 years ago by anonymous

Replying to jaubourg:

Could we have a test case that doesn't use mockjax? Because the test case in #13261 doesn't fail if and when you remove the contentType option. It doesn't make any sense since the option is request-related not response-related. So I really think something's fishy here.

Unfortunately jsFiddle can't be used to respond with a 204/No Content.

You might want to have a look at the jQuery function ajaxConvert which takes the request into account when creating the converters. This might be the magic thing happening behind the scenes - I don't believe it's related to mockjax.

Also, the Content-Type header in the request is correct since - in my case - the request sends some JSON, it's just the fact that the response is empty.

Is is possible that jQuery expects a JSON response if JSON has been sent?

comment:7 in reply to: ↑ 4 Changed 2 years ago by anonymous

Replying to anonymous:

Pull request:  https://github.com/jquery/jquery/pull/1142

+1 That's exactly what should happen!

comment:8 in reply to: ↑ 6 Changed 2 years ago by jaubourg

Unfortunately jsFiddle can't be used to respond with a 204/No Content.

Could you then provide a test case somewhere on the web? A URL on a server of yourse maybe.

You might want to have a look at the jQuery function ajaxConvert which takes the request into account when creating the converters. This might be the magic thing happening behind the scenes - I don't believe it's related to mockjax.

Everything is in "I don't believe". Have you reproduced this without mockjax? If yes, could you provide a url demonstrating the issue?

Also, the Content-Type header in the request is correct since - in my case - the request sends some JSON, it's just the fact that the response is empty.

I'm not saying it's not correct. I'm just saying it doesn't make sense in relation to the bug. contentType simply sets a *request* header: it doesn't change anything regarding the response. That dataType would have an impact, I would understand, but contentType? Hence why I suspect a problem with mockjax and why I'd like to have a clean, pure ajax example.

Is is possible that jQuery expects a JSON response if JSON has been sent?

In jQuery? As per the previous paragraph: no, it is not. I dunno if there is some logic like this in mockjax.

Pull request:  https://github.com/jquery/jquery/pull/1142

+1 That's exactly what should happen!

Except we never had a test specific to 204 at that level in the code before so the (eventual) regression is elsewhere. I'd still would like to see a test case demonstrating the issue without mockjax (even if only a unit test in a PR).

comment:9 Changed 2 years ago by byroot

Here a JSFiddle that show the error using Github API:  http://jsfiddle.net/arkVR/7/

Note that you have to provide your GIthub username and passwords.

You will see that in the second request it's the error callabck that is called even if the server successfully deleted the gist

And this version using jQuery 1.8.3:  http://jsfiddle.net/arkVR/8/ call the success callback.

So yeah it's a regression.

comment:10 Changed 2 years ago by jaubourg

  • Owner set to jaubourg
  • Priority changed from undecided to blocker
  • Status changed from new to assigned
  • Component changed from unfiled to ajax
  • Milestone changed from None to 1.9.1

Merci, Jean. Much appreciated test case.

This won't fail with any other dataType than "json" because the regression is due to the re-alignment of parseJSON with native JSON.parse (throwing an exception for null/undefined values).

So THAT is the regression.

Thing is it showed a fault in the ajax code: it should not assume parsers will bypass on null/undefined inputs. The test for 204 is indeed needed but was never there because our parsers never .

I'm taking ownership. The PR is fine but there's a shorter path (in terms of bytes) and I can add a test case.

comment:11 Changed 2 years ago by byroot

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

Don't try and convert data for 204 No Content responses. Fixes #13292. Fixes #13261.

Changeset: bfc61b879e9e94ef7c6b31919b7f287ffdf180a8

comment:12 Changed 2 years ago by byroot

Don't try and convert data for 204 No Content responses. Fixes #13292. Fixes #13261.

Changeset: eb47553eeac58861bcefac063c0e03e161b4d52c

comment:13 follow-up: ↓ 14 Changed 11 months ago by martyn.frank.87@…

I can see that this was released in jQuery 2, but is there any plans to release it in jQuery 1?

comment:14 in reply to: ↑ 13 Changed 11 months ago by dmethvin

Replying to martyn.frank.87@…:

I can see that this was released in jQuery 2, but is there any plans to release it in jQuery 1?

The problem shouldn't exist in the 1.x branch. If it does, file a new ticket with a test case.

Note: See TracTickets for help on using tickets.