-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Add fragment API to allow returning multiple components from render #2127
Comments
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 |
(I don't think it's particularly controversial but it does add code complexity and hasn't been done yet.) |
IIRC @syranide has a few comments on this |
@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, 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 :) |
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. :/ |
can this not affect things like jquery or other libraries that target specific elements, so if you do something like <div id="id-name">
<div>
<div class="class-name">
</div>
</div>
</div> the |
It affects also css selectors in the very same way @AdamKyle posted before. |
Any updates on this issue? |
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')
); |
@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? |
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. |
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. |
Just saying.. I'm not advocating returning multiple children from component but I'd love to do that in 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 |
@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 EDIT: My bad, I overloaded |
The problem with arrays is they trip our designer. Need commas, explicit keys. |
@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? |
If I use array syntax, I need to specify |
@gaearon Ah yes, I choose to just mentally ignore that warning for now :), if you really want to to avoid it, you can do |
Another case I've ran into with a UI kit. When you are placing children inside a fixed scrollable container like so:
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. |
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. |
@Prinzhorn I think you might be confusing DOM output with React's JSX. |
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".
(from knockout docs) |
Also, another use case for this - the extra wrapping elements can be problematic when using flexbox. |
🦏 |
@gaearon Awesome improvement! Is it support pure text node? |
Yes, it supports returning strings too. |
May I ask how is this supposed to work? I was hoping it would allow us to render 2 adjacent XJS elements but if I do So was the feature a way not to surround a real list by an extraneous element? |
Yes, a way to provide several elements from one render. (For more information, see the initial issue post.) If all your array items are known to be strings, just join them and return a single string. If not, you can either eat this warning or work out what to do to wrap them in elements so that they can be keyed to improve DOM reconciliation, just as you would have if you had included an array of strings inside another element in JSX. |
@diligiant returning
It removes a big limitation in react. Btw returning |
@JesperTreetop thank you. |
@diligiant I doubt it introduces any performance penalty but you don't need a surrounding useless |
@sassanh cool then. Of course my use case was with multiple components. Happy!! ;) |
Actually it seems odd to me that we warn about missing keys when all items are strings. I wouldn't expect us to do so. Is this existing behavior in 15 when strings are inside a div? If not then we should fix it in 16 for top-level strings (since it's impossible to give keys to them). |
@gaearon sorry, my example was misleading: I meant components. I checked and under Nonetheless, I am puzzled by the And finally, thank you again. |
This warning is the exact same as for every other usage of arrays of components, because it boils down to the exact same "extra work" for the reconciler, and thus the exact same potential optimization by setting a |
.. and most of the time I will be returning an array from a function, I do want the |
The warning is necessary because lack of keys can cause correctness issues, not just performance problems. This is explained in many other issues asking why keys are necessary so I encourage you to search for them and read those discussions. I agree the docs could be clearer about this, and it's something we'll likely look at next time we do a docs change sprint. There is no conceptual differences between arrays directly returned from render, and arrays inside a div. So there is no reason why there would be a key warning in one case but not the other. They need to work the same way because both are affected by the same problems when the keys are missing. That said we understand it's annoying to specify keys for static content. Just like you don't specify keys when you write a JSX structure where children are statically known (and thus never reorder), it would be nice to have a way to do this with arrays. In the future we might solve this by adding explicit support for fragments in JSX with syntax like: return (
<>
<div>child 1</div>
<div>child 2</div>
</>
); It could produce an array but implicitly assign numeric indexes to the children because in this case we know they can never reorder. This guarantee given by JSX child expression is exactly what lets us get away without specifying keys in normal JSX code where a div might have multiple children. But we don't have this syntax yet. So for now this is a known limitation. |
@JesperTreetop & @zwily, @gaearon explained it way better than I did ;) Once you know, it's no big deal but as we all want React to thrive, I was just saying… |
@gaearon Is there another issue for the |
@smrq +1 to question — I follow everything about those awkward limitations (one-to-one I also suppose that fibers gonna fix issue with keys at all — but looks like it's unfulfilled dream |
Keys aren't an "issue". :) They're a fundamental and necessary part of any view framework that allows you to create dynamic lists. See https://facebook.github.io/react/tutorial/tutorial.html#keys for a more detailed explanation. |
@spicyj actually it is an «issue» since it cause extended energy spendings from developers and there is fundamental possibility to program view framework without such requirement (for example https://github.com/dfilatov/vidom) |
vidom does use keys in collections though. It might technically work without it but it would likely be much slower. React could also technically work without keys, but it'd be quite unexpected to find that half of your components have to update when you remove a single item from a list. With keys, only one item is unmounted. |
@goto-bus-stop vidom can use keys, but they are not required and without them only really big cases with a lot of updates can cause real performance problems so I consider this part as possible optional (as, for example, |
@veged example of vidom struggling without keys. It has no idea it's supposed to reorder the elements, so it discards the instances on each render of the root component. |
as someone who's quite familiar with the virtual-dom space [1]. i can say that:
|
however, the unambiguous issue here (as @gaearon described) is a fully static usage of arrays and the difference between static arrays and static JSX fragments |
@brigand O have no doubt that reordering of state-full components may cause some issues ;-) but force every other cases (in my opinion more of them) to struggle for this... looks controversial |
Keys are important to React. Even if they don't seem to matter in some case (e.g. because components below are all stateless) there is nothing preventing you or someone else on the team to add a stateful component (or even a plain DOM input) somewhere a few levels below in a few months. By then you might forget that you don't have keys, and thus reorders may cause the state (or input value) to be associated with a wrong item. This is why we encourage you to specify keys everywhere for dynamic lists. Not doing so leads to very hard-to-trace bugs, and we think it's better to spend extra ten seconds specifying the key than ten hours debugging why the state of a component several levels below gets messed up in some corner case. I fully agree it's inconvenient to specify keys when the list is known to be static and never reorders. You are welcome to discuss this on the JSX repo. If you can't find an issue for this, you are welcome to create a new one there. Since this thread has a lot of subscribers and the feature has been implemented I would like to lock it to avoid spamming a lot of people with notifications. I hope the clarifications above address your concerns, but if not, you are welcome to create a new issue for further discussion on a specific topic. |
@smrq created a proposal issue for |
We just released support for a new |
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:
If you remove the
<div></div>
in themap
, you get the following error: Adjacent XJS elements must be wrapped in an enclosing tagit 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.
The text was updated successfully, but these errors were encountered: