Skip to content
Permalink
Browse files
No more php/js logic duplication. Ensures modules are still loaded se… 
…parately when using "file:" protocol (makes debugging much easier, /kiss @rwaldron). Ensures test iframes will use the same jquery as specified in the main document. Known issue: chrome will cringe at cross-frame access using file: protocol, apparently chrome sees this as cross-domain... go figure.
  • Loading branch information
@jaubourg
jaubourg committed Mar 7, 2012
1 parent 92a92be commit 318d47b
Showing 1 changed file with 59 additions and 103 deletions.
162 test/data/include_js.php
View file
@@ -1,106 +1,62 @@
/*
<?php
// if php is available, close the comment so PHP can echo the appropriate JS
echo "*" . "/";
// initialize vars
$output = "";
$version = "";
// extract vars from referrer to determine version
if(isset($_SERVER['HTTP_REFERER'])){
$referrer = $_SERVER['HTTP_REFERER'];
$referrer_query_string = parse_url($_SERVER['HTTP_REFERER'], PHP_URL_QUERY );
parse_str($referrer_query_string, $referrer_params);
if(isset($referrer_params['jquery'])){
$version = $referrer_params['jquery'];
}
(function() {

window.hasPHP = false /* <?php echo "*" + "/ || true /*"; ?> */;

if ( !window.top.jQueryIncludes ) {

window.top.jQueryIncludes = (function() {

var location = window.top.document.location.href,
baseURL = location.replace( /\/test\/.+/, "/"),
version = /(?:&|\?)jquery=([^&]+?)(?:$|&)/.exec( location ),
includes, i;

if ( version ) {
version = version[ 1 ];
if( version === "min" ) {
includes = [ baseURL + "dist/jquery.min.js" ];
} else if( version === "dist" ) {
includes = [ baseURL + "dist/jquery.js" ];
} else {
includes = [ "http://code.jquery.com/jquery-" + version + ".js" ];
}
} else {
includes = [
"core",
"callbacks",
"deferred",
"support",
"data",
"queue",
"attributes",
"event",
"sizzle/sizzle",
"sizzle-jquery",
"traversing",
"manipulation",
"css",
"ajax",
"ajax/jsonp",
"ajax/script",
"ajax/xhr",
"effects",
"offset",
"dimensions",
"exports"
];
for ( i = includes.length; i--; ) {
includes[ i ] = baseURL + "src/" + includes[ i ] + ".js";
}
}

for ( i = includes.length; i--; ) {
includes[ i ] = "<script src='" + includes[ i ] + "'><" + "/script>";
}

return includes.join( "\n" );
})();
}

// load up built versions of jquery
if( $version === "min" ) {
$output = @file_get_contents("../../dist/jquery.min.js");
}elseif( $version === "dist" ) {
$output = @file_get_contents("../../dist/jquery.js");
}elseif( ctype_digit( substr( $version, 0, 1 )) || $version === "git" ) {
$output = "document.write('<script src=\"http://code.jquery.com/jquery-" . $version . ".js\"><'+'/script>');";
}
// the concatenated version of the the src files is both the default and the fallback
// because it does not require you to "make" jquery for it to update
if( $output === "" ) {
$files = array(
"intro",
"core",
"callbacks",
"deferred",
"support",
"data",
"queue",
"attributes",
"event",
"sizzle/sizzle",
"sizzle-jquery",
"traversing",
"manipulation",
"css",
"ajax",
"ajax/jsonp",
"ajax/script",
"ajax/xhr",
"effects",
"offset",
"dimensions",
"exports",
"outro"
);
foreach ( $files as $file ) {
$output .= file_get_contents( "../../src/" . $file . ".js" );
}
$output = str_replace( "(function( jQuery ) {", "", $output );
$output = str_replace( "})( jQuery );", "", $output );
}
echo $output;
die();
?>
*/

hasPHP = false;

// javascript fallback using src files in case this is not run on a PHP server!
// please note that this fallback is for convenience only, and is not fully supported
// i.e. don't expect all of the tests to work properly
var baseURL = document.location.href.replace( /\/test\/.+/, "/"),
files = [
"core",
"callbacks",
"deferred",
"support",
"data",
"queue",
"attributes",
"event",
"sizzle/sizzle",
"sizzle-jquery",
"traversing",
"manipulation",
"css",
"ajax",
"ajax/jsonp",
"ajax/script",
"ajax/xhr",
"effects",
"offset",
"dimensions",
"exports"
],
len = files.length,
i = 0;
document.write( window.top.jQueryIncludes );

for ( ; i < len; i++ ) {
document.write("<script src=\"" + baseURL + "src/" + files[ i ] + ".js\"><"+"/script>");
}
})();

10 comments on commit 318d47b

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

I'm disappointed by this commit, unless it fixes a bug. Does it fix a bug? I also wanted to get rid of the code duplication, but @rwldrn had asked me to wait till grunt was integrated before making further changes to the test suite. My plan was to have php invoke grunt, or something like that, each time the test suite were run, and only when you don't have PHP use the document.write fallback hack.

We had discussed in a jquery meeting that PHP was a requirement, but that we should make a base attempt at supporting testing without PHP. This change prioritizes the fallback method and loses any gains from having PHP installed.

The point of using PHP was to concatenate the source down to a single file, which also gets us testing the same source code as the built version of the code. Instead, we're back to including multiple files via document.write in every case. Also, I needed that single include file to test some work I'm doing on a ticket that uses a lazy loaded jQuery. When a lazy loaded script uses document.write to write multiple script tags, execution order is not preserved, so I guess I can't test that now :-/.

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

  1. Concatenating the files was making debugging a real nightmare (line number didn't have any meaning anymore).
  2. For the lazy loading script test, you'll need a special test page, correct? So why not use the dist file there? Do we really have to complicate things ad nauseum for a single test page?

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

  1. is a fair point. I didn't really think about that. I viewed it as a win by testing a single file that was equivalent to the real distributed jQuery, but you're right, it's harder to debug, so a loss there.
  2. I wasn't going to do a separate test page. I had an IFRAME test going in the regular suite. Can't use the dist file because I wanted it to run whenever the test suite was run. No one runs the other manual tests. They were in complete disrepair, and in fact, they were the inspiration for this initial consolidation. I hadn't viewed it as "complicating things ad nauseum". In fact, the initial commit I did for include_js.php simplified including jQuery in the test suite down to a single page.

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

regarding point 2) I see your point there, seeing as your test can be automated (most of the test pages I created in the past year all needed some kind of manual operation on the user's part). However, seeing as it means duplicating the logic server-side anyway, I'd still be in favor of having this in a separate file. What's the harm really? Hopefully, we'll move to nodejs at one point or another and not have duplication at all.

So, to summarize, I'd rather have good debugging and, since logic has to be duplicated, then separate the duplicates into two different files (include_js.php and concat_js.php?). Don't you think it would makes things a bit easier to follow overall?

Oh, also, I ensured all iframes using incluse_js.php would use the exact same file config as the main page in the rewrite (dunno if you noticed).

@rwaldron
Copy link
Member

Choose a reason for hiding this comment

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

You can follow the grunt progress here: https://docs.google.com/document/d/1Ne5YxQNJu_jpPLB3z8ockT7Hw569Np4wwkbSIN8Fw5A/edit

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

So, first off, "disappointed" was a bad word to use in my initial comment. I should've used a much more neutral word. Glad to see we're having a good discussion here despite my initial inflammatory words.

At the end of the day, I agree with you that the debugging clarity is more important than my one test and getting the testable source as close as possible to the real thing, for now. I also think your rewrite is clearer now that PHP and JS don't have to do the same thing. So let's not change this further, and I'll hold off working on my ticket until we come to a better solution

Once grunt get's fully integrated, let's revisit this, because I think there'll be a happy middle ground that'll accomplish what we both want.

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

No worries, Mike. I know how it feels to see changes happen out of the blue ;)

And I agree 100% with you on the happy middle ground to come once we get rid of all the non-sensical fat we have in our build process :)

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

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

I had to back this out, it was causing the TestSwarm runs to fail. Let's save the build changes for our 1.8 branch.

Before: http://swarm.jquery.org/job/999/

After: http://swarm.jquery.org/job/1006/ (Edit: WHY IS CHROME VERSION 15??)

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

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

Sweet justice :-P

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 318d47b Mar 8, 2012

Choose a reason for hiding this comment

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

window.hasPHP = false /* <?php echo "*" + "/ || true /*"; ?> */;

In contrary to JavaScript, PHP does actually have a separate operator for concatenation and mathematical addition. '+' is addition in PHP. Use . to concatenate strings in PHP...

Please sign in to comment.