Hyperhtml: Partial attribute results in off-by-one application of state

[fvsch]

Hi. I’m playing with hyperHTML and reading the docs too quickly for my own good. :D So I ended up trying to write a partial attribute.

This might be a wontfix since the docs spells out that partial attributes are forbidden (in Getting Started); but in addition to not working, a partial attribute messes with state application to other parts of the template. Simple test case:

<main></main>
<script src="https://unpkg.com/[email protected]/min.js"></script>
<script>
  const update = (render, state) => {
    render`
    <div class="Teaser ${state.test ? 'Teaser--test' : ''}">
      <h2 class="Teaser-title">
        <a href="${state.url}">${state.title}</a>
      </h2>
      <div class="Teaser-text">${state.text}</div>
    </div>
    `;
  }
  update(hyperHTML.bind(document.querySelector('main')), {
    title: 'This is the title',
    text: '<p>And the main description</p>',
    url: '/this/is/the/url',
    test: Math.random() > 0.5
  })
</script>

Renders as:

<div class="Teaser <!--_hyper_html: -718251426;-->">
  <h2 class="Teaser-title">
    <a href="">/this/is/the/url</a>
  </h2>
  <div class="Teaser-text">This is the title</div>
</div>

This is the related error (in Firefox’s console):

TypeError: updates[(i - 1)] is not a function
    update https://unpkg.com/[email protected]/hyperhtml.js:599:7
    upgrade https://unpkg.com/[email protected]/hyperhtml.js:623:31
    hyperHTML https://unpkg.com/[email protected]/hyperhtml.js:18:15
    hyperHTML self-hosted:951:17
    update http://test-hyperhtml.dev:8080/:5:5
    <anonymous> http://test-hyperhtml.dev:8080/:14:3

If possible, it would be a better result to have the partial attribute fail but not the next content.

[pinguxx]

i think you can do

<div class="${'Teaser' + (state.test ? 'Teaser--test' : '')}">

see getting started

[fvsch]

Yup. I linked to that section in my report.

This issue is not about the correct way to do it, but about limiting the side effects of this error — if possible.

[WebReflection]

Impossible by HTML specifications and since not needed anyway, a won't fix regardless? 🤷‍♂️

I've got way more interesting things on this plate now.

[WebReflection]

To clarify the previous comment:

• there are 3 rules in total with hyperHTML and all of them for a very valid reason. It's not my intention to consider fixing developers that didn't read 3 rules ;-)
• the attribute node is one per name. There is no way to change partially an attribute without triggering its whole new value. Accordingly, not only it's a won't fix, it's wrong expectation to think you can partially change any attribute

hyperHTML is not mustaches, it doesn't work with strings, it works with DOM nodes.

Learn those 3 rules and you can do everything.

Ignore those 3 rules and you can have a powerful footgun.

As example, if you want text and you don't explicitly do that by using at least one char different from > and < you'll have HTML injections if the interpolation is not a node.

Again: 3 rules, I know the documentation is not in its best shape now but ..3 rules are not too many to remember, right?

I hope I've better explained my previous comment.

[marcoscaceres]

Agree. It's good for hyperHTML to be "opinionated" about this. You end up with much cleaner code.

[WebReflection]

thanks @marcoscaceres , I've also prepared a talk that explains in details reasons behind these rules.

Just for reference sake, I'll paste in here one slide ... rules are actually 2 and a half, since text and html are the same rule to remember.

[joshgillies]

@WebReflection are those slides posted in full anywhere?

[fvsch]

Thanks for the detailed explanation.

I understand the rationale, but just to be clear this bug was never about making partial attributes work, it was about limiting the side effects of the bug to the attribute itself. So that if you do shoot your own foot, you end up with a bullet in your foot but not in other body parts as well. Even with clear documentation and cardinal rules, people will tend to learn by trial-and-error anyway, so it's useful if the part that breaks is close to the part where the mistake is.

[fvsch]

Just for the sake of completedness, and I'll shut up after that. :D

Looking at the source for the attributeSeeker function, it seems any attribute values which has extra characters, including whitespace, will trigger this issue. Confirmed with e.g.

render`
<p class=" ${state.attr} ">
  ${state.text}
</p>

(The paragraph will show the "attr" value instead of the "text" one.)

The following changes to the attributesSeeker function would 1) avoid triggering this issue due to whitespace and 2) limit the brokenness of partial attributes to the attribute itself. It's a very crude test which may be missing some big things, but it seemed to work with the test suite and my own tests.

-      if (attribute.value === value) {
+      if (attribute.value.trim() === value) {
         // with IE the order doesn't really matter
         // as long as the right attribute is addressed
         if (IE) attribute = node.getAttributeNode(IEAttributes.shift());
         actions.push(remapping ?
           {a:'attr', n:attribute} :
           setAttribute(node, attribute));
       }
+      else if (attribute.value.indexOf(value) !== -1) {
+        actions.push(function(){});
+      }

[WebReflection]

no

the moment hyperHTML is forgiving about those 2.5 rules is the moment it's a mess for everyone.

It's fundamentally essential for developers to understand the value of a space in an hyperHTML template because it's through empty spaces that #text and <html> have different meaning.

I understand attributes are attributes and cannot be changed between attributes with useless surrounding empty spaces and those without, but I don't want to encourage developers to write useless spaces because here, for #text VS <html> are very important (I know, I've just said that ... but I'd like to repat that forever, if necessary).

This is not a mustache template, and in template literals every single chars around interpolation matters.

I have really better things to do than slow down the tree parser and confuse developers allowing useless spaces in some place but not in others ... do you understand?

As Summary:

no spaces ... it's an attribute with an interpolation. no spaces 'cause you don't need 'em.

[WebReflection]

@joshgillies all my talks are online but this one I haven't even given it plus slides, without me talking on top, are always a bit useless :smile:

Will pass it through after the 22nd when I'm done with the talk (and these slides)