[Fiber] returning functions from render does not throw

[aweary]

https://jsfiddle.net/3apqnhmw/

class Foo extends React.Component {
  render() {
     return <div>Foo</div>
  }
}

class App extends React.Component {
  render() {
    return Foo
  }
}

When App returns Foo instead of <Foo /> accidentally, Fiber does not catch this.

[gaearon]

Looks like a bug. Why does it happen?

[aweary]

It looks like reconcileChildFibers does some minimal validation behind the disableNewFiberFeatures flag, but with the latest alpha release, those paths aren't hit since it's false. It doesn't even check if nextChild is a function though. edit: that check is actually fine, I still forget how invariant works sometimes

It does also check for undefined but it will actually crash if you try to return undefined, which seems like another issue. Try changing return Foo to return undefined in the example above and it will crash the tab.

[aweary]

Warning, the link below will cause an infinite loop and might crash the tab/browser

---

@gaearon Here's an example showing that returning undefined will cause an infinite loop. And a screenshot showing the endless barrage of errors:

I can open a separate issue for this if you'd like

[gaearon]

The infinite loop is probably #9193 or #9092.

[aweary]

Got it, thanks. As for the function issue, I think this would be fixed if those invariant checks in ReactChildFiber were moved out of a disableNewFiberFeatures block, or added to the correct code path if this warning should be done elsewhere now.

[acdlite]

Right now we treat unknown types as empty. We have a special case for undefined since it's almost always a bug (early return). I don't think we decided what we'll do for other unsupported values. We can either warn and treat as empty, or throw.

[acdlite]

One disadvantage to throwing versus warning is that it requires an extra check in production, for null. EDIT: I guess we already do a null check

[aweary]

I think using just a warning would make it too easy for components to silently fail. Existing behavior is to throw as well. The validation logic is already in the bundle, it's just dead code right now. Can we safely move it outside of the disableNewFiberFeatures check?

[HunderlineK]

I was actually about to ask if we could make this type of render work? I have various components (Foo, Goo, Hoo, etc.) that I need to conditionally render. Right now I use switch :

class Foo extends React.Component {
  render() {
     return <div>Foo</div>
  }
}

class Goo extends React.Component {
  render() {
     return <div>Goo</div>
  }
}

class Hoo extends React.Component {
  render() {
     return <div>Hoo</div>
  }
}

class App extends React.Component {
  render() {
  	console.log(Foo);
    switch (this.props.whatToRender) {
      case Foo: return <Foo />;
      case Hoo: return <Hoo />;
      default: return <div>No match</div>;
		}
  }
}

ReactDOM.render(<App name="world" whatToRender={Foo}/>, document.getElementById('container'));

if render () { return Component } would work, instead I could use the map method on the components array:

// whatToRender = Foo;
components = [Foo, Goo];
render() { return components.find(component => component === whatToRender) }

[gaearon]

@HunderlineK You can just wrap it in React.createElement() for the same effect.

[gaearon]

I made a list of different things which I think we should (dis)allow as return values.
It's based on what we did in 15, but with added support for numbers/strings/arrays.

            return value        child

string      yes                 yes
number      yes                 yes
element     yes                 yes
array       yes                 yes
iterable    yes                 yes
null        yes (ignored)       yes (ignored)
false       yes (ignored)       yes (ignored)
true        no*                 yes (ignored)
undefined   no                  yes (ignored)
function    no*                 yes** (ignored)
symbol      no*                 yes** (ignored)
object      no                  no

The ones with asterisk are the ones we currently silently skip (but used to error on). The ones with two asterisks are the ones we always skipped. IMO we should at least warn about them. Need to evaluate how many more conditions this would add.

We probably need to start with warnings anyway because we might already happen to rely on some of them at FB since we enabled new features a while ago.

I only feel strongly about function here because it’s a really common one. For example if you misunderstand components and return Foo rather than <Foo />.

[gaearon]

Since true seems harmless, and symbol seems rare, looks like the actionable item here is just to warn on function. Whether as a host child (e.g. of a div) or as a return value.

[gaearon]

#10376