Minifying breaks when js placeholder inside HTML comment

[lukapopijac]

Fails with this message:

(!) Plugin minify-html-literals: splitHTMLByPlaceholder() must return same number of strings as template parts

in.js:

let a = 5;
html`
    <div>
        <!-- ${a} -->
    </div>
`;

rollup.config.js:

import minifyHTML from 'rollup-plugin-minify-html-literals';
export default {
    input: 'in.js',
    output: {
        file: 'out.js',
        format: 'es'
    },
    plugins: [
        minifyHTML()
    ]
};

[asyncLiz]

html-minifier will intentionally remove comments when minifying the HTML template. Have you tried configuring the minify options to preserve comments if you need to use them in your templates?

[lukapopijac]

I haven't tried any options, it is irrelevant to me. Both removing or not removing comments is fine to me. The problem is that the minifier fails.

[asyncLiz]

Given this template:

let a = 5;
html`
    <div>
        <!-- ${a} -->
    </div>
`;

If the comment is removed while minifying, the template literal replacement is also removed:

// Problem! a is no longer used in the minified template
let a = 5;
html`<div></div>`;

This is why the minifier throws an error. It realizes that the number of template literal replacements it started with (1) is not the same as the number after it minified the template (0).

The minifier isn't smart enough to know why. It doesn't understand that this particular replacement value was within an HTML comment and html-minifier removed it because the user told it to. It just knows that "this isn't supposed to happen", so it throws an error to alert the user.

To correct the error, you can proceed with one of two routes:

1. Configure the minifier to not remove the HTML comment so that the template literal replacement ${a} is not removed.
2. Do not use a template literal replacement in an HTML comment if you wish comments to be removed.

[lukapopijac]

Not a problem for me, I will find my way around it. I created this issue because It doesn't minify anything, regardless of removing or not removing comments. The output file is the same as the input file. I would consider this to be a bug.

[asyncLiz]

Sound good!

I'm going to close this issue then since this is working as intended. The minifier is correctly reporting the accidental removal of a template literal replacement value.

Here is a rollup plugin config example for how you can preserve comments in your minified templates if you wish to use use replacements within HTML comments:

import minifyHTML from 'rollup-plugin-minify-html-literals';
import {defaultMinifyOptions} from 'minify-html-literals';

export default {
  entry: 'index.js',
  dest: 'dist/index.js',
  plugins: [
    minifyHTML({
      options: {
        minifyOptions: {
          ...defaultMinifyOptions,
          removeComments: false
        }
      },
    })
  ]
};

[lukapopijac]

I wouldn't consider this to be working as intended. The warning message might be correctly reported, that is not a problem. The problem here is that if I want to remove comments, my template is not minified.

[asyncLiz]

The goal of this plugin, like all minifiers, is to minify template code without changing the semantics of the JavaScript AST syntax.

The use case you have provided is contradictory to this requirement: you desire to remove comments in the template, but doing so changes JavaScript code because template expressions are being used in those comments.

Check out https://astexplorer.net/ and paste both versions of the template in https://github.com/asyncLiz/rollup-plugin-minify-html-literals/issues/12#issuecomment-687560050. Both are TaggedTemplateExpressions, but the first has an expression and the second does not.

Minifiers by design should not change code semantics.

I hope this clarifies that your use case is not simply removing comments (normally a side-effect free change), but has a negative side-effect of changing the JavaScript semantics of your application as well.

If you still do not understand, I'd encourage you to describe the use case that you are trying to achieve.

• What real-world JavaScript values are you inserting into HTML comments in your template?
• Why do you need to use HTML comments to display them?
• Are the HTML comments used elsewhere in code?
• Are you using those JavaScript values in other places in the template?
• Why do you want the HTML comments with those JavaScript values to be displayed while developing and removed when minified for production?

[lukapopijac]

I understand why this is happening, and I am also aware that the warning message is logical and I don't think it needs to be changed. The part which I have trouble with is that the tool doesn't minify at all in case when I want to remove comments.

My use case is very simple: At some point I didn't need that one line of code anymore, so I commented it out. I did that instead of deleting only because I might change my mind in the future and uncomment it back. Simple as that.

I suggest the minifier does one or both of these:

1) There can be an option (a dangerous option) that user can explicitly specify they want to remove html comments even when they affect the AST tree. Warning message appears normally, but it is up to the user to deal with the situation as they explicitly defined this in options.

and/or

2) Minifier detects that removing a comment would affect AST tree, so it does not remove this comment, proceeds with the rest of minification, and gives the user appropriate message. This way the result would be minified (although maybe not this specific comment), the AST tree preserved, and user notified about the situation. win-win-win.

Or even better, an upgrade to the second option:

3) Minifier does everything defined in point 2, and additionaly those troublesome comments replaces with stripped ones preserving the js placeholders.

E.g.

html`
<div>
    <!-- <div class="${a}" data-something=${b}>${c=d+e}</div> -->
</div>
`;

goes into:

html`<div><!--${a}${b}${c=d+e}--></div>`;

After all, from a minifier tool I expect it to result in minified code if the input is valid. If something is hard for the tool to do, I still expect to give me the best result as currently possible. Definitely, the tool should not break and ignore all the other lines of code only because it found one line (a valid line of code) that is somehow troublesome.

[asyncLiz]

I understand, thank you for explaining your use case to me. There are two things influencing my decision:

1. I'd strongly oppose any options that change the AST, even if they are documented as "unsafe", since that goes against the spirit of what minifiers are supposed to be doing.

2. I want to ensure that the minifier performs well and does not need to parse strings. Right now my template minifier does not parse the text between template expressions. It doesn't know where an HTML comment begins or ends. It would need to parse the entire template as an HTML AST, like its dependency html-minifier does. That's some heavy lifting I don't want the top-level template minifier to perform and maintain.

With those two things in mind, I took a look at html-minifier's options and found ignoreCustomComments. This option would allow my minifier to instruct html-minifier to ignore any HTML comment that had a template expression placeholder in it.

This would effectively prevent the removal of HTML comments where template expressions are used without any additional logic required.

That seems worthwhile, and I think the use case you described makes sense in the context of using IDEs like VS Code, where commenting out a node inside an HTML template using shortcuts can generate this error.

I'll reopen this issue to see if ignoreCustomComments can be a solution.

[lukapopijac]

Fair enough. Your ignoreCustomComments suggestion sounds quite satisfactory for my use case.