Skip to content

Commit

Permalink
Rewrite of globalEval. Uses window.execScript or window.eval with a t…
Browse files Browse the repository at this point in the history
…rick to ensure proper context. Unit tests added.
  • Loading branch information
jaubourg committed Apr 7, 2011
1 parent 4552d13 commit f3c6077
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 17 deletions.
27 changes: 10 additions & 17 deletions src/core.js
Expand Up @@ -561,24 +561,17 @@ jQuery.extend({

noop: function() {},

// Evalulates a script in a global context
// Evaluates a script in a global context
// Workarounds based on findings by Jim Driscoll
// http://weblogs.java.net/blog/driscoll/archive/2009/09/08/eval-javascript-global-context
globalEval: function( data ) {
if ( data && rnotwhite.test(data) ) {
// Inspired by code by Andrea Giammarchi
// http://webreflection.blogspot.com/2007/08/global-scope-evaluation-and-dom.html
var head = document.head || document.getElementsByTagName( "head" )[0] || document.documentElement,
script = document.createElement( "script" );

if ( jQuery.support.scriptEval() ) {
script.appendChild( document.createTextNode( data ) );
} else {
script.text = data;
}

// Use insertBefore instead of appendChild to circumvent an IE6 bug.
// This arises when a base node is used (#2709).
head.insertBefore( script, head.firstChild );
head.removeChild( script );
if ( data && rnotwhite.test( data ) ) {
// We use execScript on Internet Explorer
// We use an anonymous function so that context is window
// rather than jQuery in Firefox
( window.execScript || function( data ) {

This comment has been minimized.

Copy link
@jdalton

jdalton Apr 7, 2011

Member

noooos! 'execScript' is soo bad. It kills any meaningful error messages. Instead script inject that calls eval().

This comment has been minimized.

Copy link
@dmethvin

dmethvin Apr 7, 2011

Member

Might as well reference that kangax article: http://perfectionkills.com/global-eval-what-are-the-options/

Seemed like a name-your-poison situation since none of them are perfect, but script insertion does seem to have its advantages.

This comment has been minimized.

Copy link
@jdalton

jdalton Apr 7, 2011

Member

I don't believe .call is needed if window.eval(....) works.

This comment has been minimized.

Copy link
@jaubourg

jaubourg Apr 7, 2011

Author Member

I'm almost certain something goes wrong in Webkit browsers regarding context not being the window object.

This comment has been minimized.

Copy link
@jaubourg

jaubourg Apr 7, 2011

Author Member

Anyway, easy enough to test since unit tests are all about this damn context... I mean, we have the whole anonymous closure just so FF gets back to its senses.

This comment has been minimized.

Copy link
@jdalton

jdalton Apr 7, 2011

Member

A bonus of script injection+eval is you can capture the return value from the eval(....) code too

This comment has been minimized.

Copy link
@jdalton

jdalton Apr 7, 2011

Member

Ok maybe execScript's behavior has changed over the years. Now I get one meaningful error and one Error: Could not complete the operation due to error 80020101.

window[ "eval" ].call( window, data );
} )( data );
}
},

Expand Down
20 changes: 20 additions & 0 deletions test/unit/core.js
Expand Up @@ -172,6 +172,26 @@ test("selector state", function() {
);
});

test( "globalEval", function() {

expect( 3 );

jQuery.globalEval( "var globalEvalTest = true;" );
ok( window.globalEvalTest, "Test variable declarations are global" );

window.globalEvalTest = false;

jQuery.globalEval( "globalEvalTest = true;" );
ok( window.globalEvalTest, "Test variable assignments are global" );

window.globalEvalTest = false;

jQuery.globalEval( "this.globalEvalTest = true;" );
ok( window.globalEvalTest, "Test context (this) is the window object" );

window.globalEvalTest = undefined;
});

if ( !isLocal ) {
test("browser", function() {
stop();
Expand Down

3 comments on commit f3c6077

@rwaldron
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jaubourg, I'm going to be nit-picky here and ask why there was no ticket for this?

@jaubourg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tickets actually. They are all related to Firefox not executing the string passed to globalEval synchronously when using script tag injection and non-async scripts are being loaded (jdalton confirmed to me this was fixed in firefox a few months ago provided you set the injected script tag as async).

But, in the end, the globalEval rewrite is a by-product of the support rewrite, I just put it in a separate commit so that it could be "inspected" separately. Not feature-testing for how to add text to a script tag saves a lot of size in support.js.

@jdalton
Copy link
Member

@jdalton jdalton commented on f3c6077 Apr 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.