WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#25169 closed enhancement (fixed)

Optimize all images / pngs in core

Reported by: bradparbs Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: javascript, performance Cc:

Description

Right now, we can optimize all of the images included in core and save about 177kb - not bad for no loss in quality, etc.

Ran the images through pngcrush, saved about 7% overall.

Attachments (3)

25169.patch (2.0 MB) - added by bradparbs 5 years ago.
Oops - screwed up the diff, and then the naming - this is the right patch
25169.diff (1.4 KB) - added by jorbin 4 years ago.
25169.2.diff (1.3 KB) - added by wonderboymusic 4 years ago.

Download all attachments as: .zip

Change History (24)

@bradparbs
5 years ago

Oops - screwed up the diff, and then the naming - this is the right patch

#1 @jeremyfelt
5 years ago

I wonder if this is something that can be done through the build process with Grunt.

#2 follow-up: @bradparbs
5 years ago

Good point - that would be a way better solution that this bandaid I'm doing here.

I know there's grunt packages for pngcrush / image optimizations - not sure if we want to require a package for it though.

#3 in reply to: ↑ 2 ; follow-up: @nacin
5 years ago

Replying to bradparbs:

I know there's grunt packages for pngcrush / image optimizations - not sure if we want to require a package for it though.

Certainly — this is something we were planning to do.

#4 in reply to: ↑ 3 ; follow-up: @bradparbs
5 years ago

Replying to nacin:

Replying to bradparbs:

I know there's grunt packages for pngcrush / image optimizations - not sure if we want to require a package for it though.

Certainly — this is something we were planning to do.

Cool - have it all set up in Grunt right now, but npm just broke - will have a patch as soon as I fix that locally.

For that patch - same ticket or make a new one?

#5 in reply to: ↑ 4 @nacin
5 years ago

Replying to bradparbs:

For that patch - same ticket or make a new one?

This one sounds great.

#6 @jeremyfelt
5 years ago

  • Keywords has-patch removed
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from trivial to normal

#7 @nacin
4 years ago

  • Component changed from Appearance to Build/Test Tools

@jorbin
4 years ago

#8 follow-up: @jorbin
4 years ago

  • Focuses javascript performance added
  • Keywords has-patch added
  • Milestone changed from Future Release to 3.9

Luckily, many of our files are already pretty optimized. This only saves saved 143.81 kB, but it also means we don't need to worry as much going forward.

This comes with a bit of a time cost the first time you run it, but subsequent builds are not noticeably different.

I'm using grunt-contrib-imagmin 0.4.1 rather than the newer 0.5 since 0.5 is using a new library that seems to throw errors on subsequent runs. I'm looking into that upstream.

#9 @wonderboymusic
4 years ago

  • Keywords commit added

we use this at the Times, runs every time we build, which is often, not a buzz kill. +1

#10 @wonderboymusic
4 years ago

Just ran this a bunch of times, it is only sorta slow the very first time it runs. After that, it's way faster than uglify. .2.diff unJorbins the whitespace.

Something to think about - could this just be a dev tool that generates smushed images that we then check in?

#11 @nacin
4 years ago

In 27172:

Dev tools: Add grunt imagemin:core task for optimizing images pre-commit.

Adds grunt-contrib-imagemin, so an npm install will be required.

props jorbin, wonderboymusic for initial patches.
see #25169.

#12 @nacin
4 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27173:

Optimize 69 images (of 127) using the new Grunt task. (See [27172].)

fixes #25169.

#13 @nacin
4 years ago

I'm with wonderboymusic on this being a pre-commit tool rather than a build tool. It can essentially be in the same boat as autoprefixer (#27078). Maybe we'll hook it up to build later, but I think just having the task there is a good first step.

Since it'll be applying these changes directly to files in src, we need to be really restrictive of which images we apply them to. I made it imagemin:core as imagemin:themes makes sense in the future. That said, all of our theme screenshots are currently perfect, and the optimization of jpeg header images and such is something that is done by hand — balancing quality for file size. So rather than coming up with a rule that would essentially be just screenshots, I left it out for the moment.

This ticket was mentioned in IRC in #wordpress-dev by jorbin. View the logs.


4 years ago

#15 @nacin
4 years ago

In 27176:

Add grunt precommit for running front-end dev and test tasks before commit.

This includes autoprefixer, imagemin, jshint, and qunit.

props jorbin.
fixes #27121. see #25169 and #27078.

#16 follow-up: @netweb
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This causes numerous issues with npm install on Windows installs :(

I am reopening the ticket because of the console errors from running npm install. Previously to the addition of grunt-contrib-imagemin to package.json it was an error free build environment for Windows contributors.

Granted I don't think any of the core committers are running Windows to commit against core and the crux of this patch is for the the Grunt precommit task.

I tested both grunt-contrib-imagemin v0.4.1 and v0.5.0 and the errors are very similar for both versions.

Using v0.5.0 fails much more gracefully, you still get similar errors but subsequent runs of npm install does not try to reinstall the missing component.

npm install

Using: "grunt-contrib-imagemin" : "~0.4.1",

Using: `"grunt-contrib-imagemin" : "~0.5.0",

  • npm WARN optional dep failed, continuing optipng-bin@0.3.1

grunt build

Using: "grunt-contrib-imagemin" : "~0.4.1",

  • >> Local Npm module "grunt-contrib-imagemin" not found. Is it installed?
  • Done, without errors.

Using: `"grunt-contrib-imagemin" : "~0.5.0",

  • Done, without errors.

grunt precommit

Running "imagemin:core" (imagemin) task
Warning: Cannot find module 'optipng-bin' Use --force to continue.

Aborted due to warnings.
C:\xampp\htdocs\develop.wp.nw [trunk]> 

#17 in reply to: ↑ 8 @netweb
4 years ago

Replying to jorbin:

I'm using grunt-contrib-imagmin 0.4.1 rather than the newer 0.5 since 0.5 is using a new library that seems to throw errors on subsequent runs. I'm looking into that upstream.

From a quick bit of research it looks like the optional dependencies will fail a little more gracefully in v0.5.0 and going further upstream:

grunt-contrib-imagemin currently has a dependency of image-min v0.1.1

  • v~0.1.1 has a dependency on optipng-bin
  • v~0.1.2 has optipng-bin as an optional dependency
  • v~0.1.3 removes the optipng-bin dependency in favour of pngquant-bin

I submitted a pull request https://github.com/gruntjs/grunt-contrib-imagemin/pull/152 to bump the grunt-contrib-imagemin dependency of image-min to v0.1.3

Maybe reclose this ticket and create a new one for updating core build to grunt-contrib-imagmin to v0.5.x once all the upstream issues are fixed? (I can survive with the errors in Windows for a while)

#18 in reply to: ↑ 16 ; follow-up: @azaozz
4 years ago

Replying to netweb:

This causes numerous issues with npm install on Windows installs :(

Installs properly here on Win7.

However I'm 50/50 on whether this should be running automatically on each build. We are up to 46MB! of build tools (node_modules), 7,428 files in 1,699 directories... The chances for failed updates, platform specific bugs, local and platform differences, different output because of different version of the build tools, etc. are quite high. Our build process shouldn't be "fragile".

Considering that new images are added to core only a few times per cycle, seems this can be a "manual" task, i.e. should be run with grunt imagemin only when needed.

#19 in reply to: ↑ 18 @netweb
4 years ago

  • Keywords close added

Replying to azaozz:

Replying to netweb:

This causes numerous issues with npm install on Windows installs :(

Installs properly here on Win7.

After spending the best part of last hour of constantly running npm install (after a npm cache clean) it looks like everything has installed correctly. Looks like the errors were further upstream in not downloading all the packages from https://registry.npmjs.org

All grunt tasks complete successfully with Done, without errors.

Replying to azaozz:

However I'm 50/50 on whether this should be running automatically on each build. We are up to 46MB! of build tools (node_modules), 7,428 files in 1,699 directories... The chances for failed updates, platform specific bugs, local and platform differences, different output because of different version of the build tools, etc. are quite high. Our build process shouldn't be "fragile".

Considering that new images are added to core only a few times per cycle, seems this can be a "manual" task, i.e. should be run with grunt imagemin only when needed.

In theory irrespective of platform we should all be using the same build tool/s version/s per the package.json file, however we do have to do an old fashioned grey matter remember to do task to run npm install occasionly to add/update any tool/s changes in package.json.

Forking github-update-checker could be a solution to this, compare versions in your local package.json with the latest in the WordPress Core repo.

The task is only run manually as part of the grunt precommit task, it is NOT part of the grunt build task.

// Pre-commit task. 
grunt.registerTask('precommit', 'Runs front-end dev/test tasks in preparation for a commit.',
['autoprefixer:core', 'imagemin:core', 'jshint', 'qunit:compiled']);

Added 'close' and this can be reclosed, worksforme :)

#20 @jorbin
4 years ago

However I'm 50/50 on whether this should be running automatically on each build. We are up to 46MB! of build tools (node_modules), 7,428 files in 1,699 directories... The chances for failed updates, platform specific bugs, local and platform differences, different output because of different version of the build tools, etc. are quite high. Our build process shouldn't be "fragile".

Considering that new images are added to core only a few times per cycle, seems this can be a "manual" task, i.e. should be run with grunt imagemin only when needed.

I completely agree that our build process shouldn't be fragile. It also shouldn't require much cognitive load or knowledge to run. Yes, updating images is not a common tasks, but you shouldn't need to think "Well I'm adding images, so I need to run this task, but I'm also modifying CSS, so I need to run this other task". By including all front end tasks that we expect to be run before a commit into one task, we reduce the cognitive load on developers and require them to only run one task.

#21 @helen
4 years ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.