Pass minification errors to the user

[ptgott]

Previously, *minifyTransformation.Transform suppressed the
error returned by t.m.Minify. This meant that when minification
returned an error, the error would not reach the user. Instead,
minification would silently fail. For example, if a JavaScript
file included a call to the Date constructor with:

new Date(2020, 04, 02)

The package that the minification library uses to parse JS files,
github.com/tdewolff/parse would return an error, since "04" would
be parsed as a legacy octal. However, the JS file would remain
un-minified with no error.

Fixing this is not as simple as replacing "_" with an "err" in
*minifyTransformation.Transform, however (though this is
necessary). If we only returned this error from Transform,
then hugolib.TestResourceMinifyDisabled would fail. Instead of
being a no-op, as TestResourceMinifyDisabled expects, using the
"minify" template function with a "disableXML=true" config
setting instead returns the error, "minifier does not exist for
mimetype."

The "minifier does not exist" error is returned because of the
way minifiers.New works. If the user's config disables
minification for a particular MIME type, minifiers.New does
not add it to the resulting Client's *minify.M. However, this
also means that when the "minify" template function is executed,
a *resourceAdapter's transformations still add a minification.
When it comes time to call the minify.Minifier for a specific
MIME type via *M.MinifyMimetype, the github.com/tdewolff/minify
library throws the "does not exist" error for the missing MIME
type.

The solution was to change minifiers.New so, instead of skipping
a minifier for each disabled MIME type, it adds a NoOpMinifier,
which simply copies the source to the destination without
minification. This means that when the "minify" template
function is used for a particular resource, and that resource's
MIME type has minification disabled, minification is genuinely
skipped, and does not result in an error.

In order to add this, I've fixed a possibly unwanted interaction
between minifiers.TestConfigureMinify and
hugolib.TestResourceMinifyDisabled. The latter disables
minification and expects minification to be a no-op. The former
disables minification and expects it to result in an error. The
only reason hugolib.TestResourceMinifyDisabled passes in the
original code is that the "does not exist" error is suppressed.
However, we shouldn't suppress minification errors, since they
can leave users perplexed. I've changed the test assertion in
minifiers.TestConfigureMinify to expect no errors and a no-op
if minification is disabled for a particular MIME type.

Resolves #8954

[jmooring]

@bep Can you review this in the next week or two? It passes mage -v check locally, and throws an error as expected with this simple test:

{{ (resources.FromString "temp.js" "const a = 04" | minify).Content }}

My only comment is that the error is a bit verbose:

ERROR 2021/09/12 21:32:59 MINIFY: failed to transform "temp.js" (application/javascript): legacy octal numbers are not supported in expression on line 1 and column 11
    1: const a = 04
                 ^
Error: Error building site: failed to render pages: render of "page" failed: "/home/jmooring/code/hugo-testing/layouts/_default/single.html:2:59": execute of template failed: template: _default/single.html:2:59: executing "main" at <(resources.FromString "temp.js" "const a = 04" | minify).Content>: error calling Content: MINIFY: failed to transform "temp.js" (application/javascript): legacy octal numbers are not supported in expression on line 1 and column 11
    1: const a = 04
                 ^

[ptgott]

@jmooring Thanks for the feedback! I've added an assertion to the integration test to ensure that the minification error is only printed once, via the build logger. The test is now failing, so I'll dig in later today/tomorrow and see how to get it to pass.

[ptgott]

@bep @jmooring It turns out that the verbose error message is not unique to minification errors and applies to other resource transformation errors as well (e.g., fingerprint). I've created an issue to track this (#8974) separately so all work in this PR is relevant to #8954.

[bep]

@ptgott lets leave the "double error" for another time.

[bep]

I know this isn't a hot path, but my head twisted a little when I see a map like this with values that just get thrown away after selecting one.

I would prefer a simpler switch statement e.g.

case "css" && !c.DisableCSS:
   return &c.Tdewolff.CSS
...
default
   return noop

[bep]

No reason to export this, I suggest renaming to noopMinifier

[ptgott]

@bep Thanks for your review. I have made the requested changes.

[github-actions]

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.