Imagemin middleware #459

timkelty · 2017-11-21T02:46:13Z

Started here: #446

This seemed better suited for a middleware, so that we can apply a single set of imagemin options to both loader assets, as well as any assets written from other plugins (eg copy-webpack-plugin).

eliperelman · 2017-11-21T02:56:45Z

packages/imagemin/index.js

@@ -0,0 +1,46 @@
+const merge = require('deepmerge');
+
+const imageminLoader = require.resolve('imagemin-webpack/imagemin-loader');


Group the requires together, and move the resolve to its own grouping:

const merge = require('deepmerge'); const ImageminWebpackPlugin = require('imagemin-webpack/ImageminWebpackPlugin'); const gifsicle = require('imagemin-gifsicle'); const svgo = require('imagemin-svgo'); const pngquant = require('imagemin-pngquant'); const mozjpeg = require('imagemin-mozjpeg'); const webp = require('imagemin-webp'); const imageminLoader = require.resolve('imagemin-webpack/imagemin-loader');

eliperelman · 2017-11-21T03:01:05Z

packages/web/index.js

+      neutrino.use(chunk);
+      neutrino.use(minify);
+
+      if (options.imagemin) {


Is the goal to switch this to neutrino.options.optimize when it's available?

Yes, but also to pass options if desired, or disable imagemin by passing false.

timkelty · 2017-11-21T13:07:20Z

@eliperelman I rebased this off master.

Note: When this is merged into master, there will be a conflict that needs to resolved, as I've moved the .when(process.env.NODE_ENV === 'production', () => { below the .when(neutrino.options.command === 'build', (config) => {.

This is because we need the imagemin plugin to be applied after the @neutrinojs/copy stuff, so those images are optimized as well.

eliperelman · 2017-11-21T13:09:48Z

packages/imagemin/package.json

+    "image",
+    "imagemin"
+  ],
+  "author": "Eli Perelman <eli@eliperelman.com>",


Feel free to put your information here since you wrote this package. 😃

timkelty · 2017-11-21T13:22:30Z

Ok, made those change, plus rebased off master again to mitigate the above mentioned conflicts.

eliperelman · 2017-11-21T15:25:38Z

packages/imagemin/package.json

+  ],
+  "author": "Tim Kelty <timkelty@gmail.com>",
+  "license": "MPL-2.0",
+  "repository": "https://github.com/mozilla-neutrino/neutrino-dev/tree/master/packages/imagemin",


Does this repo format actually work on npm?

Hmmm not sure - I was just following suit with the rest of the middlewares: https://github.com/mozilla-neutrino/neutrino-dev/blob/master/packages/web/package.json#L14

wtf. When I was viewing this diff, it showed a different format...don't mind me.

edmorley · 2017-11-21T15:44:30Z

Do we know how deterministic the output images files are after being run though minification? It occurred to me that perhaps the process might not be deterministic and thus cause constant hash churn.

Continuing on from #446:

Should people instead be minimizing images once in the repo rather than at build time?

good question. All my experiences with Neutrino involve gitignoring the build folder, and building as part of the deploy process, so that doesn't really apply for me.

Furthermore, I always want my source assets pre-optimized, so I wouldn't want to commit optimized versions there.

To clarify, I don't mean store the Neutrino build directory in the repo. I mean outside the Neutrino build optimise your source directory assets once and commit them, to save the build time penalty every time. I agree one wouldn't want to lose the original Photoshop document or whatever, but normally that would be separate from the file being minimised anyway?

edmorley · 2017-11-21T15:46:19Z

I think this is a great middleware, and I'll probably end up using it myself in certain projects. I'm just trying to decide whether it would be best for it to be opt-in or opt-out. Knowing impact on build times and image sizes would with calculating a cost-benefit.

timkelty · 2017-11-21T15:51:55Z

good points, @edmorley.

Do we know how deterministic the output images files are after being run though minification? It occurred to me that perhaps the process might not be deterministic and thus cause constant hash churn.

Not certain. In my experiences the hash's have been the same, but I don't know for sure.

I'm just trying to decide whether it would be best for it to be opt-in or opt-out

I'd be fine with defaulting options.imagemin to false. This might also mitigate the potential heft added if people had huge images and weren't expecting it to exponentially affect their build time, and also would not assume that users weren't pre-optimizing their assets.

eliperelman · 2017-11-21T17:22:58Z

If we end up making this opt-in instead of opt-out, then there is no value in embedding this into the web middleware. They should just manually add it to their uses:

module.exports = {
  use: [
    '@neutrinojs/react',
    '@neutrinojs/imagemin'
  ]
};

timkelty · 2017-11-21T17:45:13Z

@eliperelman good point…

I'm fine either way. Maybe leave it out of web until we have a better idea how it will impact builds?

timkelty · 2017-11-21T22:11:33Z

@eliperelman @edmorley my most recent commit on this PR removes the middleware from web.

Reasoning:

there are some fairly heavy deps involved (imagemin-mozjpeg, etc), that I could see people being annoyed with having to install if they didn't want neutrino to optimize images
The plugin needs to be included AFTER any other plugins that add images to the manifest. Having it separate allows users to manage that themselves.
The above mentioned concerns about build times and deterministic file names

edmorley · 2017-11-21T22:27:34Z

Sounds good - we can always re-evaluate in the future depending on popularity :-)

eliperelman · 2017-11-27T18:50:48Z

packages/imagemin/README.md

+module.exports = {
+  use: [
+    ['@neutrinojs/imagemin', {
+	imagemin: {},


Indentation here seems off.

eliperelman · 2017-11-27T18:51:19Z

packages/imagemin/README.md

+## Customization
+
+`@neutrinojs/imagemin` creates some conventions to make overriding the configuration easier once you are
+ready to make changes.


Looks like we are missing the table of conventions here.

Added a Rules and Plugins table.

eliperelman · 2017-11-27T18:52:35Z

packages/web/index.js

@@ -192,5 +182,15 @@ module.exports = (neutrino, opts = {}) => {
      }

      config.output.filename('[name].[chunkhash].js');
+    })


Looks like this was moved without modification, so we should probably reset this file to what is in master.

eliperelman · 2017-11-27T20:03:00Z

packages/imagemin/README.md

+module.exports = {
+  use: [
+    ['@neutrinojs/imagemin', {
+			imagemin: {},


Heh, looks like we may have some mixed tabs in here. Make each indentation here 2 spaces.

Oh dear…sorry in the middle of changing-editors-hell 🥇

eliperelman · 2017-11-27T20:04:23Z

packages/imagemin/package.json

+  "homepage": "https://neutrino.js.org",
+  "bugs": "https://github.com/mozilla-neutrino/neutrino-dev/issues",
+  "dependencies": {
+    "deepmerge": "^2.0.0",


Use deepmerge ^1.5.2.

eliperelman · 2017-11-28T14:31:16Z

🎉

eliperelman suggested changes Nov 21, 2017

View reviewed changes

eliperelman reviewed Nov 21, 2017

View reviewed changes

timkelty force-pushed the imagemin-middleware branch from c961963 to 1a71210 Compare November 21, 2017 12:32

eliperelman reviewed Nov 21, 2017

View reviewed changes

timkelty force-pushed the imagemin-middleware branch from 670dc52 to a573e16 Compare November 21, 2017 13:21

eliperelman reviewed Nov 21, 2017

View reviewed changes

Tim Kelty added 12 commits November 21, 2017 16:48

Initial imagemin preset

d79f2da

Add imagemin middleware to web

1f22c4f

Combine chain

d1c1166

Initial commit

7d4b6b3

Fix package.json

e60a562

README

3a5e661

Match test from image-loader

56470fa

Add SVG

6433573

Group requires

88dd78f

Extrapolate options.plugin.test from existing rules.

eecea98

Author, deps.

40a3c9e

Remove imagemin from web preset

36db012

timkelty force-pushed the imagemin-middleware branch from a573e16 to 36db012 Compare November 21, 2017 22:06

eliperelman suggested changes Nov 27, 2017

View reviewed changes

Fix spacing

42d9d70

Tim Kelty added 2 commits November 27, 2017 14:15

Add Rules, Plugins table

ff798d4

Revert web changes.

62f4aba

eliperelman reviewed Nov 27, 2017

View reviewed changes

Tim Kelty added 2 commits November 27, 2017 15:13

Fix deepmerge dep

4fed652

Fixing spacing…again.

1b29460

eliperelman approved these changes Nov 28, 2017

View reviewed changes

eliperelman merged commit 5c8fa48 into neutrinojs:master Nov 28, 2017

edmorley mentioned this pull request Sep 17, 2018

Decide what to do with @neutrinojs/image-minify #1104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imagemin middleware #459

Imagemin middleware #459

timkelty commented Nov 21, 2017

eliperelman Nov 21, 2017

eliperelman Nov 21, 2017

timkelty Nov 21, 2017 •

edited

timkelty commented Nov 21, 2017

eliperelman Nov 21, 2017

timkelty commented Nov 21, 2017

eliperelman Nov 21, 2017

timkelty Nov 21, 2017

eliperelman Nov 21, 2017

edmorley commented Nov 21, 2017

edmorley commented Nov 21, 2017

timkelty commented Nov 21, 2017 •

edited

eliperelman commented Nov 21, 2017

timkelty commented Nov 21, 2017

timkelty commented Nov 21, 2017 •

edited

edmorley commented Nov 21, 2017

eliperelman Nov 27, 2017

eliperelman Nov 27, 2017

timkelty Nov 27, 2017

eliperelman Nov 27, 2017

eliperelman Nov 27, 2017

timkelty Nov 27, 2017

eliperelman Nov 27, 2017

eliperelman commented Nov 28, 2017

		@@ -0,0 +1,46 @@
		const merge = require('deepmerge');

		const imageminLoader = require.resolve('imagemin-webpack/imagemin-loader');

Imagemin middleware #459

Imagemin middleware #459

Conversation

timkelty commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkelty Nov 21, 2017 • edited

Choose a reason for hiding this comment

timkelty commented Nov 21, 2017

Choose a reason for hiding this comment

timkelty commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley commented Nov 21, 2017

edmorley commented Nov 21, 2017

timkelty commented Nov 21, 2017 • edited

eliperelman commented Nov 21, 2017

timkelty commented Nov 21, 2017

timkelty commented Nov 21, 2017 • edited

edmorley commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman commented Nov 28, 2017

timkelty Nov 21, 2017 •

edited

timkelty commented Nov 21, 2017 •

edited

timkelty commented Nov 21, 2017 •

edited