Skip to main content

Bug Tracker

Side navigation

#12838 closed feature (fixed)

Opened November 02, 2012 08:44PM UTC

Closed April 17, 2013 03:55PM UTC

domManip script evaluation doesn't tolerate ajax implementations with alternate signatures

Reported by: gibson042 Owned by: gibson042
Priority: low Milestone: 1.next
Component: ajax Version: 1.8.2
Keywords: Cc:
Blocked by: Blocking:
Description

As identified at https://github.com/jquery/jquery/pull/864/files#r2012889

.domManip assumes that if jQuery.ajax exists, it will behave identically to our ajax method with respect to parameters like async, global, and throws. However, this is not a safe assumption now that ajax is an optional module and users are free to drop in completely variant implementations.

.domManip should instead call a method—or throw an exception if it doesn't exist—specifically intended to synchronously fetch and execute script in the global context, which can share the internals of jQuery.ajax but should not call it directly.

Attachments (0)
Change History (13)

Changed November 02, 2012 08:47PM UTC by gibson042 comment:1

component: unfiledajax
milestone: None1.9
priority: undecidedlow
status: newopen

Changed November 22, 2012 04:19PM UTC by jaubourg comment:2

It's a little overkill, wouldn't you say? If an alternative ajax implementation doesn't support several options, then it will break much more than just this code (ie, all the plugins relying on $.ajax -- including validation --, all the ajax calls in the app, etc). It's more than likely that an alternative ajax imp won't use $.ajax() as its entrypoint.

Also, I'm completely opposed to methods bypassing $.ajax but still using its internals. It makes for very nasty hidden dependencies (prefilters, transports, global ajax option, etc).

If you really want to go that route, create a helper in manipulation.js that uses $.ajax() as it is used now in $.domManip(). That way, alternative ajax imp can just redefine the method and be done with it.

Changed November 22, 2012 05:24PM UTC by dmethvin comment:3

Yeah that's basically what I think were were advocating here, the extension point would be implemented by $.ajax in the default implementation (and should be in ajax.js).

Changed November 23, 2012 01:11AM UTC by jaubourg comment:4

I think it should rather be in manipulation.js. It is not a general purpose ajax helper. It is just an internal mechanism of domManip exposed so that it can be overriden if needs be. Hell, it could even be re-implemented in plain javascript by a third-party. Overriding something that doesn't exist (if the helper was in ajax.js and the build wasn't including it) seems kinda strange to me.

To make it short: having the dependency from manipulation.js to ajax.js isolated in an overridable helper that belongs to manipulation.js makes a lot of sense. Having this helper (only used by a single method from another module) defined in ajax.js doesn't. Adds bytes to manipulation.js but it's so much more explicit.

Also, I read the OP again to make sure but what I read is not quite what I'm suggesting.

Changed November 23, 2012 03:42PM UTC by gibson042 comment:5

Replying to [comment:4 jaubourg]:

I don't think the extension point ''can'' be in manipulation.js... because the internals of ajax.js aren't (necessarily) available there, it's just moving the problem around. I use "problem" loosely, though, because to my knowledge no user has complained about this. But there is value in having a clear hook that we can document for people who want to build a better ajax, which is why this ticket remains open.

I see four basic approaches to resolution:

1. jQuery.ajax({ ..., async: false, global: false, throws: true }) (what we have now, but made more public)

2. jQuery._ajax( option ) (probably a saved reference of the original jQuery.ajax, but maybe skips ajaxSetup etc.)

3. jQuery.evalUrl( node.src )

4. jQuery.ajax.evalUrl( node.src )

Changed November 23, 2012 04:40PM UTC by jaubourg comment:6

_comment0: {{{#!js \ // In manipulation.js \ jQuery.evalURL = function( url ) { \ jQuery.ajax({ \ url: node.src, \ type: "GET", \ dataType: "script", \ async: false, \ global: false, \ "throws": true \ }); \ }; \ \ // Later in domManip \ if ( node.src ) { \ jQuery.evalURL( node.src ); \ } \ }}} \ \ I fail to see how this cannot be done in `manipulation.js`. Any alternate ajax implementation can override evalURL because it ''knows'' it exists (it's declared somewhere, called and something goes wrong inside of it). \ \ If you really, really want to have the implementation of evalURL in ajax for some purity reasons, then let's do something like this: \ \ {{{#!js \ // In manipulation.js \ jQuery.evalURL = function( url ) { \ throw "Not Supported"; \ }; \ \ // Later in domManip \ if ( node.src ) { \ jQuery.evalURL( node.src ); \ } \ \ // Then in ajax.js \ jQuery.evalURL = function( url ) { \ jQuery.ajax({ \ url: node.src, \ type: "GET", \ dataType: "script", \ async: false, \ global: false, \ "throws": true \ }); \ }; \ }}} \ \ But no matter how you move things around there is no incentive whatsoever to use the internals of ajax. \ \ This is all very overkill for a hypothetic use-case we never ever saw the shadow of yet. But for the whole idea to make sense, you '''have to''' provide an implementation of the helper in `manipulation.js` (even if it just throws an exception or is a no-op). That way, it is present in the code even if the default ajax module is not and it is documented where it belongs (the manipulation module). If it throws an exception, the stack trace makes it clear that some helper was called and threw an exception. If the helper was defined in `ajax.js`, in a build without `ajax.js`, the stack trace would indicate that domManip called something that doesn't exist. As a dev, I much prefer the former as it makes it clear the helper is supposed to exist in the first place but that something went wrong within it. \ \ Let me re-assert that this helper has no use whatsoever outside of `domManip` and that we're talking about exposing it solely in order to make it overridable (it should probably be `_evalURL` to make it clear it's just some internal stuff exposed btw). As such, the idea of having it buried inside `ajax.js` while it doesn't even need access to any of its internals doesn't make any kind of sense. \ \ Oh and this much too excited post for a much too narrow use-case needs some smiley, so :P1353689092459073
// In manipulation.js
jQuery.evalURL =  function( url ) {
  jQuery.ajax({
    url: url,
    type: "GET",
    dataType: "script",
    async: false,
    global: false,
    "throws": true
  });
};

// Later in domManip
if ( node.src ) {
  jQuery.evalURL( node.src );
}

I fail to see how this cannot be done in manipulation.js. Any alternate ajax implementation can override evalURL because it ''knows'' it exists (it's declared somewhere, called and something goes wrong inside of it).

If you really, really want to have the implementation of evalURL in ajax for some purity reasons, then let's do something like this:

// In manipulation.js
jQuery.evalURL =  function( url ) {
  throw "Not Supported";
};

// Later in domManip
if ( node.src ) {
  jQuery.evalURL( node.src );
}

// Then in ajax.js
jQuery.evalURL =  function( url ) {
  jQuery.ajax({
    url: url,
    type: "GET",
    dataType: "script",
    async: false,
    global: false,
    "throws": true
  });
};

But no matter how you move things around there is no incentive whatsoever to use the internals of ajax.

This is all very overkill for a hypothetic use-case we never ever saw the shadow of yet. But for the whole idea to make sense, you have to provide an implementation of the helper in manipulation.js (even if it just throws an exception or is a no-op). That way, it is present in the code even if the default ajax module is not and it is documented where it belongs (the manipulation module). If it throws an exception, the stack trace makes it clear that some helper was called and threw an exception. If the helper was defined in ajax.js, in a build without ajax.js, the stack trace would indicate that domManip called something that doesn't exist. As a dev, I much prefer the former as it makes it clear the helper is supposed to exist in the first place but that something went wrong within it.

Let me re-assert that this helper has no use whatsoever outside of domManip and that we're talking about exposing it solely in order to make it overridable (it should probably be _evalURL to make it clear it's just some internal stuff exposed btw). As such, the idea of having it buried inside ajax.js while it doesn't even need access to any of its internals doesn't make any kind of sense.

Oh and this much too excited post for a much too narrow use-case needs some smiley, so :P

Changed November 23, 2012 04:50PM UTC by dmethvin comment:7

we're talking about exposing it solely in order to make it overridable (it should probably be _evalURL to make it clear it's just some internal stuff exposed btw).

Agreed with both of these. The reason I'd advocate putting the method in ajax.js is because if it is overridden there is no reason for the original implementation to be in manipulation.js. The standard implementation uses $.ajax so if $.ajax is missing you wouldn't want the standard implementation to be present.

Changed November 23, 2012 05:41PM UTC by gibson042 comment:8

I'm increasingly in favor of leaving the code as-is and requiring ajax implementations to support { async: false, global: false, throws: true } for full functionality in manipulation. It uses whatever ajax is defined, failing meaningfully when there isn't one. The only potential benefit we would get from an evalURL is using our pre-override ajax, then only if a replacement masks errors or doesn't support synchronous calls. After all, it's just as easy to support those options as it is to override another method.

Are there any objections to a no-change "fix"?

Changed November 23, 2012 05:58PM UTC by dmethvin comment:9

Well I was thinking that most alternative "ajax" implementations would not try to emulate the $.ajax() API because that would be very difficult to subset in a reliable way. Any code that sees a $.ajax entry point is probably going to assume the whole implementation is there.

My typical example is an app that only needs JSONP so it would exclude ajax.js and instead use something like jaubourg's $.jsonp plugin. They might call $.getJSON within the app itself but wouldn't implement $.ajax. They could certainly override an implementation placed in manipulation.js (or even let it error out if they didn't need scripts in their HTML) but in that case it would have been nice to have that implementation in ajax.js so it wouldn't be taking up bytes in the build.

Changed November 23, 2012 06:07PM UTC by jaubourg comment:10

Are there any objections to a no-change "fix"?

Apparently it's lets-disagree-with-gibson-day ;)

I think it's a good idea to have this hook. It seems to me you only consider alternative implementation that would keep $.ajax() as their entry-point. I sure hope people developping a new ajax module will first get rid of the cumbersome API.

So, assuming a new entry point (say $.request()), you'd need the hook.

Still love you <3

PS: Oh look, dmethvin agrees with me Oo

Changed December 02, 2012 05:17AM UTC by gibson042 comment:11

owner: → gibson042
status: openassigned

Changed January 25, 2013 01:57AM UTC by gibson042 comment:12

milestone: 1.91.next

Changed April 17, 2013 03:55PM UTC by Richard Gibson comment:13

resolution: → fixed
status: assignedclosed

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

Changeset: 03db1ada2cc223edf545c5a452e55062647837fa