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 minify config #6990

Closed
wants to merge 9 commits into from
Closed

Conversation

satotake
Copy link
Contributor

@satotake satotake commented Feb 29, 2020

related #6750

This PR enables users to turn on/off by minifier types and to configure minify with its options
This is proposal and any comments and suggestions are welcome

content

  • [mod] Update github.com/tdewolff/minify
  • [minifiers] New config.go
  • [docshelper] Add func merge DocProviders
  • [docs] update docs.json

related issues

edit:
By downgrading minify version, #6621 became an upstream issue

[mod] Update github.com/tdewolff/minify
[minifiers] New config.go
[docshelper] Add func merge DocProviders
[docs] update docs.json
@bep
Copy link
Member

bep commented Feb 29, 2020

This looks really good. I have 3 comments:

  1. We cannot update tdewolff deps before we have verified the reason I had to roll back recently (it broke sites badly in the wild, including the Hugo site)

  2. I have learned some lessons from Chroma/Blackfriday/Pygments ... there may be a new super minifier on the scene in a year ... with totally different options. So I suggest we split out the general and put the specifics below tdewolff (same as in the markup config)

  3. I'm not sure about this one, but if I disable XML and do { { $xml : $xml | minify }} what happens? I assume I get myxml.min.xml but with unchanged content. I think we need to split the minifiers in two (one for the final content and the other for the minify func) ...?

@satotake
Copy link
Contributor Author

satotake commented Mar 1, 2020

Thanks for comment, @bep

1 & 2 are done

Please note that #6621 is not solved even by merging this because minify is not updated and does not have KeepQuotes option.

I admit that I am also not so confident about 3 but let me explain.

I disable XML and do { { $xml : $xml | minify }} what happens?

Minification of resource raises error and cause build fail when you unset the minfier.
On the other hand, minification of inline contents in .html file never raises error.

That is my intention but it is so vague as specification that I have just added a test case and improved the error message.

Second,

I think we need to split the minifiers in two (one for the final content and the other for the minify func) ...?

It is possible to separate content minifier from resource minifier in terms of both configurations and processing behaviour (I mean #6724 ).
However I believe that it will be better to use at least single configuration because

  1. It reduces configuration simplicity (Hugo's advantage)
  2. Most of users who configure minification demand lower level of minification and higher validity of contents. It will be better to apply more secure settings for them in sites' scope.
  3. Required settings of minification vary with users' situations but they are independent of whether minification targets are derived from content (layout) or resource
  4. these speculations may be wrong but they can be separated later.

minifiers/config.go Outdated Show resolved Hide resolved
@bep
Copy link
Member

bep commented Mar 10, 2020

Thanks for this. I think we need to settle 3 before we can wrap this.

{ { $xml := $xml | minify }} 

I think I would expect the above to just silently drop the minify statement if I disable minify, that statement could well live in a theme I don't control.

But you are probably right (?) about having 1 disable for both of these. We can consider adding some flag later if needed.

/cc @kaushalmodi @regisphilibert

@regisphilibert
Copy link
Member

regisphilibert commented Mar 10, 2020

I can see enabling/disabling is made by Type so it seems very controllable. I'm not familiar with mifnication technology.

I'm simply curious of what happens if a CSS file's minified .Content

{{ ($style | minified).Content }}

is printed in an HTML file. I suppose if CSS minification is disabled, but HTML is not, then the HTML minification will take care of minifying the CSS?

@bep
Copy link
Member

bep commented Mar 10, 2020

then the HTML minification will take care of minifying the CSS?

Not if you include it in style tags (which I assume you should/would).

@satotake
Copy link
Contributor Author

I think I would expect the above to just silently drop the minify statement if I disable minify, that statement could well live in a theme I don't control.

You are right @bep . I failed to be careful for theme. I will fix

@bep bep added this to the v0.68 milestone Mar 20, 2020
@bep
Copy link
Member

bep commented Mar 20, 2020

This looks really good now, thanks a lot. I will pull this and apply some adjustments.

@bep
Copy link
Member

bep commented Mar 20, 2020

I will complete this in #7077

@bep bep closed this Mar 20, 2020
@github-actions
Copy link

This pull request 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 Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to disable inline SVG minification
3 participants