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.

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

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

Open
wants to merge 4 commits into
base: master
from

Conversation

@aschempp
Copy link

aschempp commented Nov 23, 2019

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

Copy link
Collaborator

Lyrkan left a comment

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 Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@aschempp aschempp marked this pull request as ready for review Jan 7, 2020
@aschempp
Copy link
Author

aschempp commented Jan 7, 2020

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
Copy link
Author

aschempp commented Jan 18, 2020

failing tests don't see related to the PR 🙃

Copy link
Collaborator

Lyrkan left a comment

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 Outdated Show resolved Hide resolved
lib/WebpackConfig.js Outdated Show resolved Hide resolved
@aschempp
Copy link
Author

aschempp commented Feb 28, 2020

any feedback on this PR? 🙃

@weaverryan
Copy link
Member

weaverryan commented Mar 19, 2020

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

Copy link
Member

weaverryan left a comment

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 :).

loaderFeatures.ensurePackagesExistAndAreCorrectVersion('imagemin');

const imageminOptions = {
disable: !webpackConfig.isProduction(),

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 20, 2020

Member

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.

This comment has been minimized.

Copy link
@aschempp

aschempp Mar 20, 2020

Author

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 27, 2020

Member

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());

This comment has been minimized.

Copy link
@aschempp

aschempp May 5, 2020

Author

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
Copy link
Member

weaverryan commented Apr 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.