Skip to content

Commit

Permalink
Added in support for $.ajax jsonpCallback (allowing you to specify th…
Browse files Browse the repository at this point in the history
…e name of the callback method - and allowing you to avoid skipping the cache). Fixes #4206.
  • Loading branch information
jeresig committed Dec 7, 2009
1 parent aea5b09 commit fbc73d4
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 4 deletions.
4 changes: 2 additions & 2 deletions src/ajax.js
Expand Up @@ -221,7 +221,7 @@ jQuery.extend({

// Build temporary JSONP function
if ( s.dataType === "json" && (s.data && jsre.test(s.data) || jsre.test(s.url)) ) {
jsonp = "jsonp" + jsc++;
jsonp = s.jsonpCallback || ("jsonp" + jsc++);

// Replace the =? sequence both in the query string and the data
if ( s.data ) {
Expand All @@ -235,7 +235,7 @@ jQuery.extend({
s.dataType = "script";

// Handle JSONP-style loading
window[ jsonp ] = function(tmp){
window[ jsonp ] = window[ jsonp ] || function(tmp){
data = tmp;
success();
complete();
Expand Down
34 changes: 32 additions & 2 deletions test/unit/ajax.js
Expand Up @@ -518,10 +518,10 @@ test("jQuery.getScript(String, Function) - no callback", function() {
});

test("jQuery.ajax() - JSONP, Local", function() {
expect(7);
expect(8);

var count = 0;
function plus(){ if ( ++count == 7 ) start(); }
function plus(){ if ( ++count == 8 ) start(); }

stop();

Expand Down Expand Up @@ -579,6 +579,20 @@ test("jQuery.ajax() - JSONP, Local", function() {
}
});

jQuery.ajax({
url: "data/jsonp.php",
dataType: "jsonp",
jsonpCallback: "jsonpResults",
success: function(data){
ok( data.data, "JSON results returned (GET, custom callback name)" );
plus();
},
error: function(data){
ok( false, "Ajax error JSON (GET, custom callback name)" );
plus();
}
});

jQuery.ajax({
type: "POST",
url: "data/jsonp.php",
Expand Down Expand Up @@ -624,6 +638,22 @@ test("jQuery.ajax() - JSONP, Local", function() {
});
});

test("JSONP - Custom JSONP Callback", function() {
expect(1);
stop();

window.jsonpResults = function(data) {
ok( data.data, "JSON results returned (GET, custom callback function)" );
start();
};

jQuery.ajax({
url: "data/jsonp.php",
dataType: "jsonp",
jsonpCallback: "jsonpResults"
});
});

test("jQuery.ajax() - JSONP, Remote", function() {
expect(4);

Expand Down

6 comments on commit fbc73d4

@darwin
Copy link

@darwin darwin commented on fbc73d4 Dec 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deja-vu!

few hours ago I've added similar functionality into my fork:
http://github.com/darwin/jquery/commit/458958afa24edc1127618ef43784416ef787efd5

my solution is little more flexible, because it enables user to specify function for jsonp callback computation, the motivation is explained in the commit message

@jaubourg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really not a fan of line 238 of ajax.js. If the function is already defined:

  1. it will never call complete & success
  2. it will leave a script tag dangling

I think it would be better if the function was redefined no matter what. Seems like the best course of action as long as it is documented.

@jeresig
Copy link
Member Author

@jeresig jeresig commented on fbc73d4 Dec 8, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaubourg: I don't think we should override people's global variables. I think we can safely assume that if someone is explicitly defining a callback name, and providing the callback itself, then they're handling the whole shebang. I will definitely make that clear in the docs.

@jaubourg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeresig: I still don't agree. If someone wanted to take control of the whole process, then he would probably call $.getScript and not bother with $.ajax. I'd much prefer a solution like the one I have in http://github.com/jaubourg/jquery/blob/master/src/transports/jsonp.js (line 49 to 72). I basically substitute the value but put it back and call it if it's a function.

@jaubourg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeresig: beside, I suppose you're still deleting the function anyway?

@timbunce
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @jaubourg. I've just run into the fact that it will never call complete & success. That's counter intuitive, definitely not documented, and a major inconvenience. I'm using jsonpCallback because I need the browser caching. To loose the callbacks seems a high price to pay.

The lack of documentation for that side effect is presumably a bug.

Please sign in to comment.