Opened 10 years ago
Closed 10 years ago
#12838 closed feature (fixed)
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.
Change History (13)
comment:1 Changed 10 years ago by
Component: | unfiled → ajax |
---|---|
Milestone: | None → 1.9 |
Priority: | undecided → low |
Status: | new → open |
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
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).
comment:4 follow-up: 5 Changed 10 years ago by
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.
comment:5 Changed 10 years ago by
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:
jQuery.ajax({ ..., async: false, global: false, throws: true })
(what we have now, but made more public)jQuery._ajax( option )
(probably a saved reference of the originaljQuery.ajax
, but maybe skipsajaxSetup
etc.)jQuery.evalUrl( node.src )
jQuery.ajax.evalUrl( node.src )
comment:6 Changed 10 years ago by
// 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
comment:7 Changed 10 years ago by
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.
comment:8 Changed 10 years ago by
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"?
comment:9 Changed 10 years ago by
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.
comment:10 Changed 10 years ago by
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
comment:11 Changed 10 years ago by
Owner: | set to gibson042 |
---|---|
Status: | open → assigned |
comment:12 Changed 10 years ago by
Milestone: | 1.9 → 1.next |
---|
comment:13 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fix #12838: hook point for non-jQuery.ajax synchronous script fetch/execute in domManip. Close gh-1051.
Changeset: 03db1ada2cc223edf545c5a452e55062647837fa
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.