Switch to script injection instead of indirect eval call#1449
Conversation
|
Could you remove the unrelated changes in Now that odd async behaviour is clearly a thing of the past, I'm 100% behind you on using script tag injection all the time. |
Could you remove the unrelated changes That's sad part – 3) Errors do not propagate, but instead would be thrown in different flow, meaning you can't catch them without window.onerror |
|
Oh yes! I had forgotten that was the thing using |
|
I assume the Function constructor can't be used? |
I assume the Function constructor can't be used? http://perfectionkills.com/global-eval-what-are-the-options/ |
|
IIRC, I had this for 2.x, I'm glad to see it finally happening :) |
There was a problem hiding this comment.
Hey, how about using Script URL to eval global code?
diff --git a/src/core.js b/src/core.js
index 994fd4c..62ad085 100644
--- a/src/core.js
+++ b/src/core.js
@@ -281,8 +281,7 @@ jQuery.extend({
// Evaluates a script in a global context
globalEval: function( code ) {
- var script,
- indirect = eval;
+ var indirect = eval;
code = jQuery.trim( code );
@@ -291,9 +290,7 @@ jQuery.extend({
// strict mode pragma, execute code by injecting a
// script tag into the document.
if ( code.indexOf("use strict") === 1 ) {
- script = document.createElement("script");
- script.text = code;
- document.head.appendChild( script ).parentNode.removeChild( script );
+ location.href = "javascript:" + code;
} else {
// Otherwise, avoid the DOM node creation, insertion
// and removal by using an indirect global evalThere was a problem hiding this comment.
Give it a try with some test cases. I don't think it globally evaluates the script reliably.
There was a problem hiding this comment.
@UncleBill a lot of problems with this one, in some browsers for some constructions it would indeed change the URL, in some browsers it would be a global eval in some browsers it would not, in some browsers it would be async execution in some browsers it would not.
This has some consequences in both ajax and manipulation. I think we should agree on it as a team before committing anything, and synchronize behavior between both branches to match the decision. |
It is, that's why i mentioned it, right now we have inconsistency between branches because this consequence already exist if evaluated code uses "use strict" statement, i don't really care what way we choose, but we have to choose one, although
Looks more logical and native, this PR is three months old now, so if you don't have any arguments against i don't really see the point to wait any longer. |
|
The important point about error behavior being changed didn't really register with me. Maybe it's all the discussion about Promises making things clearer. It would be worth some careful thought here about what might be broken and the options we can offer people to get the old behavior back. |
There is only one thing that could be broken - "Errors do not propagate", if we want to save this, i could provide a PR that would work with indirect eval call, i think we just need to choose. |
|
I agree that on our side, it comes down to "errors do not propagate" but we do not know the impact on the code base of our users. The fact that we weren't able to be consistent here in the past works to our advantage I think,. There could be situations where someone might have only gotten files from a the same domain and thus depended on catching errors, but I am not sure how we would be able to find those situations and warn people about them. Perhaps we could shim this somehow in jQuery Migrate? In any case this needs to be documented really well since it could break code. (Bad code 👅 ) On the timing of landing this, I suspect we'll want a 1.11.1/2.1.1 release to wrap up a few bug fixes. This would definitely be a 1.12/2.2 thing. |
We could use the indirect call in there.
Definitely, if we doing that, this change should not be introduced in patch release. |
|
We could definitely use the indirect call in Migrate, but I was wondering how we could detect and warn about a situation where the new script-tag method would hose them. Those warnings are the main job of Migrate. We could try/catch and warn about it if we saw an error, then rethrow. But any successful code that somehow expected synchronous execution would be hard to detect. |
|
See also http://bugs.jquery.com/ticket/14568 |
|
This will need a similar workaround in manipulation tests for Android 2.3 as in #1573. |
|
This PR seems to make Android fail AJAX tests even on |
|
Okay, if we leave 2.3, i will investigate, if not, then lets leave it as is. |
Right now, 2.x uses two techincs for code execution in global scope: script injection if "use strict" statement is found and indirect eval call if not. 1.x only uses eval path.
With indirect eval call, we have only one problem - if evaluated code uses strict pragma, it will not be executed in global scope, which subverts function purpose. Although current code tries to circumvent it, but fails on edge cases.
With script injection we have four problems:
window.onerrorOne common and obvious issue is difference of
globalEvalfunctionality between branches. Because now we have all those problems combine, so i propose to start using only one technic, this PR will try to do it with script injection.Now about issues with this way:
Well, jQuery is DOM-centric library, in other environments you should use other means
It's no longer true, since Firefox 4.0, but just to be sure, i added a test for it.
Yes, that would be a incompatible difference
It's actually the opposite (it's not my test if anyone asks :-)
I think the important thing is that jQuery should use only one technic and it should be identical between branches, i, personally, could live with any of them.
/cc @dmethvin, @rwaldron, @jdalton, @jaubourg