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.

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

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

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@gibson042
Member

gibson042 commented Dec 2, 2012

I'm not thrilled about the name, but at least it's unambiguous.

Sizes - compared to master @ c2d6847

    264083       (+44)  dist/jquery.js                                         
     92059       (+60)  dist/jquery.min.js                                     
     32626       (+11)  dist/jquery.min.js.gz
@rwaldron

View changes

Show outdated Hide outdated test/unit/ajax.js Outdated
@rwaldron

View changes

Show outdated Hide outdated src/ajax.js Outdated
@jaubourg

View changes

Show outdated Hide outdated test/unit/ajax.js Outdated
data = undefined;
}
return jQuery.ajax({

This comment has been minimized.

@jaubourg

jaubourg Dec 2, 2012

Member

Nice use of options to protect from custom converters.

@jaubourg

jaubourg Dec 2, 2012

Member

Nice use of options to protect from custom converters.

Show outdated Hide outdated src/ajax.js Outdated
@@ -194,25 +194,6 @@ jQuery.each( [ "ajaxStart", "ajaxStop", "ajaxComplete", "ajaxError", "ajaxSucces
};
});
jQuery.each( [ "get", "post" ], function( i, method ) {

This comment has been minimized.

@Krinkle

Krinkle Jan 5, 2013

Member

Why was this block moved?

@Krinkle

Krinkle Jan 5, 2013

Member

Why was this block moved?

Show outdated Hide outdated src/ajax.js Outdated
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 4, 2013

Member

Updated for the new year! Also, @jaubourg was right... putting this alongside ajax methods with which it has little in common is just begging for bug reports. So manipulation it is!

   raw     gz Sizes                                                             
239589  71007 dist/jquery.js                                                    
 82978  29518 dist/jquery.min.js                                                

   raw     gz Compared to master @ 32b066d3805a48f8c8312562ed52a1b5910b1d85     
   +35     -3 dist/jquery.js                                                    
   +52    +17 dist/jquery.min.js
Member

gibson042 commented Apr 4, 2013

Updated for the new year! Also, @jaubourg was right... putting this alongside ajax methods with which it has little in common is just begging for bug reports. So manipulation it is!

   raw     gz Sizes                                                             
239589  71007 dist/jquery.js                                                    
 82978  29518 dist/jquery.min.js                                                

   raw     gz Compared to master @ 32b066d3805a48f8c8312562ed52a1b5910b1d85     
   +35     -3 dist/jquery.js                                                    
   +52    +17 dist/jquery.min.js
global: false,
"throws": true
});
jQuery._evalUrl( node.src );

This comment has been minimized.

@rwaldron

rwaldron Apr 4, 2013

Member

-1, No new API surface.

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

@rwaldron

rwaldron Apr 4, 2013

Member

-1, No new API surface.

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

This comment has been minimized.

@jaubourg

jaubourg Apr 4, 2013

Member

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

@jaubourg

jaubourg Apr 4, 2013

Member

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

This comment has been minimized.

@gibson042

gibson042 Apr 4, 2013

Member

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".

@gibson042

gibson042 Apr 4, 2013

Member

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".

This comment has been minimized.

@dmethvin

dmethvin Apr 4, 2013

Member

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.

@dmethvin

dmethvin Apr 4, 2013

Member

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.

This comment has been minimized.

@jaubourg

jaubourg Apr 4, 2013

Member

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.

@jaubourg

jaubourg Apr 4, 2013

Member

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.

This comment has been minimized.

@rwaldron

rwaldron Apr 4, 2013

Member

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?

@rwaldron

rwaldron Apr 4, 2013

Member

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?

This comment has been minimized.

@jaubourg

jaubourg Apr 4, 2013

Member

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.

@jaubourg

jaubourg Apr 4, 2013

Member

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.

This comment has been minimized.

@rwaldron

rwaldron Apr 4, 2013

Member

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

@rwaldron

rwaldron Apr 4, 2013

Member

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

This comment has been minimized.

@jaubourg

jaubourg Apr 4, 2013

Member

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

@jaubourg

jaubourg Apr 4, 2013

Member

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

This comment has been minimized.

@rwaldron

rwaldron Apr 4, 2013

Member

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.

@rwaldron

rwaldron Apr 4, 2013

Member

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.

@gibson042 gibson042 closed this in 03db1ad Apr 17, 2013

gibson042 added a commit that referenced this pull request Apr 17, 2013

Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/e…
…xecute in domManip. Close gh-1051.

(cherry picked from commit 03db1ad)

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/e…
…xecute in domManip. Close gh-1051.

(cherry picked from commit 03db1ad)
@somixyz

This comment was marked as off-topic.

Show comment
Hide comment
@somixyz

somixyz Sep 4, 2018

ooooooooo

somixyz commented Sep 4, 2018

ooooooooo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment