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.

Sign up for GitHub

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

Jump to bottom

Build: First pass at minifying module JS #8153

Merged
merged 2 commits into from
Nov 18, 2017
Merged

Build: First pass at minifying module JS #8153

merged 2 commits into from
Nov 18, 2017

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Nov 13, 2017 •
edited
Loading

As part of a front-end performance audit that @gravityrail and I are doing, this PR will start minifying all front-end assets that are loaded from the shortcodes module.

To test:

  • Checkout branch
  • yarn build
  • Ensure that license is at top of the output for this command:
    • more _inc/build/shortcodes/js/jmpress.min.js
  • Test the following shortcodes:
    • brightcove
    • gist
    • instagram
    • presentation
    • quiz
    • recipe
    • slideshow
  • Ensure there are no JS errors on the frontend and that shortcodes are still functional
  • In wp-config.php, add define( 'SCRIPT_DEBUG, true );` and check that non-minified files are now enqueued by checking the source

Note: I am removing modules/shortcodes/js/jmpress.min.js from source control since we'll now build it.

@ebinnion ebinnion self-assigned this Nov 13, 2017
@ebinnion ebinnion requested a review from a team as a code owner November 13, 2017 20:44
@ebinnion ebinnion force-pushed the update/minify-module-js branch from aad15a1 to b4dec19 Compare November 13, 2017 23:44
gravityrail
gravityrail approved these changes Nov 14, 2017
View reviewed changes
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

This looks great!

@ebinnion ebinnion force-pushed the update/minify-module-js branch 3 times, most recently from 40db227 to 35a94e7 Compare November 15, 2017 00:01
@ebinnion ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 15, 2017
@ebinnion
Copy link
Contributor Author

I've now scoped this changeset down to the shortcodes module for the time being. This will make the PR easier to review for the time being and allows us to verify this approach without investing time in changing all the files that enqueue JS multiple times.

@ebinnion ebinnion added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 15, 2017
@ebinnion
Copy link
Contributor Author

Looks like there's an issue with sourcemap paths. I'll look into this tomorrow.

@oskosk
Copy link
Contributor

oskosk commented Nov 15, 2017 •
edited
Loading

I've just checked this branch and have these observations:

  • On the frontend I still keep getting served modules/shortcodes/js/jmpress.min.js?ver=0.4.5
  • You mention in the description that modules/shortcodes/js/jmpress.min.js is ignored now in this PR but .gitignore states that the whole /modules/shortcodes/js is to be ignored ).
  • I ran the build and I never got a file _inc/build/shortcodes/js/jmpress.min.js built.
  • After running yarn build again I realized by going into the _inc/build directory that the jmpress.min.js and jmpress.min.js.map are left in _inc/build and not in _inc/build/js but loading the frontend results in that I'm still served the original min.js in modules/shortcodes/js/

EDIT: All of the above was me testing on an old version of this branch.

@ebinnion ebinnion force-pushed the update/minify-module-js branch from c1a5419 to fc18ef3 Compare November 15, 2017 20:08
@ebinnion ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 15, 2017
@ebinnion
Copy link
Contributor Author

I have fixed the issue that I noticed yesterday with sourcemaps. The issue was that we were piping to banner between sourcemaps init and sourcemaps write. As soon as I moved the pipe to banner to above sourcemaps init, all was well with the world.

oskosk
oskosk reviewed Nov 15, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Searching the codebase for modules/shortcodes/js/jmpress.min.js says we are referencing some of the files removed or changed by this PR in .jshintignore. Should it be updated ?

oskosk
oskosk approved these changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Tested well with all shortcodes. Apart from the comment I left about .jshintignore, PR looks great!

oskosk
oskosk previously requested changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So, this tests well. @dereksmart how should we handle the new .map files regarding .svnignore... We don't want those in the plugin directory... Can we solve this doubt before merging ?

@oskosk oskosk added this to the 5.6 milestone Nov 16, 2017
@dereksmart
Copy link
Member

We don't want those in the plugin directory

Correct, we want to make sure any .map files are added to svnignore, as we're doing with the admin UI map files.

@ebinnion ebinnion mentioned this pull request Nov 16, 2017
Merged
@ebinnion ebinnion force-pushed the update/minify-module-js branch from fc18ef3 to a5eb68f Compare November 17, 2017 18:59
@ebinnion
Copy link
Contributor Author

I've addressed feedback such that we are now:

  • Conditionally enqueueing min or non-min JS based on SCRIPT_DEBUG
  • Properly .svnignoreing map files

As a follow-up to this PR, I'll move other JS and CSS map files to the _inc/build/maps directory as well.

@ebinnion ebinnion force-pushed the update/minify-module-js branch from a78ff1d to 4679eeb Compare November 17, 2017 19:10
@ebinnion ebinnion force-pushed the update/minify-module-js branch from 4679eeb to 1ce178a Compare November 17, 2017 19:15
@ebinnion ebinnion dismissed oskosk’s stale review November 17, 2017 22:25

Feedback has been addressed

@gravityrail gravityrail merged commit cadaa58 into master Nov 18, 2017
@gravityrail gravityrail deleted the update/minify-module-js branch November 18, 2017 04:16
@gravityrail gravityrail removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 18, 2017
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 20, 2017
@ebinnion ebinnion mentioned this pull request Nov 20, 2017
Merged
@Umangvaghela Umangvaghela mentioned this pull request Nov 24, 2017
Merged
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 24, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8153
2cfd951
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gravityrail gravityrail gravityrail approved these changes

@oskosk oskosk oskosk left review comments

Assignees

@ebinnion ebinnion

Labels
[Focus] Performance General Touches WP.com Files
Projects
None yet
Milestone
5.6
Development

Successfully merging this pull request may close these issues.
6 participants
@ebinnion @oskosk @dereksmart @gravityrail @jeherve @matticbot