WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 112 forks source link

Prevent stringification of DOM element #77

Closed oslego closed 7 years ago

oslego commented 7 years ago

I know this is mentioned in the readme:

if there's any char different from > and < surrounding the interpolation, that content will be text, instead of HTML

But I still got bitten hard by it when interpolating a hyperHTML.wire(). If there is a space or newline between the last > and the interpolation, regardless of what's inside, it .toString()s the content of the interpolation.

Example:

function OptionElement(item) {
  return hyperHTML.wire(item)`<option value="${item.value}">${item.name}</option>`;
}

/* 
This fails to do what's expected and injects "[object OptionElement]" string
instead of DOM element because of the newline between `<select>` and `${`
*/
function render(){
  return this.html`
    <select>
     ${this.state.items.map(item => OptionElement(item))}
    </select>
  `;
}

/* 
This works as expected and injects DOM elements. 
Notice the missing newline and/or space  between `<select>` and `${`
*/
function render(){
  return this.html`
    <select>${this.state.items.map(item => OptionElement(item))}</select>
  `;
}

This fails very silently particularly when generating <option> elements in a <select> and the browser UI doesn't show anything because it's getting pure strings, not HTML. To debug, one has to inspect the element and get a hint from seeing [object HTMLOptionElement] that the object is converted to string.

I spent most of the day debugging, until I discovered that structuring my template with newlines for readability is actually the problem. This will cause headaches for a lot of people until they go through the same right of passage, debug, and then restructure their template to lack spaces or newlines.

The requests are:

In the meantime, you're welcome to take this example and add it to a prominent troubleshooting section of the docs. I could help with that and a few other examples where I got cryptic error messages because I didn't play to hyperHTML's exact tune.

WebReflection commented 7 years ago

Thanks for the questions and the report. I'll go through "all the things" reasoning about them :smiley:

To start with, this works

function OptionElement(item) {
  return hyperHTML.wire(item)`
  <option value="${item.value}">
    ${item.name}
  </option>`;
}

If you want the compact way:

function OptionElement(item) {
  return hyperHTML.wire(item)`<option value="${item.value}">\t${item.name}</option>`;
}

You can add any space in the HTML because spaces are meaningless (at least at the beginning of a node, it's part of specifications)

a <a href="#b">
    b</a> c

Looks like this on a browser:

screenshot from 2017-08-03 13-34-12

TL;DR put just one white space at the beginning of an interpolation that should be treated as text and the problem is solved. If you want to be explicit, \t, where t is for text looks like the only thing to learn/remember about these templates (beside non partial attributes).

Moreover

// this is just fine, same readability
function render(){
  return this.html`
    <select>${
      this.state.items.map(item => OptionElement(item))
    }</select>
  `;
}

You can use new lines within interpolations as much as you want. It's just javascript, these won't be reflected inside the template.

If this is new to developers then again I'm happy they can learn standards again with hyperHTML :tada:

This fails very silently

Like a +'a' woudl produce NaN, you need to understand there are 2 rules and a half to remember.

These 2.5 rules are way less than any mustache, hogan, Markdown, you name it, template engine, but of course every newcomer will probably hit this bit: it's inevitable, which is why my documentation will have a test page to make you understand what is a white space and how powerful it is.

For edge cases, however, I've recently exposed hyperHTML.escape which can be trapped as const t = hyperHTML.escape one, and used as <div>${t(content)}</div> whenever you want.

Opting out is not an option, because non textual content can be a Promise, an Array, a DOM node, so it'd make no much sense.

I discovered that structuring my template with newlines for readability is actually the problem

I hope you also discovered with this answer you were thinking you couldn't use new lines within interpolations.

Answering questions

is it possible to do a lookahead into the interpolation and assume that if the content is a DOM element, then it should be inserted as is and not stringified?

the content of what?

<div>${html|node|Promise|array}</div> works as expected.

So does a<br>${html|node|Promise|array} with no element whatsoever.

Put a space around those cases and you've explicitly opted out from magic, declaring #text.

This will save a lot of headaches for a lot of people.

I think this is speculation. I have 719 stars at this point and 5 complains like this that I remember.

There are rules to understand, and there are reasons behind.

<div>Hello ${user.name}</div> is a very common use case, for instance. with the current architecture you can do literally whatever you want. Any extra magic on top would make hyperHTML templates ambiguous, which is the least thing I want to implement.

Accordingly, unless I have examples and a concrete proposal that consider all use cases, this is a hell of a nail on the floor but once everyone will step on it, they'll learn how to avoid it.

Same goes for non partial attributes: it makes no sense to have partial attributes 'cause the browser consider attributes as a whole, not as partial. Yet few asked already saying:

I cannot do this <p class="title ${variable}">

The answer was: <p class="${'title ' + variable}">. That user is still happy with that :smile:

is it possible to log in the console warnings when DOM elements are stringified, potentially by accident accident, and point to a reference in the docs that explains why it happened?

I have no idea how I can classify "by accident" from intentional. If I had such idea, I would've make everything magically unambiguous already.

If you have any idea how to make that happen without bloating the library with all HTML specifications that talks about which element can contain what as either text or html, I'd be happy to consider this option.

you're welcome to take this example and add it to a prominent troubleshooting section of the docs.

Thank you, I will once I have the doc page showing up :+1:

oslego commented 7 years ago

Andrea, you are very agitated. You need to slow down for a bit. Nobody is attacking you or your work. I'm going to try and keep this short:

First, you were in such a rush to reply that you missed the original issue I mentioned: that a newline when using the interpolation yields a string instead of a DOM element as one would originally expect by looking at the code.

Here's the example again:

/* 
This fails to do what's expected and injects "[object OptionElement]" string
instead of DOM element because of the newline between `<select>` and `${`
*/
function render(){
  return this.html`
    <select>
     ${this.state.items.map(item => OptionElement(item))}
    </select>
  `;
}

I repeat again that I have read the docs again and I understand now why this happens. I may not agree, but I understand. But you have to observe that this is a side effect that's easy to miss for someone that's just picking up hyperhtml and trying to use it.

Secondly, please observe that my wording is about trying to find a way to help future users avoid this pain and maybe avoid abandoning hyperhtml in frustration.

Please slow down and read the text with from a human perspective, not a puritan web standards one. Yes, we are all fallible. But what makes web standards and the community around them great is trying to help each other out and grow. Not beating each other over the head. Your replies read very condescending. Not to me directly, but to some prototype of flawed web developer that you seem to lambast in your writings.

I'm trying to keep a hard skin, but I hope you can see my enthusiasm wearing thin when I attempt to help and I'm shot down for being imperfect.

Note that I was trying to suggest alternatives to help people avoid falling into the same problem, not demand that you change hyperhtml to suit my, or anyone else's, needs.

I suggested a low-hanging fruit could be to show console warnings with instructions when people do things you know are wrong.

Like partial attributes, for example. Throwing:

Uncaught TypeError: this[(i - 1)] is not a function

is not very helpful in the current situation.

On users

I have 719 stars at this point and 5 complains like this that I remember.

Don't conflate people's bookmarking with validation. More so, if you had 5 people who went through the effort of telling you what problems they encountered, you know for sure at least 5 people use your work and are genuinely interested in it. Listen to users when they attempt to talk to you. Especially when they argue in a non-sentimental way, as I've tried before this one response (I'm sorry, but I had to show some emotion from behind this keyboard too). You don't have to cater to all of their needs, but they're a good indicator for problems that are genuinely important for them.

You're very knowledgeable about web standards and I can see you're genuinely trying to make web dev better for people. I really respect you for that. But you're also very opinionated and, at times, too impassioned with your creation to stop and listen. I hope you slow down a bit, reflect, then continue to do great stuff attuned more to people who are tying to use it and/or help you.

To end on a more constructive note: what do you think about a hyperhtml tag on stackoverflow where we log questions and answers for these gotchas? Others may find solutions easier like that than buried in closed issues.

(Please don't start lambasting Stack Overflow now 😄. See it as tool.)

WebReflection commented 7 years ago

Wow, I'm telling you the exact same: slow down, I've just answered all of your thoughts reasoning about them, why did you take it so personal?

I'll try again to explain how to solve your issues.

First, you were in such a rush to reply that you missed the original issue

I haven't missed anything. I've showed you the counter code that has no issues.

function render(){
  return this.html`
    <select>${
      this.state.items.map(item => OptionElement(item))
    }</select>
  `;
}

You talked about new lines for readability, above works as expected without producing strings and is as readable as your example and it does what you expect and it's readable.

this is a side effect that's easy to miss for someone that's just picking up hyperhtml and trying to use it.

The library is simple but it has its own gotchas like any other library: you need to know how to do things.

In order to do that, I need to write documentation, I hope we agree on this part.

Back to your example, look at these two templates:

function select(items){
  return hyperHTML.wire(items)`
  <select>${
    items.map(item => option(item))
  }</select>`;
}
function option(item){
  return hyperHTML.wire(item)`
  <option value="${item.id}">
    ${item.name}
  </option>`;
}

hyperHTML.bind(document.body)`
<form>${
  select([
    {id: 0, name: 'a'},
    {id: 1, name: 'b'}
  ])
}</form>`;

Do you see the difference between select and option ?

Isn't what you are asking me to console.warn every time somebody wants text in element?

partial attributes, for example. Throwing ... is not very helpful in the current situation.

Rather than having 2 and a half rule for the template I'm not sure how to address this.

What you are asking me, is to throw away all performance and inspect every single attribute of every DOM node on the page in order to find out if somebody forgot to read the 2.5 rules.

I am afraid this is not going to happen. I'm the laziest dev of this planet but there's a limit to everything: no partial attributes is an hyperHTML constrain, the same as you cannot do onClick=alert{123} in React.

This looks reasonable to me, if interested, learn the few rules and stick with them, and remember: if you write a Syntax error in Javascript, the parser is going to tell you SyntaxError and most likely goodbye (on minified code where line numbers make no sense).

Do you want a try catch around each function invocation? It's a no-go for performance, I'm sorry.

One day maybe I'll create also a AST parser that tells you upfront in your IDE what you are doing wrong.

Asking me to put these things in the library is not the solution.

However, maybe these slides from the talk I gave could help you or others understand better what is all this about, at least I hope so:

http://webreflection.github.io/talks/nodejs-meetup-london/

WebReflection commented 7 years ago

On a second thought though ...

Do you want a try catch around each function invocation?

I could crate an update function, the whole fast path of this library, differently in case you previously setup a global DEBUG=true variable or something similar.

In that case it's easy to branch out the try { update() } catch(e) { tons of info } case, yet the spaces around interpolation remains, until some different idea comes up, a constrain of the library.

WebReflection commented 7 years ago

P.S. the Stack Overflow idea is ace and as much as I don't Stack Overflow much, I see it as a valuable tool for documentation.

I don't now if hyperhtml is the right tag (probably it is) because hyperhtml is already a GitHub and a Twitter account I don't owe, while viperhtml is the umbrella name ( also everyone laughed about hyperHTML name but it's the best word they remember :smile: ).

On top of that, I have no idea how to create a tag at all in Stack Overflow.

I will try to do so, not sure what's needed, never done it before.


Side note

I'm doing 767834567887654 things per day at this time and I'm rushing answers. Last 2 days has been crazy and both you and Jordan felt like my replies were harsh. You are both probably right but all I am trying to do is to quickly help ASAP while I'm in the middle of a storm doing all other things so I don't mean to be mean but everything you're telling me is a well known documentation issue and I'm restating myself each time while I'm trying to write such documentation.

Nothing against anyone interested and I perfectly know it doesn't click instantly because it uses an approach that is new to every developer.

That's the pride and the issue of this library or its side-projects: everyone think they can instantly use it but it has its own constrains and for valid reasons, developers should try to read more examples/poor docs while I'm trying to create a good documentation.

I hope you won't ever take any of my answer as a personal attack again, 'cause I haven't taken any of your comments personal neither: I know the docs state is poor, either I keep answering tickets about poor docs or I keep producing better docs. Doing both seems to be out of my abilities.

Apologies for that.

oslego commented 7 years ago

It's ok. Thanks for seeing the human situation from my perspective too.

Remember that you don't need to do everything and be immediately responsive.

Take care of yourself. Take a break. Especially if you feel overwhelmed.

Replies can wait. Nobody has, or should have, the expectation to get instant detailed replies. In many cases a simple: "Issue acknowledged. Working on improving the docs at URL" is enough if you really want to respond instantly.

I'll have a look too at Stack Overflow to see if there's any formal process or just arbitrarily tagging an question is enough to generate a tag. Both hyperhtml and viperhtml are good candidates as long as he projects exist separately. Don't sweat it about people making fun of the library name.

WebReflection commented 7 years ago

as related follow up: https://github.com/WebReflection/hyperHTML/issues/79#issuecomment-320925053