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

Remove pngquant/optipng #5553

Closed
jdubois opened this Issue Apr 6, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@jdubois
Member

jdubois commented Apr 6, 2017

I haven't looked very closely at pngquant/optipng but we keep having issues with those:

  • Those are native libraries that do image minification
  • They need to be native (you can't do that kind of thing in JavaScript)
  • They rely on libraries that should be installed on the machine, which make them fail quite often on Linux (and also on Windows, see #5547 )

Apart from those errors, as those are native librairies, they should be downloaded at each project generation (they can't be stored in the Yarn cache), and that's a real pain when generating an application.

I find those are totally useless. Images should be done by graphic designers, and they don't work with PNG or JPEG. So when images are put into the application, they should already be minified, and shouldn't be touched by our minification process.

So I'm opening this ticket to remove those.

@deepu105

This comment has been minimized.

Show comment
Hide comment
@deepu105

deepu105 Apr 6, 2017

Member
Member

deepu105 commented Apr 6, 2017

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Apr 6, 2017

Member

OK I'm doing it - I'm so fed up with those image issues!

Member

jdubois commented Apr 6, 2017

OK I'm doing it - I'm so fed up with those image issues!

@jdubois jdubois closed this in 88660c8 Apr 6, 2017

@stevehouel

This comment has been minimized.

Show comment
Hide comment
@stevehouel

stevehouel Apr 6, 2017

Contributor

+1 @jdubois :)

Contributor

stevehouel commented Apr 6, 2017

+1 @jdubois :)

@stevehouel

This comment has been minimized.

Show comment
Hide comment
@stevehouel

stevehouel Apr 6, 2017

Contributor

@jdubois you were faster that me lol

Contributor

stevehouel commented Apr 6, 2017

@jdubois you were faster that me lol

@ruddell

This comment has been minimized.

Show comment
Hide comment
@ruddell

ruddell Apr 6, 2017

Member

We also should revert da24c3a (it references pngquant and I don't think the phantomjs part is needed)

Member

ruddell commented Apr 6, 2017

We also should revert da24c3a (it references pngquant and I don't think the phantomjs part is needed)

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Apr 6, 2017

Member

Oh sorry I missed this one, thanks @ruddell

Member

jdubois commented Apr 6, 2017

Oh sorry I missed this one, thanks @ruddell

@jdubois jdubois modified the milestone: 4.3.0 Apr 13, 2017

@sdoxsee

This comment has been minimized.

Show comment
Hide comment
@sdoxsee

sdoxsee Apr 13, 2017

Contributor

@ruddell @jdubois the phantomjs part seemed to be needed for me but I can't tell you exactly why yet. I added it back and everything worked. ref: karma-runner/karma-phantomjs-launcher#120 (comment)

Contributor

sdoxsee commented Apr 13, 2017

@ruddell @jdubois the phantomjs part seemed to be needed for me but I can't tell you exactly why yet. I added it back and everything worked. ref: karma-runner/karma-phantomjs-launcher#120 (comment)

@ruddell

This comment has been minimized.

Show comment
Hide comment
@ruddell

ruddell Apr 16, 2017

Member

@sdoxsee I originally added those lines without much reason besides that several people had to run into that same issue, I can't reproduce it anymore though. Also our Travis tests seem to be passing without a problem, so maybe it's related to node version or operating system? If you can reproduce it or have more info on why it happens, please open a new issue with the details

Member

ruddell commented Apr 16, 2017

@sdoxsee I originally added those lines without much reason besides that several people had to run into that same issue, I can't reproduce it anymore though. Also our Travis tests seem to be passing without a problem, so maybe it's related to node version or operating system? If you can reproduce it or have more info on why it happens, please open a new issue with the details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment