Skip to content

Build: Begin uglifying frontend JS files#5620

Closed
ebinnion wants to merge 1 commit intomasterfrom
update/gulp-minify-js
Closed

Build: Begin uglifying frontend JS files#5620
ebinnion wants to merge 1 commit intomasterfrom
update/gulp-minify-js

Conversation

@ebinnion
Copy link
Copy Markdown
Contributor

@ebinnion ebinnion commented Nov 14, 2016 •
edited
Loading

After running a performance report on my site this weekend, I realized that the frontend JS that we use for our modules is not minified/uglified. 😞

Before

screen shot 2016-11-28 at 4 13 37 pm

This PR seeks to use the uglified versions of much of the frontend JS that we load. Here is an after for comparison.

After

screen shot 2016-11-28 at 4 03 02 pm

The pros

You can see that I was able to knock off about 25 Kb with this PR. So, that's great! Plus, you can also see that the YSlow and PageSpeed scores both went up for my site. This will be welcomed by users.

Another pro is that we are only loading the uglified versions of these files on the frontend and when SCRIPT_DEBUG is not true. This will allow users the ability to opt out of the minified files if it is an issue for their setup.

The cons

The biggest cons I can think of are that this is not an automated solution. Future work will require that we first add the target file to gulpfile.js and then we must conditionally enqueue that. 😞

I had considered implementing a module to minify and concatenate all JS into one file, but that gets pretty interesting and is a bit out of this initial scope.

To test

  • Checkout update/gulp-minify-js branch
  • yarn build
  • Create test posts with tiled gallery, gist embed, recipe embed, instagram embed, and facebook embed
  • Ensure that minified files are loaded *.min.js
  • Ensure everything works and there are no JS errors
  • In wp-config.php, set define( 'SCRIPT_DEBUG', true );
  • Ensure that non-minified files are loaded
  • Check same posts and ensure everything works

@ebinnion ebinnion added this to the 4.4.1 milestone Nov 14, 2016
@ebinnion ebinnion self-assigned this Nov 14, 2016
@ebinnion ebinnion force-pushed the update/gulp-minify-js branch 2 times, most recently from b01f84d to ac7d11b Compare November 14, 2016 01:54
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Nov 14, 2016

Related: #5425

Would fix #3487, #3335

@ebinnion ebinnion force-pushed the update/gulp-minify-js branch from b337bd5 to 4b2e664 Compare November 28, 2016 19:50
@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Nov 28, 2016
@zinigor zinigor modified the milestones: 4.6, 4.4.2 Nov 29, 2016
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Nov 29, 2016

This is great, but I'm moving it to 4.6, sorry.

@eliorivero eliorivero mentioned this pull request Dec 31, 2016
Merged
@samhotchkiss
Copy link
Copy Markdown
Contributor

samhotchkiss commented Jan 20, 2017

@ebinnion -- is this still alive/in progress?

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Jan 20, 2017

It looks like it needs a rebase to bring it up to date, but it should be ready besides that.

@ebinnion ebinnion force-pushed the update/gulp-minify-js branch from ff4a870 to f21760b Compare January 20, 2017 22:18
@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Jan 20, 2017

@samhotchkiss - I've rebased the PR, so it should be ready for a review.

@jeherve jeherve modified the milestones: 2/17 - February, 4.7.0 - March 2017 Jan 30, 2017
@lezama
Copy link
Copy Markdown
Contributor

lezama commented Jan 31, 2017 •
edited
Loading

@ebinnion works as discrived!

My only concern with the changes is if they are compatible with merging them back to wpcom, with paths relying on JETPACK__PLUGIN_FILE

cc @georgestephanis

@lezama lezama added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 31, 2017
@samhotchkiss samhotchkiss removed this from the 4.7.0 - March 2017 milestone Feb 3, 2017
@samhotchkiss
Copy link
Copy Markdown
Contributor

samhotchkiss commented Feb 8, 2017

Good call, Miguel.

@georgestephanis, what do we normally do about that?

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Feb 8, 2017
@georgestephanis
Copy link
Copy Markdown
Contributor

georgestephanis commented Feb 8, 2017

@samhotchkiss Well, WPCOM handles the minifying on its own, so if we basically add in the conditional something like...

$min = ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG && ( ! defined( 'IS_WPCOM' ) || ! IS_WPCOM ) ) ? '.min' : '';
wp_register_script( 'script', "/path/to/script{$min}.js" );

So basically the .min would only ever be used on Jetpack.

(this could be avoided if wpcom has something in place to ensure min isn't pulled in by default, but it probably doesn't)

Also, I'd like to be absolutely clear that we are not able to use Uglify's mangle option as per plugin guidelines, that would count as obfuscating code. WordPress/wporg-plugin-guidelines#16 && WordPress/wporg-plugin-guidelines#8 -- minifying is fine.

@georgestephanis
Copy link
Copy Markdown
Contributor

georgestephanis commented Feb 8, 2017

Also, I would very much argue against locating the minified files in a different directory from the original files. They should be side by side for understandability.

@samhotchkiss
Copy link
Copy Markdown
Contributor

samhotchkiss commented Feb 10, 2017

@ebinnion, See George's comments above

@ebinnion ebinnion force-pushed the update/gulp-minify-js branch 2 times, most recently from 8e2260b to d8dc2d7 Compare March 3, 2017 18:45
@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 3, 2017
@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Mar 3, 2017

I just pushed a couple of commits that I believe address @georgestephanis's feedback. This PR should be ready for review again.

@georgestephanis
Copy link
Copy Markdown
Contributor

georgestephanis commented Mar 6, 2017

"that should not be source controlled" -- out of curiosity, do we have / need / should we consider a way to make sure Jetpack doesn't try to include minified sources for these files, if someone has just loaded a pre-built version from git? Or at least display an error saying how to build, instead of not working?

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Mar 6, 2017

That is definitely another route that we could go. I did not do that in this PR since I noticed at least one other place in the existing code where we were calling a minified file. And while briefly browsing, I see at least 4 places where we're directly calling minified JS files.

So, we'd want to update those calls to be conditional.

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented May 2, 2017

@samhotchkiss @georgestephanis do we want to move forward with this?

@samhotchkiss
Copy link
Copy Markdown
Contributor

samhotchkiss commented May 2, 2017

I'm good with this, @ebinnion

@zinigor, would you mind giving it a review at your convenience?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented May 30, 2017

Also suggested here:
https://wordpress.org/support/topic/non-minified-javascript-files/

@ebinnion ebinnion force-pushed the update/gulp-minify-js branch from 3b8fb60 to 626a382 Compare July 7, 2017 03:49
@ebinnion ebinnion force-pushed the update/gulp-minify-js branch from 626a382 to 194cde8 Compare July 7, 2017 03:52
@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Jul 7, 2017

@dereksmart - I rebased this PR and tested it manually, and it seems to be working. There's a failing unit test for some reason though. Have you ran into something like this?

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Jul 24, 2017

Closing since this doesn't seem to have traction. Feel free to open and take it over if you'd like.

@ebinnion ebinnion closed this Jul 24, 2017
@ebinnion ebinnion deleted the update/gulp-minify-js branch July 24, 2017 22:27
@ebinnion ebinnion removed the [Status] Needs Review This PR is ready for review. label Jul 24, 2017
@jeherve jeherve mentioned this pull request Oct 31, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ebinnion ebinnion

Labels

[Focus] Performance General [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. Touches WP.com Files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ebinnion @jeherve @zinigor @samhotchkiss @lezama @georgestephanis @matticbot