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

Add option to disable inline SVG minification #6750

Closed
bf opened this issue Jan 13, 2020 · 4 comments · Fixed by #7077
Closed

Add option to disable inline SVG minification #6750

bf opened this issue Jan 13, 2020 · 4 comments · Fixed by #7077

Comments

@bf
Copy link

bf commented Jan 13, 2020

The minify tool used by hugo to minify the output works great on HTML, but there have been several issues with it destroying inlined SVGs:

These issues stem from the fact that minify changes the svg <path> attributes and attempts to optimize the drawing of lines on the svg canvas. This is a very hard problem, and as a result hugo users are affected by SVG optimization bugs from time to time. Furthermore, the testing for SVG minification in the minify repo seems to still be rudimentary: https://github.com/tdewolff/minify/blob/master/svg/pathdata_test.go

In minify project readme https://github.com/tdewolff/minify it says:
"Status: SVG: partially implemented; in development"

I think in light of this, we should add an option to disable inline SVG minification, or - even better - be able to pass options through to minify if another class of problems emerges.

@bf bf added the Proposal label Jan 13, 2020
@bep bep added Enhancement and removed Proposal labels Jan 13, 2020
@bep bep added this to the v0.63 milestone Jan 13, 2020
@bep
Copy link
Member

bep commented Jan 13, 2020

I suggest we add a new minify config struct. As we use the same minifier setup for both --minify and the minify template func, there is some thinking to be done how to disable one but not the other.

@tdewolff
Copy link

tdewolff commented Jan 14, 2020

The bug has been fixed. I appreciate the report.

This is a very hard problem

Actually it's a fairly easy problem. The only thing that makes it hard is making it remotely fast, which requires a lot of byte fiddling which is very bug prone. The HTML minifier has had a lot more testing in this regard so it's no surprise that it is more stable. The SVG test set is fairly complete to be honest so I'm not sure what you mean with rudimentary. Bugs creep in but that is normal in any piece of software.

The phrase "partially implemented; in development" means just that: thus far it should be production ready but there is still a lot of potential to minify further. Same goes for the JS minifier.

@bf
Copy link
Author

bf commented Jan 14, 2020

Dear @tdewolff, thanks for your quick bugfix & immediate response to this issue!

I think minify is a great project. But as you said there are still potential future issues (speed / byte fiddling) with some aspects of the tool.

Therefore it'd be great if we could have a choice which formats to minify when calling it from within the hugo toolchain.

@bep bep modified the milestones: v0.63, v0.64 Jan 22, 2020
@bep bep modified the milestones: v0.64, v0.65 Jan 30, 2020
@bep bep modified the milestones: v0.65, v0.66 Feb 18, 2020
@bep bep modified the milestones: v0.66, v0.67 Mar 2, 2020
@bep bep modified the milestones: v0.67, v0.68 Mar 9, 2020
@bep bep modified the milestones: v0.68, v0.69 Mar 20, 2020
bep pushed a commit to bep/hugo that referenced this issue Mar 20, 2020
@bep bep mentioned this issue Mar 20, 2020
bep pushed a commit to bep/hugo that referenced this issue Mar 20, 2020
@bep bep closed this as completed in #7077 Mar 20, 2020
bep pushed a commit that referenced this issue Mar 20, 2020
@github-actions
Copy link

github-actions bot commented Feb 5, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants