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

Allow generators (that yield components) in place of arrays #7536

Closed
mnpenner opened this issue Aug 20, 2016 · 11 comments
Closed

Allow generators (that yield components) in place of arrays #7536

mnpenner opened this issue Aug 20, 2016 · 11 comments

Comments

@mnpenner
Copy link
Contributor

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

Generators are silently discarded

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

This is allowed:

var Hello = React.createClass({
  render: function() {
    return <ul>
        {(() => [
        <li key="foo">foo</li>,
        <li key="bar">bar</li>
      ])()}
    </ul>;
  }
});

This should be legal too:

var Hello = React.createClass({
  render: function() {
    return <ul>
        {(function*() {
        yield (<li key="foo">foo</li>);
        yield (<li key="bar">bar</li>);
      })()}
    </ul>;
  }
});

Would be even better if the trailing () wasn't required, and it just executed the function.

What is the expected behavior?

Both examples should generate:

<ul><li>foo</li><li>bar</li></ul>

This would make it much easier to write if conditions in the middle of a JSX block. Here's a snippet of how this can be used from my current project:

 <div className="row">
     {generatorToArray(function*(){
         if(sameVehicleAndDriverForAllSegments) {
             yield <div key="billing_type" className="col">
                 <label>Billing Type</label>
                 <RadioGroup valueLink={linkState(billingForm, 'billing_type')}>
                     <RadioButton value="HOURLY">Hourly</RadioButton>
                     <RadioButton value="FLAT">Flat</RadioButton>
                 </RadioGroup>
             </div>;
         }

         switch(data.billing_type) {
             case 'HOURLY':
                 yield <div key="hourly_rate" className="col">
                     <label htmlFor="hourly_rate">Hourly Rate</label>
                     <div>$<TextBox id="hourly_rate" valueLink={linkState(billingForm, 'hourly_rate')} className="amount-input" disabled={!vehicleId}/>/hour</div>
                     <div><Slider {...hourlyRateSliderOptions} valueLink={linkState(billingForm, 'hourly_rate')} disabled={!vehicleId}/></div>
                 </div>;
                 break;
             case 'FLAT':
                 yield <div key="flat_rate" className="col">
                     <label htmlFor="flat_rate">Flat Rate</label>
                     <div>$<TextBox id="flat_rate" valueLink={linkState(billingForm, 'flat_rate')} className="amount-input" /></div>
                 </div>;
                 break;
         }
     })}
 </div>

Notice how I can write if conditions and switch statements without having to instantiate an empty array, push elements into it as needed, and then return an array at the end -- generators take care of all of that for you.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

n/a

@Bonuspunkt
Copy link

just came across the same issue

class X extends React.PureComponent {
    render() {
        const { object } = this.props;
        return (
            <li>
                { object.name }
                { this.renderIcons(object) }
            </li>
        );
    },
    *renderIcons(object) {
        if (object.x) yield (<Icon text={ object.x } />);
        /* more conditons */
    }
}

my current workaround is to wrap it with Array.from

{ Array.from(this.renderIcons(object)) }

@ksmithbaylor
Copy link

The Array.from solution seems to be the correct way to handle this. I don't think React should automatically iterate to completion any generator function it gets, because there's no guarantee that the generator will terminate. I think it's better to keep your intent explicit by creating an array from the generator first, since there's no such thing as an infinite array. Introducing values that are potentially non-finite to React's rendering algorithm would not be a good decision, in my opinion.

@jimfb
Copy link
Contributor

jimfb commented Sep 9, 2016

@ksmithbaylor Something like could be useful if/when React has integrated layout. A component could render an infinite scroll list (like facebook news feed) as a generator (or something similar), and React could render as much of it as is required to fill the user's screen. Otherwise, the component wouldn't know how big of an array to generate, and so it would need to always over-estimate, thus wasting CPU and memory.

@Bonuspunkt
Copy link

i found #2296 a while ago, but i havn't had the time yet to check, if this is just an issue with the transpiling to ES5 or an actual bug.

@ksmithbaylor
Copy link

@jimfb That's an interesting use case I hadn't thought of!

@sophiebits
Copy link
Collaborator

Right now we require that any iterables yield the same children each time you iterate through them. Otherwise we can't do diffing, etc. properly. I don't think it's likely this will change soon and it's likely that React would internally do basically an Array.from call so writing it in explicitly like suggested above is a reasonable choice.

@syranide
Copy link
Contributor

As shown it seems like generally a bad idea as it breaks Reacts implicit indices, so if the result ever changes you must be sure to key everything explicitly. I also fail to see the point, the only thing React can do is keep calling it until its done, so the generator doesn't really provide any benefit as far as I can tell, it becomes equivalent to:

function() {
  var result = [];
  result.push(...);
  result.push(...);
  return result;
}

But slower and less idiomatic, so what's the point of using generators as you show? Technically it probably makes sense as it's just another form of iteration.

@mnpenner
Copy link
Contributor Author

@syranide

so what's the point of using generators as you show?

It's just shorter is all. Your example would shrink to:

function*() {
  yield ...;
  yield ...;
}

Is it really slower? I know there's some overhead with regenerator, but in a couple years when browser support is good, would it still be slower than an array?

it breaks Reacts implicit indices

Are implicit indices even a good thing? I mean, they're nice for prototyping because I can just ignore the warnings and slowness, but in practice everything needs a key anyway, doesn't it? In fact, using a generator would discourage me from just using the array index (because there isn't one) which usually ends up being bad choice because elements shift up and down the array.

@spicyj

Right now we require that any iterables yield the same children each time you iterate through them.

They would yield the same children unless the state/props change. This isn't fundamentally different than arrays is it?

I think @ksmithbaylor has a decent point about generators that never terminate. However, React could handle this better than Array.from does. Array.from will just crash your browser, React could stop after 10K iterations or so (or X milliseconds) and throw an error.

@jimfb's idea is interesting, but I imagine any kind of infinite list or table would be handled by the specific component anyway, no?

Anyway, I respect the core team's decision on this. It's just syntax sugar.

@sophiebits
Copy link
Collaborator

I meant, React already iterates through any iterable more than once which seems incompatible with generators.

@syranide
Copy link
Contributor

syranide commented Sep 12, 2016

It's just shorter is all.

IMHO, sure, shorter syntax is definitely preferable to some extent, but it's important not to ignore the purpose behind the functionality either. Generators are a special class of functions (i.e. resumable functions), and not just simple "iterables", they're similar but not the same.

Is it really slower? I know there's some overhead with regenerator, but in a couple years when browser support is good, would it still be slower than an array?

It will almost certainly always be slower. Generator functions require their own stack which must be allocated and stored on the heap (rather than the stack) and involves restoring CPU registers every time its called. Its imaginable that it could be optimized away in some circumstances (i.e. when you don't use them async), but considering the complexity and dynamic nature of JS it seems very unlikely.

Are implicit indices even a good thing? I mean, they're nice for prototyping because I can just ignore the warnings and slowness, but in practice everything needs a key anyway, doesn't it? In fact, using a generator would discourage me from just using the array index (because there isn't one) which usually ends up being bad choice because elements shift up and down the array.

Implicit indices work perfectly for everything but dynamic lists. Generator functions may promote a behavior where people generate "static" content the same way (like someone did in this very issue). Also they're not primarily about performance, it's about maintaining correct behavior for stateful/animating components.

@gaearon
Copy link
Collaborator

gaearon commented Aug 2, 2018

We're going to start warning about this.
#13312

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

No branches or pull requests

7 participants