Knockout.js parsing issue with if statements which surround th and td elements due to removed closure

[mattscotty]

I was getting the following error from Knockout.js.

Error: Cannot find closing comment tag to match: ko if :

After some trial and error I believe the issue lies with th and td closures being removed during minification. This is usually fine, but if using a knockout if statement, it cannot parse the HTML, so it cannot find the closing "/ko".

The following example;

<table>
        <thead>
            <tr>
                <!-- ko if : a -->
                <th data-bind="text:'a'"></th>
                <!-- /ko -->
            </tr>
        </thead>
        <tbody>
            <tr>
                <!-- ko if : a -->
                <td data-bind="text:'a'"></td>
                <!-- /ko -->
            </tr>
        </tbody>
    </table>

Gets minified as;

   <table>
      <thead>
         <tr>
            <!-- ko if : a -->
               <th data-bind="text:'a'"><!--< NOTICE THE MISSING CLOSURE-->
            <!-- /ko -->
      <tbody>
         <tr>
            <!-- ko if : a -->
                <td data-bind="text:'a'"><!--< NOTICE THE MISSING CLOSURE-->
            <!-- /ko -->
   </table>

Manually adding the closing and works (post minification).

I suggest a check be put in to not remove the closing tags if contained within a ko if.

Thanks

[trullock]

What minification settings are you using, i.e. how are you invoking NUglify to create this issue?

[mattscotty]

I am using the HTML minifier with the following settings (in C#);

string htmlCode = Uglify.Html(System.IO.File.ReadAllText(fileName)).Code;

string newFileName = fileName.Replace(".html", ".min.html");

File.WriteAllText(newFileName, htmlCode);

[trullock]

Its removing the closing </th> because it can, this is a good minification because browsers can handle this., Notice how the </tr>s are gone too.

The error is that KO doesn't like having KO logic around unclosed tags.

I'm not familiar with KO, is this a setting that can be controlled or must NUglify not aggressively remove closing tags when inside KO logic?

[mattscotty]

I think you're correct in that the browsers can handle it, it's knockout that doesn't like it. I think the fix is a specific check for knockout if statement around the tag before removing the closing tag. I checked and adding the closing tag back in (manually) fixes it.

[mattscotty]

This is more of an issue than I thought. For showing / hiding columns we can use CSS rather than ko if statement as a workaround.

However, this also affects KnockoutJS foreach statements where we are dynamically drawing th/td columns. For example;

<!-- ko foreach: years -->
    <th data-bind="text: year"></th>
<!-- /ko -->

Becomes

<!-- ko foreach: years -->
    <th data-bind="text: year">
<!-- /ko -->

Which leads to the error;

Uncaught Error: Cannot find closing comment tag to match: ko foreach: years

Other than using JavaScript to manually add these columns, I'm struggling to think of a workaround for this.

[trullock]

This makes sense, it will apply to any other kind of KO blocks

PRs welcome, the html parser needs to understand KO comments properly, rather than treating them as dumb comments

It then needs to know if youre inside a KO block when stripping the closing tags

immediate workaround is to disable closing tag stripping if youre using KO

[mattscotty]

Fair enough, I will try to make some time to investigate a PR, thank you.

In the meantime, which is the HtmlSettings setting I am looking for to disable this feature? The closest I see RemoveInvalidClosingTags which doesn't see to be the right option as they're not invalid.

Thanks in advance.

[trullock]

HTMLSettings.RemoveOptionalTags = false is what you want

[trullock]

Otherwise, I'm not really sure how much NUglify should know or care about KO, its one of many arbitrary "hacks" that sit atop HTML.

I won't reject a PR but it should probably be done in some extensible way so as to not really be KO specific

[mattscotty]

RemoveOptionalTags = false

Thanks that fixed the issue.

[mattscotty]

Otherwise, I'm not really sure how much NUglify should know or care about KO, its one of many arbitrary "hacks" that sit atop HTML.

I won't reject a PR but it should probably be done in some extensible way so as to not really be KO specific

I was (pleasantly) surprised that Knockout was considered in the first place. I think its still widely in use, but that number will probably be falling due to (seemingly) stopped development. I suppose if its in there then it should be (as close to) full support as possible so people aren't caught out.

I will try to come back to this when workload allows. Appreciate your hard work and help, thanks!