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

chore(dev workflow): add pre-commit hook to compress staged images #13762

Closed
wants to merge 1 commit into from

Conversation

moonmeister
Copy link
Contributor

Description

Adds pre-commit hook to the repo using existing husky config to compress jpg,png,gif images that have been added to the repository.

This package uses plugins for compression that are lossless. This is good for quality but means images can remain quite large.

https://web.dev/use-imagemin-to-compress-images - for list of lossless vs lossy plugins.

Related Issues

#13699

@moonmeister moonmeister added the type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change label May 1, 2019
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moonmeister I like the idea of this--but I'm also a little concerned about this perhaps introducing failures in the commit process. We've seen a fair amount of failures with e.g. sharp, so we don't want to put any other barriers in place or introduce potential failure points.

I think I'd rather we treat this as something we automate (e.g. perhaps we use a Github action to optimize the images in a PR with a commit?), or something that's done on an ad hoc/reactionary basis, just to reduce the potential for failure.

What do you think?

@DSchau DSchau added the status: awaiting author response Additional information has been requested from the author label May 2, 2019
@moonmeister
Copy link
Contributor Author

@DSchau Hadn't thought of doing it that way...I think it introduces a couple side effects that might be more troublesome. The primary goal of this is to is to reduce repo size. If we allow people to commit then ask a bot to checkout, compress, and re-commit images then we're duplicating the copies of images in history and significantly increasing repo size. This could be over come if the bot were to rewrite history with the new version of the image. But this seems more fragile and potentially dangerous then doing it with a commit hook. This said I am inexperienced with this kind of git operation, maybe it'd be fine.

Also, we're already using SVGO to optimize SVGs in commit hooks, this is effectively the same thing but for JPG/PNG/GIF. In fact, the tool I'm using includes SVGO for SVGs but I haven't included them in the husky file pattern matchers as our config is different.

All this said, you make a good point, this needs to be robust and not prohibit contributions in anyway. Maybe we figure out some ways to test this and see if it causes any troubles. Thoughts here? The one thing I noticed on REALLY large images(2+MB) it can take a while with little to indicate what's happening.

@wardpeet
Copy link
Contributor

wardpeet commented May 3, 2019

I agree with @DSchau, also imagemin needs some dependencies like libpng, libjpg, .... How do we feel about adding a lint rule on ci so we get a notice of this? Or maybe we shouldn't bother and just minify everything once in a while.

@sidharthachatterjee
Copy link
Contributor

Agree with @wardpeet and @DSchau here that it would be best to avoid problematic hooks in the commit flow

Closing this for now but definitely think this could be a scheduled GitHub Action perhaps

Thank you for the work though, Alex! 🥇

@LekoArts LekoArts deleted the imagemin branch May 17, 2019 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants