developit / htm

Hyperscript Tagged Markup: JSX alternative using standard tagged templates, with compiler support.
Apache License 2.0
8.67k stars 170 forks source link

Shouldn’t multiple root elements become a fragment? #175

Open rauschma opened 4 years ago

rauschma commented 4 years ago

This works for me:

  return html`
    <${React.Fragment}>
      <h1>Quiz</h1>
      ${entriesUi}
      <hr />
      <${Summary} entries=${entries} />
    <//>
  `;

If I don’t wrap the fragment around the roots, I’m getting this error:

Warning: Each child in a list should have a unique "key" prop.

(That is, HTM seems to return an Array and not a fragment.)

If my suggestion doesn’t make sense, then it may be helpful to support the <>...</> syntax for fragments.

bathos commented 4 years ago

This threw me off too. I was expecting createElement(Fragment, {}, ...nodes) treatment for cases like this given the positions will be static and keying is known to be unnecessary up-front.

I’d actually thought that this was one of the main selling-points of htm — that the explicit boundaries of the template obviated the need for explicit fragments within the string content — but it seems I misinterpreted the meaning of the “Multiple root element (fragments)” bullet point in the readme section describing improvements over JSX.

developit commented 3 years ago

Hiya! Multiple roots returns an Array, because that's the easiest and most efficient solution across all frameworks:

const root = html`
  <h1>Quiz</h1>
  ${entriesUi}
  <hr />
  <${Summary} entries=${entries} />
`;

console.log(root);
[<h1>Quiz</h1>, entriesUi, <hr />, <Summary ../>]

It's not necessary to use a Fragment for this, because this is semantically an index-ordered list of children. It's best represented as an Array, because there is no useful key that could be produced for the items.

React's "key" warning is only a high-level generalized piece of guidance, and it does not apply to auto-generated lists of items for which there is no viable unique key. I have a StackOverflow post that explains this more in-depth, but the gist is that this advice tends to cause people to use index keys, which is extremely harmful to diffing. At best, index keys just duplicate the behavior of unkeyed homogenous lists. In all other cases, they cause shifted and conditional items to be remounted on every render.

Was the main issue here that React displayed the questionable warning, or is there a case I'm not aware of where this actually prevents usage?

jimniels commented 3 years ago

Was the main issue here that React displayed the questionable warning, or is there a case I'm not aware of where this actually prevents usage?

For me personally, it was that React keeps displaying a warning. Because htm makes multiple root elements so easy, I do it in many components. However, that leads to react throwing lots of warnings in the console, which makes finding things I've actually done wrong more difficult.

Screen Shot 2020-11-18 at 2 18 20 PM

Best fix at the moment is to either:

Being able to tersely type <>...</>, as JSX allows, would be a nice enhancement.

developit commented 3 years ago

Might be worth filing a bug with React for this if it's warning in this case - key isn't useful here and that makes the warning quite misleading.

iandunn commented 3 years ago

This confused/surprised me too, and took me awhile to understand what was going on / how to avoid it.

What does the readme mean by the following ?

Multiple root element (fragments): <div /><div />

Is it possible for HTM to automatically wrap multiple root elements in a fragment, or any harm in doing that?

What are your thoughts on a terse syntax similar to JSX?

<>
    <p>foo</p>
    <p>bar</p>
<//>
iandunn commented 3 years ago

...and/or maybe an optional config like:

htm.bind( React.createElement, {
    autoFragment: true,
}
developit commented 3 years ago

@iandunn hmm - we definitely can't do anything configuration-based in a library of this size. It's especially hard to justify given that benefit is effectively that a console warning stops being shown.

Patching automatic fragment wrapping in is pretty easy:

import htm from 'htm';
import React from 'react';

function html() {
  const root = htm.apply(React.createElement, arguments);
  return Array.isArray(root) ? React.createElement(React.Fragment, {}, root) : root;
}

... then use it like you would use html.

Maybe the best option here would be to ship the above patch by default in htm/react?

iandunn commented 3 years ago

Thanks, that sounds like a good idea to me.

I'm having trouble getting it to work, though. Or, it does work in the sense that it successfully wraps the return in a Fragment, but for some reason React is still complaining about it.

I diff'd the objects using that wrapper vs manually wrapping in Fragment, and don't see any difference, which is confusing. It does make me wonder, though, if it were as simple as this, then why don't things like @babel/plugin-transform-react-jsx just do it?

https://github.com/facebook/react/issues/8920 seems to imply that the short syntax <> is the best that can be done, and any kind of attempt to automatically detect static vs dynamic arrays is risky. I could easily be wrong, though.

If that's correct, then could htm/react support <>?


As an aside, false-negative errors don't feel like a trivial thing to me. It may be a personality difference, but I find them very distracting, and they make it difficult to focus on what (if anything) is actually going wrong.