Automate script and style compression and bumps

[nacin]

Commits with CSS changes (and to a lesser extent JS changes) are polluted with thousands of characters of scripts and styles. This was agitated further in 3.3 with the merge of most CSS into a single file.

These commits are more difficult to review, both for wp-svn and on Trac, and most certainly get less eyes.

Here's a tiered proposal to fix this:

1. After a commit that edits a .dev.css or .dev.js file, a bot should compress all relevant files, bump all relevant version numbers, and commit this as a subsequent revision.

2. Use a single version number for all scripts and styles. For users, the only necessary version number is the WordPress version. The individual version numbers benefit development only. We should merge this into a single variable to be bumped, thus invalidating all cache files, to make it simple.

3. This single variable should actually be $wp_version. We already manually add SVN revision numbers to beta and RC versions to provide additional context in nightly builds. The bot can instead be charged with bumping $wp_version to add or update a revision number. We will still be able to update (or remove, in the case of a pre-release or final release) the revision number on our own, as necessary.

Attached patch handles 2 and 3. The bot still needs to be written.

[ocean90]

I try to understand the concept behind your patch. You want to remove the existing version strings from WordPress files, but leave it for third-party scripts?

[scribu]

@ocean90: I think the idea is that all the internal WP scripts can share a single version number. Third-party scripts, such as jQuery, will maintain their existing version.

+1 for the bot, by the way. It's the best compromise between clean commits and keeping the compressed versions under svn.

[azaozz]

+1. My suggestion is that the bot uses the output from YUI Compressor in the commit message. That would be good for JS files as the compressor (minifier) runs them through ​Rhino and catches any JS errors or outputs some helpful tips at the end.

[ryan]

Sounds good.

[nacin]

scribu and ocean90: If you don't specify a version number on register/enqueue, it falls back to $wp_version already. So we're just making all core scripts fall back to $wp_version and bumping $wp_version whenever we need to invalidate cache. We already do this during RCs and betas, and it wouldn't be a bad thing to have for alpha.

[nacin]

[19618] [19619] [19620] — the bot has been written and tested, though not set up to automatically compress scripts yet. (A bot account will be set up, it won't continue to occur against my name.)

[nacin]

In [19621]:

Remove individual version numbers for core scripts and styles. Fall back to the WP version (well, that already happens). $wp_version should now be bumped (and soon will occur automatically). see #19592.

[ocean90]

Replying to nacin:

(A bot account will be set up, it won't continue to occur against my name.)

Oh, I'm fine with using nacin as a bot. :)

[westi]

Can we see and review the bot code before we push this live.

I also like @azaozz's idea of doing something with the output from YUI - maybe we should be running Rhino as a pre-commit hook for js files as well to block commits with JS errors

[nacin]

bumpbot.php attached. It captures warning output, but doesn't currently do anything with it.

I originally aimed to write it to function as a post-commit hook and then identify any .dev.* files in the previous commit. This has the benefit of easily identifying where the change was made, with the potential of extracting a ticket number (for warnings to be posted), etc. After starting down that path, I came up with the existing code, which would run on cron (for now, probably on my sandbox) and simply attempt to compress all dev.css and dev.js files it finds in trunk. If it doesn't diff clean, it results in a commit.

I agree, JSLint should probably be run as a pre-commit hook.

[nacin]

In [19790]:

Give install.css on setup-config.php some cache busting. see #19592.

[nacin]

In [19803]:

Compress CSS/JS using the latest version of YUI Compressor, 2.4.7. Removes final semicolon in a CSS declaration block. Previously, committers used 2.4.2. see #19592.

[ocean90]

r19946 => r19947 => r19948

azaozz:

bumpbot may need to be UTF-8 aware.

[duck_]

Replying to ocean90:

azaozz:

bumpbot may need to be UTF-8 aware.

In this case it's ISO-8859-1 that's required. Adding --charset ISO-8859-1 to the set of arguments passed to yui-compressor.jar fixes this in local testing, but there could be side effects on other files doing this for everything so it could be restricted to tinymce sources.

[ocean90]

There was another one in r19718.

[nacin]

r19718 was fixed by using UTF-8. I turned off bumpbot and will experiment.

[azaozz]

Replying to duck_:

In this case it's ISO-8859-1 that's required.

My bad, it was late and didn't look into this more. It seems we may be running into this whenever non-ascii chars are present in the files. From YUI Compressor's description:

--charset character-set
      If a supported character set is specified, the YUI Compressor will use it
      to read the input file. Otherwise, it will assume that the platform's
      default character set is being used. The output file is encoded using
      the same character set.

Thinking we should keep it UTF-8 as that affects the output too. Perhaps it's worth it having some basic charset detection or running mb_detect_encoding() and then iconv. Alternatively can try to detect invalid chars in the output iconv("UTF-8","UTF-8", $string) and not commit that minified file, showing a warning in the commit message.

In that particular case the file comes pre-minified in the TinyMCE distribution package and we will be skipping processing the pre-minified files there as discussed with @nacin in IRC.

[azaozz]

In [19951]:

FIx charset in minified file, see #19592

[nacin]

In [19952]:

Update TinyMCE mark loaded src. see #19592.

[nacin]

In [19953]:

Update wp-tinymce.js.gz.

• Plugins are now sorted alphabetically during concatenation, to ensure consistent generation.
• Key tinymce/wp-tinymce.js.gz off the revision number, rather than the date.

see #19592.

[nacin]

bumpbot will now handle any TinyMCE _src.js files, as long as they are not an external TinyMCE plugin (it whitelists the 'wordpress' plugin and any that match wp*).

And, if wp-tinymce.js.gz needs to be updated, bumpbot will do so, and bump $tinymce_version in the process.

So, a future TinyMCE update should simply update tiny_mce.js and any other files, as well as update $tinymce_version to be the new version of TinyMCE. bumpbot will then come by and update wp-tinymce.js.gz and bump the version string to the current revision number.

[azaozz]

Replying to nacin:

Works great, good job :)

[nacin]

In [19996]:

Correct [19952]. see #19592.

[nacin]

bumpbot-warnings.txt​ is a list of every warning that YUI returns for core JavaScript files. (It's a rough output. A single "1" means the file was minified without issue.)

If someone is willing to patch all of these, I can make it so bumpbot posts comments to tickets that cause warnings.

[soulseekah]

Replying to nacin:

If someone is willing to patch all of these, I can make it so bumpbot posts comments to tickets that cause warnings.

Most of the warnings are "try to use only one --> var <-- ". Readability of JS code is severely impaired when fixing these. Refer to the experimental diff resolving all WARNINGs in /wp-admin/js/theme.dev.js and /wp-admin/js/tags.dev.js

Most of these are best left alone, not really worth chasing down the 3 extra bytes here and there, although there are cases where it's possible to fix up two or three of these without much movement around. Is it worth the hunt? By removing all of them we can potentially save around 480 bytes (160 single var warnings * 3 bytes).

Replying to ​bumpbot-warnings.txt L75

wp-admin/js/widgets.dev.js? => [WARNING] The symbol isRTL is declared but is apparently never used.

isRTL is used in the ​same line. By expanding breaking down the lines and not using defining later, the error magically goes away. Doesn't make sense.

Replying to ​bumpbot-warnings.txt L93

wp-includes/js/jcrop/jquery.Jcrop.dev.js? => [WARNING] The variable obj has already been declared in the same scope... =function(obj,opt){var ---> obj <--- =obj,opt=opt;if(typeof

obj is the argument ​already, makes no sense to var obj = obj , no? Do we optimize third party libraries? This Jcrop library is one of the largest warning generators.

Replying to ​bumpbot-warnings.txt L335

WARNING] The variable t has already been declared in the same scope... (this.use_gebi&&e){var ---> t <--- =e.originalTarget;while(t.parentNode

Patched up some of the relevant ​colorpicker warnings, it did have an unused variable.

Overall should we be listening to some sort of Lint instead of a compressor, the main purpose of which is to keep the bytecount down?

+ Today got a couple of warnings from the newer:

53/64: ./wp-includes/js/customize-controls.dev.js
[WARNING] Found an undeclared symbol: wp
(exports,$){var api= ---> wp <--- .customize;api.Previewer=api.Messenger

Parts are still in very active development so no point in touching them yet.

Also, attached is a stripped-down bumpbot for local use, if anyone one wants to run on local trunks for debugging and patching. Where can I get the latest bumpbot source code, btw?

[soulseekah]

A local stipped-down bumpbot for local debugging

[soulseekah]

Experimental poc to show readability decrease

[soulseekah]

removes 3 warnings

[azaozz]

Replying to soulseekah:

Most of the warnings are "try to use only one --> var <-- ". Readability of JS code is severely impaired when fixing these...
...
Most of these are best left alone, not really worth chasing down the 3 extra bytes here and there...

This is rather a question of coding standards not of trying to save few bytes. It is a good practice to declare all local variables at the beginning of each scope (i.e. have one var ...; at the top of each function). This actually helps readability and is preferred in most (all?) IDEs that have JS support. (It also seems to simplify the minifying in YUI Compressor).

Overall should we be listening to some sort of Lint instead of a compressor...

The YUI Compressor uses ​Rhino to actually execute the JS in java, not just "look through" the code. Seems most warnings are generated from there. It would also catch fatal errors, etc. making it better than other "lint" processors.

[mbijon]

What is the preferred JS coding standard now? Currently the Codex has a standards section & page for CSS, ​http://codex.wordpress.org/WordPress_Coding_Standards#CSS, but no equivalent on JS. Since we're using the YUI compressor I think those are where we should start, if only to reduce errors in this build bot. As @nacin's long warnings log shows that will require a lot of minor refactoring of our current JS. However, this does go against past @nacin warnings on Trac that minor refactors won't get added to the WP codebase.

(BTW, I'm game to start adding JS standards to the Codex if there is some general agreement on YUI's standards being good for WP to start with.)

Specific to those "use only one --> var <--" warnings: the YUI compressor seems to prefer variable declarations in a format like:

var useWindow=false,
    coordinates=new Object(),
    x=0,
    y=0; 

The standard I've seen more often is from JSLint/Crockford and also happens to match WP's trend toward more-readable code:

var useWindow=false;
var coordinates=new Object();
var x=0;
var y=0; 

Which is preferred, readability or reduced buildbot warnings?

[azaozz]

Replying to mbijon:

What is the preferred JS coding standard...

Frankly I prefer all vars to be at the top and in one declaration, that was the usual way in the early JS days I think (similar to camel case var names) :)

The coding standard for JS is roughly the same as for PHP with some language specific exceptions.

Specific to those "use only one --> var <--" warnings...

These actually should be:

var useWindow = false, coordinates = {}, x = 0, y = 0; 

Note the spaces around the = and the shorthand for defining new object.

The standard I've seen more often is from JSLint/Crockford and also happens to match WP's trend toward more-readable code...

Don't think repeating var on each line makes it more readable but that's just a personal opinion. It makes it to look more like a PHP class declaration, not JS. In WP almost all JS vars are defined on the same line with one var and that's from before we started using YUI.

[mbijon]

Thanks for the guidance, that should make it easy (easier?) to refactor enough to fix many of those warnings.

Replying to azaozz:

These actually should be:

var useWindow = false, coordinates = {}, x = 0, y = 0; 

Note the spaces around the = and the shorthand for defining new object.

I like the object-creation shorthand. Not sure if that's part of this YUI package but will test later.

The standard I've seen more often is from JSLint/Crockford and also happens to match WP's trend toward more-readable code...

Don't think repeating var on each line makes it more readable but that's just a personal opinion. It makes it to look more like a PHP class declaration, not JS. In WP almost all JS vars are defined on the same line with one var and that's from before we started using YUI.

One line is fine by me. I'm not a fan of declaring var inline or as-needed, ven if the block of declarations has conditionally un-used `vars