Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rauschma opened this issue Aug 22, 2020 · 3 comments
Open

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

rauschma opened this issue Aug 22, 2020 · 3 comments

Comments

@rauschma
Copy link

@rauschma rauschma commented Aug 22, 2020

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
Copy link

@bathos bathos commented Aug 27, 2020

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
Copy link
Owner

@developit developit commented Sep 21, 2020

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
Copy link

@jimniels jimniels commented Nov 18, 2020

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:

  • nest every component in a root <div> (less than ideal when semantic structure matters)
  • use @rauschma 's suggestion of <${React.Fragment}>

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.