a bug with removeComments&&ignoreCustomFragments

[kath4job]

when deciding whether to ignore this comment, the "text" has been replaced by "reCustomIgnore".

so this logic will never retrn true, this comment will be removed and cause some problems. my point of view, this function should be according to the following:

[alexlamsl]

I'm confused - why should ignoreCustomComments be using a variable from ignoreCustomFragments?

[kath4job]

@alexlamsl ? this include ignoreCustomComments comment can't be removed, if it removed, it will cause the ignoreCustomComments replace back logic error.

[alexlamsl]

Bear with my confused soul... 😰

Would you mind giving an example of html code (and options) that would produce an unexpected result?

[kath4job]

before htmlminfier:

after :

options:

[alexlamsl]

Ah, I see what you mean now - when ignoreCustomFragments matches something in the comments, and removeComments is used, then we hit a replacement issue.

Just to be clear, is the following output what you'd expect instead?

{{if a==1}}<div>i'm a div</div>{{/if}}

[kath4job]

@alexlamsl yes

[alexlamsl]

ok - will have a look at it 👍

@kangax this bug / corner case exists since ignoreCustomFragments is introduced - what are your thoughts on this? Do you agree with the expected results as stated above?

[kangax]

@kath4job for the future, it would be best to phrase an issue like this by simply stating that <!-- {{ test }} -->{{if ... }}<p>...</p>{{/if}} unexpectedly removes <!-- {{ test }} --> when "ignoreCustomFragments" is on. Took me a while to figure out what the problem is...

It's an interesting use case and I think the right outcome here is for minifier to strip a comment:

{{if ... }}<p>...</p>{{/if}}

Even though "ignoreCustomFragments" is on, we can't be aware of an outer context. If we try to parse input as HTML first — to be aware of the context — then we run into the issue of that markup not being valid HTML due to the presence of all that stuff that has to be ignored, and the entire parsing process goes to hell.

It's a catch 22.

For this reason, we can only ignore custom fragments as the very first step of minification, before trying to parse/minify anything. After that, removeComments logic kicks in. Two of these processes are independent and are unaware of each other, hence the removed fragment. In other words, removeComments takes precedence because it runs after ignoreCustomFragments.

We should definitely mention this in the docs but I think this is the correct behavior.

@kath4job @alexlamsl what do you think? Does this make sense? Is there anything else we can do without shooting ourselves in the foot?

[alexlamsl]

@kangax in terms of expected behaviour we are on the same page.

You might already know, but just in case I'd like to clarify that the bug is not just comment being removed. It is this:

<!--{{comment}}-->{{if}}blah{{/if}}

getting turned into this:

{{comment}}blah{{if}}

due to the assumption of sequential replacement.

I have an idea to fix this (the suggested fix above doesn't give our expected outcome...) - will post as soon as I'm back to a real keyboard.

[kangax]

Yep, I figured that. Thanks for taking care of it!

Sent from my iPad

On Feb 28, 2016, at 11:45 PM, Alex Lam S.L. notifications@github.com wrote:

@kangax in terms of expected behaviour we are on the same page.

You might already know, but just in case I'd like to clarify that the bug is not just comment being removed. It is this:

{{if}}blah{{/if}}

getting turned into this:

{{comment}}blah{{if}}
due to the assumption of sequential replacement.

I have an idea to fix this (the suggested fix above doesn't give our expected outcome...) - will post as soon as I'm back to a real keyboard.

—
Reply to this email directly or view it on GitHub.