WebReflection / domtagger

The hyperHTML's template literal parser
ISC License
43 stars 3 forks source link

Bugfix: Getting non direct attribute name #19

Closed tiagomapmarques closed 5 years ago

tiagomapmarques commented 5 years ago

This PR fixes the bug on lighterhtml of matching the attribute name correctly when interpolated values are introduced.

Example of code that does not work before the fix:

html`<div class="block__element${hasModifier ? '--modifier' : ''}"></div>`;

Implementation of attribute re-ordering is based on the fact that all "attributes" exist in the parent node, so "indexOf" will not return indices from other nodes (children for example).

Discussion started here: https://github.com/WebReflection/lighterhtml/pull/66

WebReflection commented 5 years ago

There's something I don't fully understand about this fix ...

As example, I have no bug here:

// options from the test/index.html page
const div = document.body.appendChild(document.createElement('div'));
    setInterval(() => {
      div.appendChild(
        domtagger(options)`<div class="block__element${Math.random() < .5 ? '--modifier' : ''}">asd</div>`
      );
    }, 1000);

so while I can counter-validate that breaks on lighterhtml, I am not sure that breaks because of domtagger.

As summary: what are we trying to solve? Can you please create an example where domtagger actually breaks with sparse attributes values?

WebReflection commented 5 years ago

OK, I've found the issue 🎉

All I had to do was ignoring the rest of the parts content if sparse, so that this is the only change needed: https://github.com/WebReflection/domtagger/commit/cfa30797281a7e3bc1329970a3f9d21168b76b1c#diff-f478ad188374b35b6bd1dd3c7ccc70a0

Thanks for filing the bug and taking time to help out, I am updating lighterhtml so that everything should be fine in minutes.

WebReflection commented 5 years ago

OK then, lighterhtml, hyperHTML, and heresy have been updated. Your example works just fine now 👋