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

[gibson042]

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.

[jaubourg]

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.

[dmethvin]

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

[jaubourg]

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.

[gibson042]

Replying to 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 )

[jaubourg]

// 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

[dmethvin]

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.

[gibson042]

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

[dmethvin]

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.

[jaubourg]

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

[gibson042]

​https://github.com/jquery/jquery/pull/1051

[Richard Gibson]

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

Changeset: 03db1ada2cc223edf545c5a452e55062647837fa