Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update/grunt to gulp: introduce js:uglify task #3647

Closed

Conversation

@jubstuff
Copy link
Contributor

commented Apr 11, 2016

Fixes #3487 .

Changes proposed in this Pull Request:

  • Introduce the js:uglify task to minify all .js files.
  • Load minified files when SCRIPT_DEBUG isn't defined or is false.

@nb

This comment has been minimized.

Copy link

commented on gulpfile.js in 29ac3df Apr 9, 2016

Managing this blacklist seems error-prone in the long-term, what can we do, so that we don’t need it?

This comment has been minimized.

Copy link
Owner Author

replied May 18, 2016

I have removed the blacklist, relying only on glob patterns.

@nb

This comment has been minimized.

Copy link

commented on package.json in 29ac3df Apr 9, 2016

Do we need this and jshint below?

This comment has been minimized.

Copy link
Owner Author

replied Apr 9, 2016

Are you referring to gulp-jshint? It acts only as a Gulp API to jshint, that is doing the real work.

@nb

This comment has been minimized.

Copy link

commented on gulpfile.js in 29ac3df Apr 9, 2016

Some spacing is missing here.

jubstuff added 4 commits Apr 9, 2016
There were already some minified scripts included in the repository. I removed them to avoid maintaining a blacklist in the js:uglify Gulp task.
@jeherve

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Repeating the same $min definition so many times doesn't seem ideal. Wouldn't there be a way to check once, and then refer to that check every time we enqueue a js file?

I would also recommend considering a few other factors in that check:

  • Are we on WordPress.com? If so, do not add min.
  • Is Jetpack in development mode? If so, do not add min
  • Include a filter allowing third-party tools to bypass minification.
@jubstuff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2016

Thank you @jeherve .
I've updated the code based on your suggestions.

Do you think something like that could work?

@jeherve

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

One potential issue here is that class.jetpack.php doesn't exist on WordPress.com. As a result, when we merge module files back to WordPress.com, every Jetpack::get_static_asset_suffix(); call will return an error, as the function won't exist on WordPress.com.

How about having no asset suffix by default, and add it if a specific filter is set to true?

@eliorivero

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

It's not entirely convenient to be able to debug a JS file only when SCRIPT_DEBUG is set. Users could have a JS issue on a site and they either don't know how to or can't add the constant to the wp-config.php file. Is there another technique we can better rely on to be able to debug minified JS in production sites?

@jubstuff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2016

@jeherve
Do you mean something along these lines?

$min = apply_filters( 'jetpack_load_minified_assets', false ) ? Jetpack::get_static_asset_suffix() : '';

@eliorivero
We could introduce a $_GET parameter to override the current state. So, for instance, a URL like http://mywebsite.com/?jetpack_load_unminified_asset=1 would load the unminified scripts.

For debugging purposes we could also add sourcemaps to the js:uglify Gulp task, if that could help

What do you think?

@eliorivero

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

Right, source maps are a convenient solution for debugging minified JS (dismiss the $_GET var).

@jubstuff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2016

I've tried another approach leveraging the script_loader_tag filter.
Instead of having to repeat the min suffix in eachwp_enqueue_script, the filter adds it when needed.

In this way the process is transparent: developers could use wp_enqueue_script without extra overhead.

*/
$load_minified_scripts = apply_filters( 'jetpack_load_minified_scripts', $load_minified_scripts );

if ( false !== strpos( $src, $jetpack_url ) && $load_minified_scripts ) {

This comment has been minimized.

Copy link
@beaulebens

beaulebens Apr 18, 2016

Member

This block feels pretty brittle, and prone to problems (especially if there are files along the lines of core.jsdom.js or something that has .js in amongst it.

You might need to parse the URLs a bit deeper and look at the basename, then regex or whatever to see that specifically the last 3 characters are .js.

Instead of simply replacing every `.js` that is present in the file
name, check just for the last three characters.
@jubstuff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

I have updated the code to reflect @beaulebens suggestions and added some more unit tests.

$load_minified_scripts = apply_filters( 'jetpack_load_minified_scripts', $load_minified_scripts );

if ( false !== strpos( $src, $jetpack_url ) && $load_minified_scripts ) {
$file_name = basename( parse_url( $src, PHP_URL_PATH ) );

This comment has been minimized.

Copy link
@beaulebens

beaulebens Apr 19, 2016

Member

need to strtolower() just in case?

This comment has been minimized.

Copy link
@beaulebens

beaulebens Apr 19, 2016

Member

Or on the line below (just for the comparison) to avoid "changing" the filename for case-sensitive OSes.

@jeherve jeherve added this to the 4.1 milestone Apr 28, 2016
@kraftbj kraftbj removed the Test on WP.com label May 5, 2016
jubstuff added 7 commits May 15, 2016
The net result of the pull request was showing a lot of modified files,
the only changes being the replacement of single quotes with double
quotes, that were used for string interpolation.
There was already a method used to minify assets. So it's better to merge the two and keep functionality in one place
The existing test for Jetpack assets would ignore files from the `_inc` folder and hardcoded Jetpack folder name.
$file = "{$base}/{$path}";
$full_path = JETPACK__PLUGIN_DIR . substr( $file, 8 );
$full_path = JETPACK__PLUGIN_DIR . substr( $file, strlen( basename( JETPACK__PLUGIN_DIR ) ) + 1 );

This comment has been minimized.

Copy link
@mdawaffe
mkdir( $base_path, 0777, true );
}
foreach ( $scripts as $script ) {
@touch( "$base_path/$script" );

This comment has been minimized.

Copy link
@mdawaffe

mdawaffe May 20, 2016

Member

Do we need to create these files for the test to work?

This comment has been minimized.

Copy link
@jubstuff

jubstuff May 20, 2016

Author Contributor

The Jetpack::maybe_min_asset method checks for file existence before returning the minified file URL (which I think is correct).
I thought about splitting the method, but since there were no tests, I wouldn't know if I had introduced regressions.

Do you think it is better for tests not to use the filesystem?

@jeherve jeherve modified the milestones: 4.2, 4.1 Jun 17, 2016
@jeherve jeherve modified the milestones: 4.3, 4.2 Jul 6, 2016
@richardmuscat richardmuscat modified the milestones: 4.3, 4.4 Jul 7, 2016
@dereksmart

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.