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 · 8 comments
Open

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

rauschma opened this issue Aug 22, 2020 · 8 comments
Labels
discussion Discussions and proposals has fix A fix has been crafted question Further information is requested

Comments

@rauschma
Copy link

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 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 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?

@developit developit added discussion Discussions and proposals question Further information is requested labels Sep 21, 2020
@jimniels
Copy link

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.

@developit
Copy link
Owner

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 added a commit to iandunn/no-build-tools-no-problems that referenced this issue Jan 31, 2021
looks like in htm you have to do this inside a nested html() template

see developit/htm#175
@iandunn
Copy link

iandunn commented Jan 31, 2021

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

iandunn commented Feb 1, 2021

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

htm.bind( React.createElement, {
    autoFragment: true,
}

@developit
Copy link
Owner

developit commented Feb 5, 2021

@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?

@developit developit added the has fix A fix has been crafted label Feb 5, 2021
@iandunn
Copy link

iandunn commented Feb 5, 2021

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?

facebook/react#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussions and proposals has fix A fix has been crafted question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants