Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fixes #7229 and #5803
  • Loading branch information
rwaldron committed Oct 24, 2010
1 parent 9bd9ebd commit 3b50eac
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/ajax.js
Expand Up @@ -208,6 +208,12 @@ jQuery.extend({
s.data = jQuery.param( s.data, s.traditional );
}

// If the jsonpCallback has been set, we can assume that dataType is jsonp
// Ticket #5803
if ( s.jsonpCallback ) {

This comment has been minimized.

Copy link
@csnover

csnover Oct 25, 2010

Member

This is a very very risky commit for a maintenance release since it is an enhancement. Someone that prepares and re-uses a single object for their AJAX requests, changing only the dataType between jsonp/regular requests, may experience a regression here.

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 25, 2010

Member

How would that happen? We're modifying a clone of the settings object - not the original object itself.

This comment has been minimized.

Copy link
@csnover

csnover Oct 25, 2010

Member

Something like:

var defaultSettings = {
  jsonpCallback: 'remoteCallback',
  data: { foo: 'bar' },
  dataType: 'json',
  success: function () {},
  failure: function () {},
  timeout: 20000
};

function remoteAccess(url) {
  $.ajax($.extend({}, defaultSettings, { dataType: 'jsonp', url: url }));
}
function localAccess(url) {
  $.ajax($.extend({}, defaultSettings, { url: url }));
}

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 25, 2010

Member

I'm not following your concern here - the property isn't being changed in the defaultSettings object since it'll never be directly manipulated.

It wouldn't affect your example since you're passing in a different object every time you call $.ajax(). Even if you passed in the same object it wouldn't matter because we clone the incoming object:
http://github.com/jquery/jquery/blob/master/src/ajax.js#L198

This comment has been minimized.

Copy link
@csnover

csnover Oct 25, 2010

Member

In my example, both $.ajax requests will be treated as jsonp requests because jsonpCallback is provided, even on requests that are not and should not be jsonp requests. This is a regression from 1.4.2 and earlier where you can safely pass jsonpCallback on a non-jsonp request and have it be ignored.

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 25, 2010

Member

You keep using the phrase "regression" when you should be using the phrase "bug fix" - just because a change occurs does not mean that it's a regression. If a jsonpCallback is being passed in then the request is decidely going to be a jsonp request. There is no reason to not land this bug fix for a decidedly fringe example case.

This comment has been minimized.

Copy link
@SlexAxton

SlexAxton Oct 25, 2010

Member

In this case it'd be silly for a developer to put the jsonpCallback in the default settings and not in the first call to $.extend but I can see where we'd break some code of someone doing something we don't expect. I'd say this isn't really a must have feature or anything, so why not just wait on it til 1.5? You can always just pass in a dataType...

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 25, 2010

Member

Well, there are two ways of thinking about this: 1) We put everything that could possibly affect anyone into major releases. The minor releases are inconsequential as a result as virtually every change has the possibility of affecting someone. 2) We sprinkle some minor changes into minor releases and larger changes into the large releases - this gives users a graceful way of upgrading, realizing exactly which major changes are coming (in a major release) and focusing only on those. Since the source to jQuery is so small and open (comparatively) people will always end up abusing it in weird ways - we can mitigate this by pushing people in the right direction, slowly, and over time - one minor release at a time.

This comment has been minimized.

Copy link
@csnover

csnover Oct 26, 2010

Member

Here’s my perspective of those two same scenarios—the first being my proposal for a development roadmap moving forward.

  1. We provide minor releases on a regular schedule that only include bug fixes. Users affected by issues that have been fixed in these minor releases can safely upgrade without experiencing regressions caused by new features/enhancements. Users that want new functionality can upgrade to new major versions. Both minor and major versions can be released more frequently because the team is not spending excessive amounts of time patching or backing out enhancements from the last minor release that seemed safe but turned out to break code in the wild. Everyone is happy.

  2. We include enhancements and new features in minor releases just because we have patches ready to go. Users can’t risk upgrading to get the bug fixes they need without doing more work to verify that unrelated parts of their application haven't been broken by the enhancements. This leads to end users that hand-patch their code, users that won’t upgrade to new releases, and generally risks jQuery getting a bad reputation for releasing buggy code. It also means jQuery developers spend a lot of time dealing with undocumented/inadvertent backward-compatibility breaks which could be spent getting a new major release ready to go.

Yes, this specific patch is a very minor enhancement with a low risk of regression because it involves a seldom-used feature. However, it does not fix any bugs—people aren’t having trouble getting jsonp working without this patch—and for a release that exists primarily to fix regressions, I strongly feel that it does not belong. Nobody is clamoring for this enhancement—it just happened to be trivial to write.

This comment has been minimized.

Copy link
@curiousdannii

curiousdannii Oct 26, 2010

@jeresig, did you see my concerns in the pull request? Are you sure that you want it so that a jsonpCallback set with ajaxSetup() will cause every request to be a jsonp request unless you manually reset it in your ajax() call? I maintain that it would be better to check origSettings.jsonpCallback.

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 26, 2010

Member

@csnover: There are a couple of problems with your proposed solution - first you make the (massive) assumption that bug fixes never introduce other bugs - when, in fact, I often find the opposite to be true. Virtually any change is capable of causing unforeseen problems (when the audience is as large as jQuery's any change is capable of causing a problem). I would argue that enhancements and features generally have a much lower probability of causing problems - introducing new functionality yields new paths that users have not interacted with before.

Additionally there is a massive social problem that exists: Creating releases that only contain bug fixes will guarantee that a large portion of the jQuery audience will simply never upgrade. If they're not hitting any problems then what's the point of upgrading? Including performance improvements, new features, and basic enhancements will keep the audience engaged and encourage them to continually upgrade.

Now there are a number of secondary issues which, I feel, are much more important: Finding ways to get users engaged earlier in the release process, giving users better ways of communicating important issues to us, getting users submitting better bug reports, getting better coverage in the jQuery test suite, etc. We're already making good headway on the first three of those areas and should explore ways of making the last even more feasible (such as auto-pulling-in test suites of jQuery plugins).

This comment has been minimized.

Copy link
@rwaldron

rwaldron Oct 26, 2010

Author Member

The beauty of release candidates. I'd like to put forth the motion that we back out the feature and add it to the 1.5 list where we can further discuss an implementation that pleases everyone and adds value to the jQuery core library.

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 28, 2010

Member

@curiousdannii: Ah, I did not see that particular concern - it's a valid one. While I still feel strongly that we should allow for enhancements and small features in minor releases of jQuery I'm less confident in this particular patch and have backed it out. Can you provide a pull request with a proposed patch (and include Rick's tests as well)? Thanks.

This comment has been minimized.

Copy link
@rwaldron

rwaldron Oct 28, 2010

Author Member

@jeresig I've already updated the patch and have additional unit tests, I'm holding off on pushing that branch to my fork until all the 1.4.4 issues are resolved.

This comment has been minimized.

Copy link
@jeresig

jeresig Oct 28, 2010

Member

@rwldrn: Push push push! :)

This comment has been minimized.

Copy link
@rwaldron

rwaldron Oct 29, 2010

Author Member

@jeresig, pushed and awaiting your pull: http://github.com/jquery/jquery/pull/79

s.dataType = "jsonp";
}

// Handle JSONP Parameter Callbacks
if ( s.dataType === "jsonp" ) {
if ( type === "GET" ) {
Expand Down
3 changes: 3 additions & 0 deletions src/event.js
Expand Up @@ -32,6 +32,9 @@ jQuery.event = {

if ( handler === false ) {
handler = returnFalse;
} else if ( !handler ) {

This comment has been minimized.

Copy link
@csnover

csnover Oct 25, 2010

Member

Just adding a note to remind us down the line that this is probably an area where we can save space in 1.5, depending upon what happens with the ajax rewrite, since nobody should be passing null/undefined handlers and expecting it to get silently eaten.

// Fixes bug #7229. Fix recommended by jdalton
return;
}

var handleObjIn, handleObj;
Expand Down
15 changes: 15 additions & 0 deletions test/unit/ajax.js
Expand Up @@ -799,6 +799,21 @@ test("jQuery.ajax() - JSONP, Local", function() {
plus();
}
});

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

});

test("JSONP - Custom JSONP Callback", function() {
Expand Down
20 changes: 20 additions & 0 deletions test/unit/event.js
@@ -1,5 +1,25 @@
module("event");

test("null or undefined handler", function() {
expect(2);
// Supports Fixes bug #7229
try {

jQuery("#firstp").click(null);

ok(true, "Passing a null handler will not throw an exception");

} catch (e) {}

try {

jQuery("#firstp").click(undefined);

ok(true, "Passing an undefined handler will not throw an exception");

} catch (e) {}
});

test("bind(), with data", function() {
expect(3);
var handler = function(event) {
Expand Down

0 comments on commit 3b50eac

Please sign in to comment.