Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/execute in domManip #1051

Closed
wants to merge 3 commits into from
Closed

Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/execute in domManip #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08c6348
Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/e…
gibson042 Dec 2, 2012
081a18f
Renaming to suggest that method is private
gibson042 Dec 13, 2012
bd820ad
move to manipulation module
gibson042 Apr 4, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions src/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,25 +194,6 @@ jQuery.each( [ "ajaxStart", "ajaxStop", "ajaxComplete", "ajaxError", "ajaxSucces
};
});

jQuery.each( [ "get", "post" ], function( i, method ) {
jQuery[ method ] = function( url, data, callback, type ) {
// shift arguments if data argument was omitted
if ( jQuery.isFunction( data ) ) {
type = type || callback;
callback = data;
data = undefined;
}

return jQuery.ajax({
url: url,
type: method,
dataType: type,
data: data,
success: callback
});
};
});

jQuery.extend({
Copy link
Member

Choose a reason for hiding this comment

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

Why was this block moved?


// Counter for holding the number of active queries
Expand Down Expand Up @@ -694,15 +675,34 @@ jQuery.extend({
return jqXHR;
},

getScript: function( url, callback ) {
return jQuery.get( url, undefined, callback, "script" );
},

getJSON: function( url, data, callback ) {
return jQuery.get( url, data, callback, "json" );
},

getScript: function( url, callback ) {
return jQuery.get( url, undefined, callback, "script" );
}
});

jQuery.each( [ "get", "post" ], function( i, method ) {
jQuery[ method ] = function( url, data, callback, type ) {
// shift arguments if data argument was omitted
if ( jQuery.isFunction( data ) ) {
type = type || callback;
callback = data;
data = undefined;
}

return jQuery.ajax({
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of options to protect from custom converters.

url: url,
type: method,
dataType: type,
data: data,
success: callback
Copy link
Member

Choose a reason for hiding this comment

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

Why was this block moved?

});
};
});

/* Handles responses to an ajax request:
* - finds the right dataType (mediates between content-type and expected dataType)
* - returns the corresponding response
Expand Down
20 changes: 12 additions & 8 deletions src/manipulation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,14 +334,7 @@ jQuery.fn.extend({

if ( node.src ) {
// Hope ajax is available...
jQuery.ajax({
url: node.src,
type: "GET",
dataType: "script",
async: false,
global: false,
"throws": true
});
jQuery._evalUrl( node.src );
Copy link
Member

Choose a reason for hiding this comment

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

-1, No new API surface.

I'm sorry, but we need to find another way.

Copy link
Member

Choose a reason for hiding this comment

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

It's not so much surface API as it is a simple hook. It's clearly marked as private (leading underscore).

Copy link
Member Author

Choose a reason for hiding this comment

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

It is literally impossible to satisfy #12838 without new API surface. We may opt not to do it, but we will not find "another way".

Copy link
Member

Choose a reason for hiding this comment

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

The only other way I see is to have a global core_evalUrl variable and require anyone who wants to override the ajax module to do a custom build.

Copy link
Member

Choose a reason for hiding this comment

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

The only other way I see is to have a global core_evalUrl variable and require anyone who wants to override the ajax module to do a custom build.

It's not practical. One would be expected to have a build without ajax and be able to use this build with whatever third party ajax lib (or none: $._evalURL = $.noop;) they see fit, not have to rebuild a custom version per third-party. Having to override a variable in the closure at build time is overkill when a private overridable hook on the jQuery object does the job perfectly.

I understand rick's reservations with this but I don't think we can offer a practical solution any other way. We can build jQuery without ajax now, so we should provide this hook.

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a script element being created and appended to the document.head: https://github.com/jquery/jquery/blob/master/src/ajax/script.js#L34-L48 which would be invoked as a result of this call site: https://github.com/jquery/jquery/blob/master/src/manipulation.js#L335-L345

Is that incorrect?

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a script element being created and appended to the document.head: https://github.com/jquery/jquery/blob/master/src/ajax/script.js#L34-L48 which would be invoked as a result of this call site: https://github.com/jquery/jquery/blob/master/src/manipulation.js#L335-L345
Is that incorrect?

Yes, it is incorrect. Transport selection is a bit more involved than a simple one-on-one correspondance with a dataType. You missed the condition here: https://github.com/jquery/jquery/blob/master/src/ajax/script.js#L30

If it's not a crossDomain request, then ajax will continue searching for a suitable transport, until it reaches the one in xhr.js. Effectively enabling synchronous same-domain requests. Executing scripts synchronously is mandatory for domManip. It is, and always has been, broken for scripts of another domain but that's beside the point.

Copy link
Member

Choose a reason for hiding this comment

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

What happens if the custom ajax lib doesn't do any of this? Can it just use a script element?

Copy link
Member

Choose a reason for hiding this comment

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

You can't load a script using script tag injection synchronously.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I know—thank you—but that's not what I asked.

I concede, let's pile more ugly "_" junk onto jQuery. Can't wait for the bug reports.

} else {
jQuery.globalEval( node.textContent.replace( rcleanScript, "" ) );
}
Expand Down Expand Up @@ -540,6 +533,17 @@ jQuery.extend({
// Discard any remaining `private` and `user` data
data_discard( elem );
}
},

_evalUrl: function( url ) {
return jQuery.ajax({
url: url,
type: "GET",
dataType: "text",
async: false,
global: false,
success: jQuery.globalEval
});
}
});

Expand Down
59 changes: 51 additions & 8 deletions test/unit/manipulation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2045,14 +2045,19 @@ test( "html() - script exceptions bubble (#11743)", function() {
ok( false, "Exception ignored" );
}, "Exception bubbled from inline script" );

var onerror = window.onerror;
window.onerror = function() {
ok( true, "Exception thrown in remote script" );
window.onerror = onerror;
};
if ( jQuery.ajax ) {
var onerror = window.onerror;
window.onerror = function() {
ok( true, "Exception thrown in remote script" );
window.onerror = onerror;
};

jQuery("#qunit-fixture").html("<script src='data/badcall.js'></script>");
ok( true, "Exception ignored" );
jQuery("#qunit-fixture").html("<script src='data/badcall.js'></script>");
ok( true, "Exception ignored" );
} else {
ok( true, "No jQuery.ajax" );
ok( true, "No jQuery.ajax" );
}
});

test( "checked state is cloned with clone()", function() {
Expand Down Expand Up @@ -2092,7 +2097,7 @@ testIframeWithCallback( "buildFragment works even if document[0] is iframe's win

test( "script evaluation (#11795)", function() {

expect( 11 );
expect( 13 );

var scriptsIn, scriptsOut,
fixture = jQuery("#qunit-fixture").empty(),
Expand Down Expand Up @@ -2133,6 +2138,44 @@ test( "script evaluation (#11795)", function() {
fixture.append( scriptsOut.detach() );
deepEqual( fixture.children("script").get(), scriptsOut.get(), "Scripts detached without reevaluation" );
objGlobal.ok = isOk;

if ( jQuery.ajax ) {
Globals.register("testBar");
jQuery("#qunit-fixture").append( "<script src='" + url("data/test.js") + "'/>" );
strictEqual( window["testBar"], "bar", "Global script evaluation" );
} else {
ok( true, "No jQuery.ajax" );
ok( true, "No jQuery.ajax" );
}
});

test( "jQuery._evalUrl (#12838)", function() {

expect( 5 );

var message, expectedArgument,
ajax = jQuery.ajax,
evalUrl = jQuery._evalUrl;

message = "jQuery.ajax implementation";
expectedArgument = 1;
jQuery.ajax = function( input ) {
equal( ( input.url || input ).slice( -1 ), expectedArgument, message );
expectedArgument++;
};
jQuery("#qunit-fixture").append("<script src='1'/><script src='2'/>");
equal( expectedArgument, 3, "synchronous execution" );

message = "custom implementation";
expectedArgument = 3;
jQuery._evalUrl = jQuery.ajax;
jQuery.ajax = function( options ) {
strictEqual( options, {}, "Unexpected call to jQuery.ajax" );
};
jQuery("#qunit-fixture").append("<script src='3'/><script src='4'/>");

jQuery.ajax = ajax;
jQuery._evalUrl = evalUrl;
});

test( "wrapping scripts (#10470)", function() {
Expand Down