facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.93k stars 46.52k forks source link

Add fragment API to allow returning multiple components from render #2127

Closed AdamKyle closed 7 years ago

AdamKyle commented 10 years ago

Note from maintainers:

We know this is an issue and we know exactly what set of problem can be solved. We want this too but it is a hard problem with our current architecture. Additional comments expressing desire for this feature are not helpful. Feel free to subscribe to the issue (there's button in the right hand column) but do not comment unless you are adding value to the discussion. "Me too" and "+1" are not valuable, nor are use cases that have already been written in the comments (e.g., we know that you can't put <tr> or <dd> elements with a <div>).


Consider the following:

var ManagePost = React.createClass({

  render: function() {
    var posts = this.props.posts

    var something;
    var somethingelse;

    var row = posts.map(function(post){
      return(
        <div>
          <div className="col-md-8">
          </div>
          <div className="cold-md-4">
          </div>
        </div>
      )
    });

    return (
        {row}
    );
  }

});

If you remove the <div></div> in the map, you get the following error: Adjacent XJS elements must be wrapped in an enclosing tag

it isn't till I re-add the surrounding, and rather pointless, divs that it compiles with out issue. I am running 0.11.1

Is this being addressed? It adds extra, and again - IMO - useless and pointless html to the page, that while harming nothing - looks messy and unprofessional. Maybe I am just doing something wrong, please enlighten me if I am.

chenglou commented 10 years ago

Because when you don't put the wrapper, it desugars to this:

return React.DOM.div(...)React.DOM.div(...)

Which doesn't make syntactical sense. The jsx compiler page might help if you need a visual mapping.

That being said, it's possible to desugar this to [div, div] instead. This is hard, somewhat controversial and won't be implemented in a near future.

sophiebits commented 10 years ago

(I don't think it's particularly controversial but it does add code complexity and hasn't been done yet.)

chenglou commented 10 years ago

IIRC @syranide has a few comments on this

syranide commented 10 years ago

@chenglou Hehe.

I had a brief chat about it with chenglou a little while ago, I don't really lean one way or the other at the moment. I see a lot of hidden dangers in allowing a composite components to return multiple components and it breaks a lot of intuitive assumptions, but I don't know of any (right now) good practice use-cases that would obviously benefit from this.

The simplicity of returning at most one component means that it's very easy to reason about what you see, if not, <table><TableHeader /></table> could actually render any number of rows, you have no way of knowing other than inspecting TableHeader and whatever composite components it returns.

I feel like being able to return multiple components only moves the responsibility of wrapping components as necessary from the "composite component" to whatever "renders the composite component". The component "that renders composite component" rarely has or should have the knowledge of whether to wrap composite components or not, whereas children are more likely to have knowledge of their parents.

But perhaps it's simply a case of developer responsibility. There may be good use-cases for both and we should just look past the inevitable misuse.

@sebmarkbage Probably has a few comments too :)

sebmarkbage commented 10 years ago

We will probably never allow that syntax implicitly. You would need a wrapper like

<>
  <div className="col-md-8">
  </div>
  <div className="cold-md-4">
  </div>
</>

OR

[
  <div className="col-md-8">
  </div>,
  <div className="cold-md-4">
  </div>
]

However, even this doesn't work. Often, it's probably for the best. It can be confusing for consuming components when a child might expand into multiple elements.

But, really, the only reason we don't support this right now is because it's hard to implement. Hopefully, we'll be able to support it sometime in the future but probably not short term. Sorry. :/

AdamKyle commented 10 years ago

can this not affect things like jquery or other libraries that target specific elements, so if you do something like $('#id-name').children(), the following:

<div id="id-name">
  <div>
    <div class="class-name">
    </div>
  </div>
</div>

the <div> and <div class="class-name"> would be selected in this case. (If I understand this properly)

DAddYE commented 9 years ago

It affects also css selectors in the very same way @AdamKyle posted before.

srph commented 9 years ago

Any updates on this issue?

gabssnake commented 9 years ago

Spent a couple of minutes understanding why my component didn’t work. I feel like there should be a notice somewhere, maybe I missed it ? Maybe it is obviously wrong to try :

var Optimistic = React.createClass({
  render: function() {
    return ( 
      <h1>{this.props.name} loves React</h1>
      <p>React doesn’t. Idea: sprinkle some divs here and there.</p>
    );
  }
});

React.render(
  <Optimistic name="Peter" />,
  document.getElementById('myContainer')
);
sophiebits commented 9 years ago

@gabssnake You should have gotten a JSX compile error with the error "Adjacent XJS elements must be wrapped in an enclosing tag"; did you not see the error or was it not clear in its explanation?

gabssnake commented 9 years ago

Thank you for your answer @spicyj. Well, I meant a notice in the React documentation. Yes, the console did show an error, but the need to wrap didn’t make sense to me at first. That is why I searched and got here.

GetContented commented 9 years ago

I've had this pain as well... particularly painful for my designer, as a matter of fact. It'd be kind of nice if a component could output a node (therefore node-list or fragment) instead of an element.

gaearon commented 9 years ago

Just saying.. I'm not advocating returning multiple children from component but I'd love to do that in render* methods that I extract from render:

  render: function () {
    return (
      <div className={this.getClassName()}
           style={{
             color: this.props.color,
             backgroundColor: this.props.backgroundColor
           }}>
        {condition ?
          this.renderSomething() :
          this.renderOtherThing()
        }
      </div>
    );
  },

  renderSomething() {
    return (
      <>
        <div className='AboutSection-header'>
          <h1>{this.props.title}</h1>
          {this.props.subtitle &&
            <h4>{this.props.subtitle}</h4>
          }
        </div>,

        {hasChildren &&
          <div className='AboutSection-extra'>
            {this.props.children}
          </div>
        }
      </>
    );
  }

But I should probably just shut up and use keys.

syranide commented 9 years ago

@gaearon You can do that already though, you just need to return an array for now (which is a bit cumbersome though yes) ... buuuuut, you can work around that, I have hacked my own <Frag> component which is translated to an array (overloaded React.render) ... you could also do return <NoopComp>...</NoopComp>.props.children I assume, if you want to avoid hacks.

EDIT: My bad, I overloaded React.createElement not React.render.

gaearon commented 9 years ago

The problem with arrays is they trip our designer. Need commas, explicit keys.

syranide commented 9 years ago

@gaearon Yeah you can avoid the commas using either of my two workarounds for now (if you find either acceptable)... but what you do mean with explicit keys?

gaearon commented 9 years ago

If I use array syntax, I need to specify key on each element. Not that it's hard to do, but feels awkward because I know they never change.

syranide commented 9 years ago

@gaearon Ah yes, I choose to just mentally ignore that warning for now :), if you really want to to avoid it, you can do <MyComp children={this.renderWhatever()} /> to avoid it (EDIT: although you obviously can't use that if you have adjacent children, you could use some flattening helper... but yeah).

natew commented 9 years ago

Another case I've ran into with a UI kit. When you are placing children inside a fixed scrollable container like so:

return (
  <div style={{
    position: fixed
    top: 0;
    left: 0;
    right: 0;
    bottom: 0;
    overflow-y: scroll;
  }}>
    {this.props.children}
  </div>
);

The children now must be passed as an array, but if a composite class is passed, it must also implement those styles to avoid breaking it. Just adds a layer of complexity. But I can completely understand how complex it must be to make the change. Just throwing my hat in for support.

Prinzhorn commented 9 years ago

I have another use-case I described in detail over here #3415. But I can workaround it for now and understand that it is hard to implement and rare.

I know nothing about react internals (yet), but I'll just throw in an idea how you could mark such virtual parent elements/fragments in the DOM: comments. E.g.

<A>
    <B></B>
    <Fragment>
        <C></C>
        <D></D>
    </Fragment>
    <E></E>
</A>

would render to

<a>
    <b></b>
    <!--<fragment data-reactid="">-->
        <c></c>
        <d></d>
    <!--</fragment>-->
    <e></e>
</a>

This means c and d will be treated as children of sth. (including the nested reactid to not conflict with b and e). I've seen other frameworks "misuse" comments for these kind of semantical jobs.

GetContented commented 9 years ago

@Prinzhorn I think you might be confusing DOM output with React's JSX.

aldendaniels commented 9 years ago

In case it's useful: @Prinzhorn's suggestion of using HTML comments to map "fragment components" to the DOM is the same approach that Knockout uses. Knockout calls these "virtual elements".

<!-- ko component: "message-editor" -->
<!-- /ko -->

(from knockout docs)

aldendaniels commented 9 years ago

Also, another use case for this - the extra wrapping elements can be problematic when using flexbox.

natew commented 9 years ago

@aldendaniels absolutely agree, I've ran into flexbox issues already.

brigand commented 9 years ago

I ran into this with flexbox with dependent dropdowns. Where A would render and manage the first dropdown, followed by B or C or D depending on the value of A's dropdown, which would each render the appropriate number of dropdowns, e.g.

<A>
 <Drop />
 <C><Drop /><Drop /></C>
</A>

A, B, C, and D are stateless so I changed them from class B {render(){ this.props }} to function B(props){ return [...]; }.

In this case, it doesn't much matter to the parent that multiple children are rendered, it's just to deal with my CSS. There are things I'd do differently if these didn't need to be reused (another component needs B, C, and D).


As an alternative, maybe a way to unravel it from the parent? I don't have any specific ideas of how that'd look.

jeffhandley commented 9 years ago

I hit a scenario today that I think is a good use case for this feature: A component that renders multiple <script> elements, where it could be rendering into the <head> element of the page. Any wrapper element there would be bad.

My scenario is wanting to have a component that is responsible for rendering the <script> tag both for the runtime code needed on the page as well as another <script> tag that carries the localized strings to be used by the runtime code. For example:

<html>
    <head>
        <script language="runtime.resources.en-us.js"></script>
        <script language="runtime.js"></script>
    </head>
    <body>
    ...
    </body>
</html>

In this case, I'd like to have the code authored as:

var RuntimeScripts = require('./Runtime')
...
return (
    <html>
        <head>
            <RuntimeScripts language="en-us" />
        </head>
    </html>
)
...
Axbon commented 9 years ago

I also ran into some flexbox issues. Nothing that can't be solved in CSS, but one of the "beauties" of flexbox is that you need less "wrapper" elements everywhere to make your layout work, but you'll still end up with wrapper elements everywhere when using React since you always wrap whatever you return in div/div or similar, unless it makes sense to have a container.

syranide commented 9 years ago

For all the use-cases presented here I'm pretty sure you could replace <BunchOfComponents /> with {getBunchOfComponents()} and the visual output would be the same, without introducing the practical and technical issues related to having components with fragments as root.

wmertens commented 9 years ago

@syranide but every time one of the components changes all of its siblings need recalculating...

wmertens commented 9 years ago

Also if you use plain coffeescript it's easy to return an array, so please decouple the functionality from the jsx representation. IOW if it's easy to handle a returned array of elements, don't wait for jsx to catch up.

syranide commented 9 years ago

@syranide but every time one of the components changes all of its siblings need recalculating...

@wmertens Yes, but many times you would have that anyway because the parent would need to rerender for other reasons, or simply because you receive the data through props anyway. But yes, that is the difference, but it doesn't mean this approach is right, it's an optimization and there are many ways to accomplish them.

Also if you use plain coffeescript it's easy to return an array, so please decouple the functionality from the jsx representation.

That is irrelevant and this is not a problem with JSX. A big issue is that you lose the technical, practical and intuitive assumption of one component = one element/node. I can't speak for the devs but I would not give up that willingly, it's a very useful assumption to have. I'm sure there are equally good or better optimizations that could be designed if optimization is the only the reason people want this.

wmertens commented 9 years ago

@syranide the biggest problem is that you can't always use a wrapping element in html, like in tables, lists, flexbox, head... Working around that leads to ugly code.

I would be perfectly happy with a virtual wrapper element that only renders comments, as suggested before.

On Fri, May 29, 2015, 3:56 PM Andreas Svensson notifications@github.com wrote:

@syranide https://github.com/syranide but every time one of the components changes all of its siblings need recalculating...

@wmertens https://github.com/wmertens Yes, but many times you would have that anyway because the parent would need to rerender for other reasons, or simply because you receive the data through props anyway. But yes, that is the difference, but it doesn't mean this approach is right, it's an optimization and there are many ways to accomplish them.

Also if you use plain coffeescript it's easy to return an array, so please decouple the functionality from the jsx representation.

That is irrelevant and this is not a problem with JSX. A big issue is that you lose the technical, practical and intuitive assumption of one component = one element/node. I can't speak for the devs but I would not give up that willingly, it's a very useful assumption to have. I'm sure there are equally good or better optimizations that could be design if optimization is the only the reason people want this.

— Reply to this email directly or view it on GitHub https://github.com/facebook/react/issues/2127#issuecomment-106810565.

thomasboyt commented 9 years ago

fwiw, It's relatively easy to hack a "fragment" component into React that is treated as an array of its children by React. It will auto-generate keys, but since it happens after the initial validation of components, it won't throw the usual "no keys supplied" error.

With that said, that hack only solves what @gaearon was talking about above - not having to deal with ugly array syntax/setting arbitrary keys in your JSX - and not the issue of returning multiple nodes at the root of a component's render method.

I have a problem with the idea that a component needs to return one "element/node". To me, it seems perfectly reasonable for a JSX structure of:

<Main>
  <Foo />
  <Fragment>
    <Bar />
    <Baz />
  </Fragment>
</Main>

to end up as the DOM:

<div>
  <div>Foo</div>
  <div>Bar</div>
  <div>Baz</div>
</div>

I don't think this is a principle-of-least-surprise violation because components already do all kinds of "surprising things" with the DOM using lifecycle hooks (just look at the common wormhole pattern). It is generally accepted that a component will not necessarily result in a single element being created, and that's fine, because some compromises have to be made to work with the DOM.

This isn't about "optimizations," either, or even just about not liking array syntax. As many users have mentioned, wrapper elements break styling and layout in serious ways. Tables are the most obvious, but Flexbox is also a major issue. I already have CSS that just reapplies flex rules to wrapper elements that only exist because of React, and it's pretty ugly.

For all the use-cases presented here I'm pretty sure you could replace with {getBunchOfComponents()} and the visual output would be the same, without introducing the practical and technical issues related to having components with fragments as root.

This requires developers to compromise on making isolated, reusable components - heaven help them if they decide they want to reuse their bunch of components elsewhere - because of an underlying implementation issue in React. I don't think that should be accepted.

syranide commented 9 years ago

@thomasboyt

EDIT: My mistake, I conflated some of your arguments with the table-discussion above, I largely agree with what you're saying I think. But there are still problems with components being opaque, so what's intended as a useful transparent wrapper becomes opaque to the parent. Imagine <Wrapper1><Wrapper2>...</Wrapper2></Wrapper1>, Wrapper1 cannot see the children of Wrapper2. So perhaps wrapMyElements(...) simply is a better solution all-around still (including any other necessary supporting functionality).

I have a problem with the idea that a component needs to return one "element/node". To me, it seems perfectly reasonable for a JSX structure of:

Components are more than just dumb wrappers, they have a purpose. IMHO it seems that returning multiple elements blocks some very useful expectations. For example, React.render will get a companion in the future which renders an element and returns the nodes, this must now produce an array of nodes instead.

But I think a very important issue is that of readability which IMHO is the biggest selling point of React, everything is explicit.

<table>
  <tr>
    <td />
    <td />
    <td />
  </tr>
  <tr>
    <Columns1 />
    <Columns2 />
  </tr>
</table>

Looking at that it makes no sense, where is the 3rd cell coming from? Perhaps it's actually wrong and it's rendering 2 or 4 cells, who knows, perhaps it's actually dynamic and depends on a prop or external state? There are many of variations of this problem which only get harier when you consider other non-HTMLDOM frontends which may have explicit expectations. Another thing to consider is that elements are opaque, so if you replace <tr /> with <MyMagicalTr /> then it is not able to interact with the individual cells or even deduce how many there are, so even though <MyMagicalTr /> may only accepts <MyMagicalTd />'s there is no guarantee it can actually interact with them.

This requires developers to compromise on making isolated, reusable components - heaven help them if they decide they want to reuse their bunch of components elsewhere - because of an underlying implementation issue in React. I don't think that should be accepted.

"This requires developers to compromise on making isolated...", but that is exactly the problem if you ask me, if a component can return multiple elements it's no longer isolated, it's substituted, the component is leaking into the parent.

Hammer, nails. It being an underlying implementation issue is a separate issue from whether or not this should actually be done at all. It's not my decision, but I don't see how one rare use-case is convincing argument without considering the trade-offs that comes with it or what other alternative solutions there are.

IMHO I don't see the problem with {getBunchOfComponents()}, it's explicit, it allows us to keep our useful expectations. If performance is a problem then React.createSmartFragment() (or w/e) to the rescue, a transparent array/object-like type but one which can update independently from its parent.

Again, the React devs are the authority (not me), but I don't see a convincing argument here considering the various side-effects. I'm not even sure I agree with the presented solution being a good pattern at all, even if it was supported.

EDIT: To clarify, perhaps components may be able to return multiple elements in the future because there are other obviously beneficial use-cases, especially in the context of passing through children (like the one you show @thomasboyt), readability is maintained.

thomasboyt commented 9 years ago

I think I'll need a bit more coffee before I can respond to the philosophical side of this conversation (thanks for the very good points, @syranide), but on the implementation side, I started poking around at this last night to see how viable a change of this scope is, leading to this spike: https://github.com/facebook/react/compare/master...thomasboyt:fragment

And threw up a little demo here: http://www.thomasboyt.com/react-fragment-demo/

Some observations on the implementation side of things:

I'm still not convinced that requiring fragments to maintain a count of their root nodes is the best way to do this (though it did at least get me to that demo), and this was all hacked together quite rapidly and quite late at night, so if anyone else has a suggestion for implementation, feel free to chime in :>

syranide commented 9 years ago

@thomasboyt IIRC the main implementation obstacle comes from React referencing child nodes by mountIndex, this does not work when one "node" can suddenly become any number of nodes and this may happen without invoking the parent and it may also happen several components deep (wrapping). If I'm not mistaken it's pretty trivial to have React support multiple root elements as long as the number never changes.

So I don't think it would be especially hard to just make it work in React, but a truly proper solution is more problematic and should probably involve dropping mountIndex.

thomasboyt commented 9 years ago

@syranide Right; the solution I'm working on actually introduces a new nodeIndex that is supposed to be the "real offset" of a node (which reminds me that I need to go back and remove mountIndex, since I think it's now unused in my branch).

But, as you note, this is problematic if the number of root elements changes, as the nodeIndex of a component needs to be updated whenever a previous sibling component's node count changes. Still need to find a solution for that.

landabaso commented 9 years ago

I've also run into flexbox issues. @syranide could you please elaborate a bit more on your "getBunchOfComponents" proposed solution? Being new to React it's hard to fully get the idea where to define this function / how to apply it.

syranide commented 9 years ago

@landabaso

function getBunchOfComponents(...) {
  return [<ColumnA key="a" />, <ColumnB key="b" />];
}
slorber commented 9 years ago

Hey,

I've not read the whole thread but here's a render optimization usecase that may require this feature:

http://stackoverflow.com/questions/30976722/react-performance-rendering-big-list-with-purerendermixin

slorber commented 9 years ago

If this feature is released, ReactCSSTransitionGroup won't need a wrapper node anymore right?

sophiebits commented 9 years ago

@slorber Yes, that's probably true.

wangchj commented 9 years ago

Run into the need for this feature everyday.

GetContented commented 9 years ago

If you have many small components (ie design very modularly), you end up having to wrap all kinds of things in divs that shouldn't be. I might be conflating, but I think this relates to this issue.

wangchj commented 9 years ago

For <div>'s you can wrap them in a <div>, but for table rows <tr> elements, it's not so easy. You can wrap <tr> in <tbody>, but may not be desirable to have multiple layers of <tbody> wrapping multiple layers of <tr>'s.

jeffhandley commented 9 years ago

The scenario I called out was trying to have a component that provides <link> and <script> elements without having to become the <head> renderer entirely.

zpao commented 9 years ago

I added a note to the top of this issue. Please read it before commenting. https://github.com/facebook/react/issues/2127#issue-41668009

dead-claudia commented 8 years ago

Bump...I have a large need for it on the server side. It's very complicated to render full web pages (excluding doctype) without the ability to render fragments, because of the <head> section. I'm currently working around that via mixins and a little logic in the final render, but it would be much simpler if there was support for rendering multiple components.

goldensunliu commented 8 years ago

@impinball you can attempt to write something similar to react-document-title based on react-side-effect to solve these problems. I was able to do the same for meta tags, headers, title and occasionally redirects

jonchay commented 8 years ago

I'm hitting this problem as well, are there any workarounds for the time being? I couldn't get {getBunchOfComponents()} to work as suggested.