Skip to content
Permalink
Browse files

Fixes #11151, #13388. Minor refactor of response conversion and when…

…/where

 responseXXX fields are set on the jqXHR. Close gh-1164.
  • Loading branch information
jaubourg authored and dmethvin committed Feb 8, 2013
1 parent a14a317 commit 69b3d5ce0f081d3f113b2917495f35df160f8522
Showing with 73 additions and 41 deletions.
  1. +45 −41 src/ajax.js
  2. +6 −0 test/data/errorWithJSON.php
  3. +22 −0 test/unit/ajax.js
@@ -257,7 +257,8 @@ jQuery.extend({

responseFields: {
xml: "responseXML",
text: "responseText"
text: "responseText",
json: "responseJSON"
},

// Data converters
@@ -606,13 +607,19 @@ jQuery.extend({
// Set readyState
jqXHR.readyState = status > 0 ? 4 : 0;

// Determine if successful
isSuccess = status >= 200 && status < 300 || status === 304;

// Get response data
if ( responses ) {
response = ajaxHandleResponses( s, jqXHR, responses );
}

// Convert no matter what (that way responseXXX fields are always set)
response = ajaxConvert( s, response, jqXHR, isSuccess );

// If successful, handle type chaining
if ( status >= 200 && status < 300 || status === 304 ) {
if ( isSuccess ) {

// Set the If-Modified-Since and/or If-None-Match header, if in ifModified mode.
if ( s.ifModified ) {
@@ -628,20 +635,17 @@ jQuery.extend({

// if no content
if ( status === 204 ) {
isSuccess = true;
statusText = "nocontent";

// if not modified
} else if ( status === 304 ) {
isSuccess = true;
statusText = "notmodified";

// If we have data, let's convert it
} else {
isSuccess = ajaxConvert( s, response );
statusText = isSuccess.state;
success = isSuccess.data;
error = isSuccess.error;
statusText = response.state;
success = response.data;
error = response.error;
isSuccess = !error;
}
} else {
@@ -701,22 +705,13 @@ jQuery.extend({
});

/* Handles responses to an ajax request:
* - sets all responseXXX fields accordingly
* - finds the right dataType (mediates between content-type and expected dataType)
* - returns the corresponding response
*/
function ajaxHandleResponses( s, jqXHR, responses ) {
var firstDataType, ct, finalDataType, type,
contents = s.contents,
dataTypes = s.dataTypes,
responseFields = s.responseFields;

// Fill responseXXX fields
for ( type in responseFields ) {
if ( type in responses ) {
jqXHR[ responseFields[type] ] = responses[ type ];
}
}
dataTypes = s.dataTypes;

// Remove auto dataType and get content-type in the process
while( dataTypes[ 0 ] === "*" ) {
@@ -765,19 +760,14 @@ function ajaxHandleResponses( s, jqXHR, responses ) {
}
}

// Chain conversions given the request and the original response
function ajaxConvert( s, response ) {
var conv2, current, conv, tmp,
/* Chain conversions given the request and the original response
* Also sets the responseXXX fields on the jqXHR instance
*/
function ajaxConvert( s, response, jqXHR, isSuccess ) {
var conv2, current, conv, tmp, prev,
converters = {},
i = 0,
// Work with a copy of dataTypes in case we need to modify it for conversion
dataTypes = s.dataTypes.slice(),
prev = dataTypes[ 0 ];

// Apply the dataFilter if provided
if ( s.dataFilter ) {
response = s.dataFilter( response, s.dataType );
}
dataTypes = s.dataTypes.slice();

// Create converters map with lowercased keys
if ( dataTypes[ 1 ] ) {
@@ -786,14 +776,32 @@ function ajaxConvert( s, response ) {
}
}

// Convert to each sequential dataType, tolerating list modification
for ( ; (current = dataTypes[++i]); ) {
current = dataTypes.shift();

// There's only work to do if current dataType is non-auto
if ( current !== "*" ) {
// Convert to each sequential dataType
while ( current ) {

if ( s.responseFields[ current ] ) {
jqXHR[ s.responseFields[ current ] ] = response;
}

// Apply the dataFilter if provided
if ( !prev && isSuccess && s.dataFilter ) {
response = s.dataFilter( response, s.dataType );
}

prev = current;
current = dataTypes.shift();

if ( current ) {

// There's only work to do if current dataType is non-auto
if ( current === "*" ) {

current = prev;

// Convert response if prev dataType is non-auto and differs from current
if ( prev !== "*" && prev !== current ) {
} else if ( prev !== "*" && prev !== current ) {

// Seek a direct converter
conv = converters[ prev + " " + current ] || converters[ "* " + current ];
@@ -803,7 +811,7 @@ function ajaxConvert( s, response ) {
for ( conv2 in converters ) {

// If conv2 outputs current
tmp = conv2.split(" ");
tmp = conv2.split( " " );

This comment has been minimized.

Copy link
@rwaldron

rwaldron Feb 28, 2013

Member

Shouldn't this be: ( conv2.match( core_rnotwhite ) || [] )?

(read that in a really snarky tone.)

This comment has been minimized.

Copy link
@dmethvin

dmethvin Feb 28, 2013

Member

Why use two spaces when one would do? 😈

This comment has been minimized.

Copy link
@rwaldron

rwaldron Feb 28, 2013

Member

Yes.

... My thoughts, exactly.

7f94a5c#L0R110

This comment has been minimized.

Copy link
@jaubourg

jaubourg Feb 28, 2013

Author Member

The converter key format is "sourceType destType" with a single space. This is already deep enough into the ajax architecture for us to be a bit stricter than usual. If it saves bytes, now that's another story :P

This comment has been minimized.

Copy link
@dmethvin

dmethvin Feb 28, 2013

Member

Honest there's no problem with doing that, but this particular situation is only encountered if someone defines a converter which is much rarer than a user adding event handlers or removing data names. So for consistency we could change this but it's not likely to cause the errors of the first two.

EDIT: What the Frenchman said.

This comment has been minimized.

Copy link
@rwaldron

rwaldron Feb 28, 2013

Member

Let me be perfectly clear: I was trolling both of you.

I don't think you should change it to that insane ( conv2.match( core_rnotwhite ) || [] ) pattern at all. I don't think any of code should use that, where a .split(" ") is sufficient—but I was strong armed into it with an argument for "consistency". I think "consistency" should also be on the side of "the least insane approach".

if ( tmp[ 1 ] === current ) {

// If prev can be converted to accepted input
@@ -817,9 +825,8 @@ function ajaxConvert( s, response ) {
// Otherwise, insert the intermediate dataType
} else if ( converters[ conv2 ] !== true ) {
current = tmp[ 0 ];
dataTypes.splice( i--, 0, current );
dataTypes.unshift( tmp[ 1 ] );
}

break;
}
}
@@ -830,7 +837,7 @@ function ajaxConvert( s, response ) {
if ( conv !== true ) {

// Unless errors are allowed to bubble, catch and return them
if ( conv && s["throws"] ) {
if ( conv && s[ "throws" ] ) {
response = conv( response );
} else {
try {
@@ -841,9 +848,6 @@ function ajaxConvert( s, response ) {
}
}
}

// Update prev for next iteration
prev = current;
}
}

@@ -0,0 +1,6 @@
<?php

header("HTTP/1.0 400 Bad Request");
header("Content-Type: application/json");

echo '{ "code": 40, "message": "Bad Request" }';
@@ -1422,6 +1422,18 @@ module( "ajax", {

});

ajaxTest( "#11151 - jQuery.ajax() - parse error body", 2, {
url: url("data/errorWithJSON.php"),
dataFilter: function( string ) {
ok( false, "dataFilter called" );
return string;
},
error: function( jqXHR ) {
strictEqual( jqXHR.responseText, "{ \"code\": 40, \"message\": \"Bad Request\" }", "Error body properly set" );
deepEqual( jqXHR.responseJSON, { code: 40, message: "Bad Request" }, "Error body properly parsed" );
}
});

ajaxTest( "#11426 - jQuery.ajax() - loading binary data shouldn't throw an exception in IE", 1, {
url: url("data/1x1.jpg"),
success: function( data ) {
@@ -1519,6 +1531,16 @@ module( "ajax", {
}
});

ajaxTest( "#13388 - jQuery.ajax() - responseXML", 3, {
url: url("data/with_fries.xml"),
dataType: "xml",
success: function( resp, _, jqXHR ) {
notStrictEqual( resp, undefined, "XML document exists" );
ok( "responseXML" in jqXHR, "jqXHR.responseXML exists" );
strictEqual( resp, jqXHR.responseXML, "jqXHR.responseXML is set correctly" );
}
});

//----------- jQuery.ajaxPrefilter()

ajaxTest( "jQuery.ajaxPrefilter() - abort", 1, {

2 comments on commit 69b3d5c

@jaubourg

This comment has been minimized.

Copy link
Member Author

jaubourg replied Feb 28, 2013

We should probably cherry-pick this for 2.0. Do you want me to do so Dave or are you already on it?

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin replied Feb 28, 2013

Doing it now, thanks for the reminder!

Please sign in to comment.
You can’t perform that action at this time.