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

Minify more JS #8076

Merged
merged 6 commits into from
Nov 3, 2017
Merged

Minify more JS #8076

merged 6 commits into from
Nov 3, 2017

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented Oct 31, 2017 •
edited by ebinnion
Loading

Minifies all the JS files in _inc into _inc/build, with sourcemaps, and modifies existing code to point to those files.

This is factored out of our larger experimental #7963 branch.

@gravityrail gravityrail requested a review from a team as a code owner October 31, 2017 18:42
@gravityrail gravityrail self-assigned this Oct 31, 2017
@jeherve
Copy link
Member

jeherve commented Oct 31, 2017

This would replace #5620

Fixes #3487

Related: #5425

@jeherve jeherve added [Focus] Performance [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Oct 31, 2017
zinigor
zinigor reviewed Nov 1, 2017
View reviewed changes
Copy link
Member

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

This is looking great so far! I understand that it's still in development. When it's ready to ship we need to make sure we're not shipping unminified code anymore. Can you update the .svnignore file accordingly when you're done please?

gulpfile.babel.js Outdated
@@ -75,6 +75,17 @@ function onBuild( done ) {
} );
}

// uglify some other random files
gulp.src( [ '_inc/*.js', '!_inc/*.min.js' ] )
.pipe( sourcemaps.init() )
Copy link
Member

Choose a reason for hiding this comment

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

Can we make sure we only generate sourcemaps when it's a development, and not a production build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we also eliminate all the CSS .map files too? We already output those for production builds.

@ebinnion
Copy link
Contributor

ebinnion commented Nov 1, 2017

When it's ready to ship we need to make sure we're not shipping unminified code anymore.

This PR doesn't minify all files, so I don't think we're ready to do this project wide. But, I think we could ignore all non-minified files in _inc.

@ebinnion
Copy link
Contributor

ebinnion commented Nov 1, 2017

I tested several things such as Masterbar, Facebook embeds, etc., and they all seem to work.

@zinigor made a good point about not shipping sourcemaps in production and making sure we minimize shipping unminified scripts to production as well.

@gravityrail
Copy link
Contributor Author

@zinigor none of the files being minified are already minified.

I have added code which only compiles sourcemaps if the environment is not production.

I think this is good to go.

@gravityrail gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress labels Nov 1, 2017
@gravityrail gravityrail requested a review from zinigor November 1, 2017 23:01
@ebinnion
Copy link
Contributor

ebinnion commented Nov 2, 2017

@zinigor none of the files being minified are already minified.

I believe what @zinigor is suggesting, is that since we're going to start shipping minified files for the above, we should not also ship the unminified versions of those files, which will help reduce the download size a tad.

@ebinnion ebinnion force-pushed the add/minify-core-js branch 2 times, most recently from 735c1e2 to 2ce0183 Compare November 2, 2017 17:13
@ebinnion
Copy link
Contributor

ebinnion commented Nov 2, 2017

To test:

  • Checkout PR
  • yarn build
  • Test various parts of Jetpack
    • Test IDC by triggering an IDC and ensuring the notice shows
    • Trigger connection banner by deactivating and then reactivating Jetpack
    • Visit $site.com/wp-admin/admin.php?page=jetpack_modules to trigger the modules js files
    • Embed a Facebook post
    • etc.

Further, I also commented out a bit of the logic in deploy-to-master-stable.sh script so that it didn't actually commit to Git so that I could test the build. And it seemed to remove map files as expected as well

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 3, 2017
@ebinnion
Copy link
Contributor

ebinnion commented Nov 3, 2017 •
edited
Loading

After rebasing against master again, everything is now working as expected.

Testing instructions are in the previous comment. I'm going to go ahead and approve this since my changes were minor. I'll leave it to someone from pit or crew to merge though.

@gravityrail
Copy link
Contributor Author

LGTM! 👍

@gravityrail gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 3, 2017
@zinigor zinigor merged commit cb9b285 into master Nov 3, 2017
@zinigor zinigor deleted the add/minify-core-js branch November 3, 2017 17:27
@enejb
Copy link
Member

enejb commented Nov 16, 2017

I ❤️ this! Thanks for working on it.
@zinigor I think we should always ship the full versions of the js and css files for human readablity.
As per plugin item #4 in the plugin guidelines. Which suggests 'Minified code may be used, however the unminified versions should be included whenever possible. '
https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/

Or maybe just need to include a comment where to find the full human readable version of the code.
Also if we are shipping the full versions it would be good to add a conditional that checks for the SCRIPT_DEBUG and serves the full version when it is present.

@ebinnion
Copy link
Contributor

@oskosk and I chatted a bit about this in Slack today and I think we're going to move forward with shipping the minified and non-minified versions, and using SCRIPT_DEBUG to determine which one to load.

@ebinnion
Copy link
Contributor

I'm currently working on a similar PR to start addressing modules in #8153.

@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Nov 16, 2017
@ebinnion ebinnion mentioned this pull request Nov 20, 2017
Merged
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8076
713145f
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
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebinnion ebinnion ebinnion approved these changes

@georgestephanis georgestephanis Awaiting requested review from georgestephanis

@dereksmart dereksmart Awaiting requested review from dereksmart

@zinigor zinigor Awaiting requested review from zinigor

Assignees

@gravityrail gravityrail

Labels
[Focus] Performance Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gravityrail @jeherve @ebinnion @enejb @zinigor @kraftbj @matticbot