Added support for image optimiziation with image-webpack-loader #675

aschempp · 2019-11-23T15:09:30Z

as discussed with @weaverryan here's a first POC for adding image optimization. Looking for general feedback as this is my first time working with Encore, so I'm not sure if my approaches make sense.

The method and feature name are kinda confusing due to the dependencies. We're using image-webpack-loader, which internally uses imagemin to optimize images. Not sure if stuff should be named after the loader (as of now), or rather something like enableImageOptimization() or name the feature imagemin instead of image-webpack…

To enable image optimization, install image-webpack-loader and call enableImagemin() in the Encore config. Further image configuration is available by passing options to the method exactly as in image-webpack-loader.

Encore.enableImagemin({
    mozjpeg: {
        progressive: true,
        quality: 65
    },
    // optipng.enabled: false will disable optipng
    optipng: {
        enabled: false,
    },
    pngquant: {
        quality: [0.65, 0.90],
        speed: 4
    },
    gifsicle: {
        interlaced: false,
    },
    // the webp option will enable WEBP
    webp: {
        quality: 75
    }
});

#SymfonyHackday

Lyrkan

Hi @aschempp,

This looks like a nice thing to add, thanks for working on it :)

Also, don't worry about the failing AppVeyor tests, they are not related to your PR.

lib/config-generator.js

index.js

aschempp · 2020-01-07T21:57:50Z

I finally got around to update this PR. It's pretty much a rewrite of the original implementation, thanks to the suggestions @Lyrkan. I have also update the initial PR description.

One caveat is that the signature of the images loaders has changes due to the nature of Webpack and chaining loaders. I'm not sure if this is really a problem, and we could try to keep BC of the signature for as long as Imagemin is not enabled if necessary?

aschempp · 2020-01-18T14:01:49Z

failing tests don't see related to the PR 🙃

Lyrkan

Hey @aschempp,

Sorry for not reviewing this sooner, I don't think the failing tests are related either.

I've some few comments but that's a great PR :)

One caveat is that the signature of the images loaders has changes due to the nature of Webpack and chaining loaders. I'm not sure if this is really a problem, and we could try to keep BC of the signature for as long as Imagemin is not enabled if necessary?

I would say that's part of Encore's internals and that we shouldn't care too much about BC there... if users are modifying loaders manually they should already know that things could break in a future release (especially when using configureLoaderRule() since it comes with a warning).

index.js

lib/WebpackConfig.js

aschempp · 2020-02-28T14:33:22Z

any feedback on this PR? 🙃

weaverryan · 2020-03-19T10:33:44Z

Sorry for the delay - I have this on my list to check soon - it's something that I think would be super cool :)

weaverryan

Wow! This is really excellent work - seriously :).

I have one comment on a "decision" you made about disabled. It is the only issue I see with this PR :).

weaverryan · 2020-03-20T13:49:30Z

lib/loaders/images.js

+            loaderFeatures.ensurePackagesExistAndAreCorrectVersion('imagemin');
+
+            const imageminOptions = {
+                disable: !webpackConfig.isProduction(),


Hmm, I'm not sure about this. I could see a user enabling this feature while developing, then checking the images and saying "yep! they look ok still!. Then on deploy, their images are now a "lower quality". I think we should just allow the user to control whether or not this is disabled entirely.

🤔 that should just be the default value, and one can just override that in the webpack/encore configuration?

If I call Encore.enableImagemin(), I would expect that my images would be optimized. NOT optimizing them is unexpected. So I think Encore needs to not have this disable option at all. Nice and simple: if they call Encore.enableImagemin(), we enable it. Then, if they choose to, they could decide to disable in dev.

I do realize why you're doing this... it's just too unexpected. One other option would be to add an isEnabled flag as the first argument to Encore.enableImagemin(). Then we would document you using it like this:

Encore.enableImagemin(Encore.isProduction());

Sorry for the delay. I decided to remove the disabled-in-dev default. You're probably right that it's unexpected, and it's easy to add. I thought about the additional option as well, but it just adds unnecessary complexity, I can achieve the same using an options object.

weaverryan · 2020-04-17T11:10:41Z

Ping! I think we're close - but there is one last big issue :)

aschempp · 2020-06-11T13:26:19Z

Finally, tests are fixed! Not sure how to fix the frozen yarn file (I'm not using yarn 🙈 )

Kocal · 2020-06-11T13:33:09Z

I'm not using yarn

But you should! 😄

The error happens because --frozen-lockfile actually check if the package.json and yarn.lock files are synchronized. If it's not the case, it fails. It's the equivalent of npm ci.

Here, it's not the case. You've added image-webpack-loader dependency in the package.json file (https://github.com/symfony/webpack-encore/pull/675/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2) but the yarn.lock file has not been updated.

You need to run yarn and commit updates, but the next time please run yarn add --dev image-webpack-loader directly.

aschempp · 2020-06-11T13:39:33Z

I'm not using yarn

But you should! 😄

haha, yeah, that's an opinion 😝

You need to run yarn and commit updates, but the next time please run yarn add --dev image-webpack-loader.

I do basically understand why this happens (I'm familiar with lock files). I'm wondering about the work flow, do I need to rebase on every commit to master to re-generate the lock file? That seems pretty cumbersome, is there a documentation what these tests are for?

Kocal · 2020-06-11T13:42:26Z

Yes the ideal is to rebase on master, yarn.lock conflicts won't be an issue since Yarn resolves them easily.

It seems that you already rebased your PR, so just run yarn and it should be fine.

stof · 2020-06-11T14:18:55Z

haha, yeah, that's an opinion stuck_out_tongue_closed_eyes

no. That's a fact. The webpack-encore project uses Yarn (Classic) for its dependency management. So when contributing to the project, you need to use it.

aschempp · 2020-06-11T14:36:34Z

no. That's a fact. The webpack-encore project uses Yarn (Classic) for its dependency management. So when contributing to the project, you need to use it.

Obviously. I was referring to the 😄 emoji looking more like a general hint 😉

yarn.lock updated in 14595cf

taylankasap · 2020-06-28T06:53:47Z

What is this waiting for? Anything I (a random developer) can help with?

infabo · 2020-07-04T13:00:57Z

@Rodrigo001-de awesome non-related spam comment!

Rodrigo001-dev · 2020-07-04T13:31:06Z

@infabo sorry

…pack-loader

aschempp · 2020-10-01T12:22:26Z

Updated to fix conflicts. Any news on merging this?

georgringer · 2020-11-02T09:10:04Z

hey @weaverryan it would be great if you could review this, let's merge that nice feature

joworeiter · 2021-01-11T09:07:51Z

hi, anything new, when the feature gets merged?

jb-thery · 2021-03-07T17:55:38Z

Hi, any update on this ?

Jupi007 · 2021-06-26T10:16:59Z

Hi, it is planed to fix conflicts and merge this soon?

aschempp · 2021-06-28T08:34:17Z

Unfortunately, this PR has become unusable due to a lot of changes/improvements in Encore. I guess I can re-implement the feature but I don't like to spend the time if it's not gonna be merged for another year… @weaverryan ?

Jupi007 · 2021-06-28T10:09:35Z

Unfortunately, this PR has become unusable due to a lot of changes/improvements in Encore. I guess I can re-implement the feature but I don't like to spend the time if it's not gonna be merged for another year… @weaverryan ?

That's sad, this feature could be awesome for many people.

Currently I manually optimize my images and just copy them into /public/build. Sorry for the offtopic @aschempp, but do you know how to manually integrate image-webpack-loader into encore?

aschempp · 2021-06-28T10:16:56Z

Sorry for the offtopic @aschempp, but do you know how to manually integrate image-webpack-loader into encore?

This is what I have in all my webpack.config.js (in addition to requiring image-webpack-loader in the package.json). Unfortunately it does not seem to work with copyFiles() but only assets that were required in scripts and stylesheets.

    // optimize and minify images
    .addLoader({
        test: /\.(gif|png|jpe?g|svg)$/i,
        use: [ 'image-webpack-loader' ]
    })

Jupi007 · 2021-06-28T10:18:17Z

Thanks a lot @aschempp :)

weaverryan · 2021-06-29T00:34:10Z

Hi!

yes, sorry to let this completely get stuck - that’s my fault. Per the last comment from @aschempp, since this is quite conflicted now and enabling it is so straightforward on your app, another option is just to document it.

I’m going to close for now. If someone wants to reopen with the updated version - or make a docs PR, that would be great. Again, my apologies @aschempp - you did really nice work here, sometimes it’s hard for me to keep up - sorry :/

aschempp · 2021-06-29T05:13:19Z

no hard feelings @weaverryan, one more thing off my plate myself 😆

Lyrkan reviewed Nov 23, 2019

View reviewed changes

lib/config-generator.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

aschempp marked this pull request as ready for review January 7, 2020 21:50

Lyrkan requested changes Jan 22, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

lib/WebpackConfig.js Outdated Show resolved Hide resolved

weaverryan reviewed Mar 20, 2020

View reviewed changes

aschempp added 9 commits June 10, 2020 14:51

Added support for image optimiziation with image-webpack-loader

6794edd

Rewrite imagemin loader to apply to images

b493cb1

Support callback for imagemin loader options

807399d

Do not disable imagemin in dev mode

90d24ae

Fixed some tests

b42f31b

Fixed some tests

f42e13e

Implement changes from #739

4f9f14c

Fix unit tests related to #739

098dbcb

Fixed some tests

c01c174

Update yarn.lock

14595cf

aschempp requested review from Lyrkan and weaverryan June 11, 2020 19:57

Lyrkan approved these changes Jun 16, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

soukicz mentioned this pull request Sep 1, 2020

image-webpack-loader does not optimize images #826

Open

Merge remote-tracking branch 'upstream/master' into feature/image-web…

8726dbd

…pack-loader

weaverryan changed the base branch from master to main November 18, 2020 18:24

weaverryan closed this Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for image optimiziation with image-webpack-loader #675

Added support for image optimiziation with image-webpack-loader #675

aschempp commented Nov 23, 2019 •

edited

Loading

Lyrkan left a comment

aschempp commented Jan 7, 2020

aschempp commented Jan 18, 2020

Lyrkan left a comment

aschempp commented Feb 28, 2020

weaverryan commented Mar 19, 2020

weaverryan left a comment

weaverryan Mar 20, 2020

aschempp Mar 20, 2020

weaverryan Mar 27, 2020

aschempp May 5, 2020

weaverryan commented Apr 17, 2020

aschempp commented Jun 11, 2020

Kocal commented Jun 11, 2020 •

edited

Loading

aschempp commented Jun 11, 2020

Kocal commented Jun 11, 2020

stof commented Jun 11, 2020

aschempp commented Jun 11, 2020

taylankasap commented Jun 28, 2020

This comment has been minimized.

infabo commented Jul 4, 2020 •

edited

Loading

Rodrigo001-dev commented Jul 4, 2020

aschempp commented Oct 1, 2020

georgringer commented Nov 2, 2020

joworeiter commented Jan 11, 2021

jb-thery commented Mar 7, 2021

Jupi007 commented Jun 26, 2021

aschempp commented Jun 28, 2021

Jupi007 commented Jun 28, 2021

aschempp commented Jun 28, 2021

Jupi007 commented Jun 28, 2021

weaverryan commented Jun 29, 2021

aschempp commented Jun 29, 2021

Added support for image optimiziation with image-webpack-loader #675

Added support for image optimiziation with image-webpack-loader #675

Conversation

aschempp commented Nov 23, 2019 • edited Loading

Lyrkan left a comment

Choose a reason for hiding this comment

aschempp commented Jan 7, 2020

aschempp commented Jan 18, 2020

Lyrkan left a comment

Choose a reason for hiding this comment

aschempp commented Feb 28, 2020

weaverryan commented Mar 19, 2020

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Mar 20, 2020

Choose a reason for hiding this comment

aschempp Mar 20, 2020

Choose a reason for hiding this comment

weaverryan Mar 27, 2020

Choose a reason for hiding this comment

aschempp May 5, 2020

Choose a reason for hiding this comment

weaverryan commented Apr 17, 2020

aschempp commented Jun 11, 2020

Kocal commented Jun 11, 2020 • edited Loading

aschempp commented Jun 11, 2020

Kocal commented Jun 11, 2020

stof commented Jun 11, 2020

aschempp commented Jun 11, 2020

taylankasap commented Jun 28, 2020

This comment has been minimized.

infabo commented Jul 4, 2020 • edited Loading

Rodrigo001-dev commented Jul 4, 2020

aschempp commented Oct 1, 2020

georgringer commented Nov 2, 2020

joworeiter commented Jan 11, 2021

jb-thery commented Mar 7, 2021

Jupi007 commented Jun 26, 2021

aschempp commented Jun 28, 2021

Jupi007 commented Jun 28, 2021

aschempp commented Jun 28, 2021

Jupi007 commented Jun 28, 2021

weaverryan commented Jun 29, 2021

aschempp commented Jun 29, 2021

aschempp commented Nov 23, 2019 •

edited

Loading

Kocal commented Jun 11, 2020 •

edited

Loading

infabo commented Jul 4, 2020 •

edited

Loading