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

Using setState inside a "function as child component" generates a warning about functions not being valid children #11116

Closed
CharlesMangwa opened this issue Oct 5, 2017 · 12 comments

Comments

@CharlesMangwa
Copy link

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

Reporting a bug.

What is the current behavior?

Context: React Native app, but the error isn't related to the renderer

I'm using a handcrafted <Fetch path="/api/route" render={this.renderChildren} /> component which fetches data and passes it to a ''function as child component", which is rendered. However when using setState to save the call result, I get a pretty interesting error as you can see:

screenshot of the code leading to the error

👇 gives you 👇

screenshot of the error

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 for React 16: https://jsfiddle.net/Luktwrdm/, template for React 15: https://jsfiddle.net/hmbg7e9w/).

I tried to remove all the business logic and reproduce the error from my React Native app. Here is a CodeSandbox which fully reproduce it: https://codesandbox.io/s/pmo6okkz1x.

What is the expected behavior?

I expected not to have this error.

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

From what I know: this error occurs on any React renderer (got it on React Native in the first place), and started the project with React 15.5.

Related issues
Might be related to: #11081, #11086.


Ping: @gaearon

@Tom-Bonnike
Copy link

I’m not sure I understand what you’re trying to do, but it seems like you‘re calling setState inside render, which is a bad idea for obvious reasons…

This error makes it pretty clear:
screen shot 2017-10-05 at 18 06 46

Even smaller reproduction: https://codesandbox.io/s/wq69qyvrx8

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

Yes, the problem is that _saveData in your example is called during rendering. So you start rendering, call setState, that starts rendering again, you call setState again, etc. This results in an infinite loop.

If your Fetch component takes care of keeping the state, and then calling its render prop to render that data, there's no need for your App component to try to do the same thing. Just render what Fetch gives you:

import React from 'react'
import { render } from 'react-dom'
import Fetch from './Fetch'

class App extends React.Component {
  _renderChildren = (data) =>
    <span>{JSON.stringify(data)}</span>

  render() {
    return (
      <Fetch
        path="https://unsplash.it/list"
        render={this._renderChildren}
      />
    );
  }
}

render(<App />, document.getElementById('root'))

Here is a working sandbox: https://codesandbox.io/s/48rrpxq33x

@gaearon gaearon closed this as completed Oct 5, 2017
@CharlesMangwa
Copy link
Author

CharlesMangwa commented Oct 5, 2017

Thanks @Tom-Bonnike @gaearon for quick answers. That's actually a little bit different from what I was trying to achieve there as you saw. The idea was to fetch data with <Fetch />, save it and then use that data with any other component that would need it. But your answers make sense with what I've put there indeed.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

The idea was to fetch data with , save it and then use that data with any other component that would need it.

The solution in your case would be to modify the render callback to pass that data down to any components that need it. I know it’s a bit counter-intuitive but since you went with this pattern, there’s no need to bring data “up” now in your component. Just tell Fetch what to render.

@CharlesMangwa
Copy link
Author

Thanks for your advice. That's exactly what I'm doing now:

<Fetch path="/api/route">
  {(data) => <MyComponent content={data} />}
</Fetch>

I'm just exploring different patterns to see if there is another approach that could be more interesting. Anyhow thanks again for your time!

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

One way to think about it is that for every piece of data there should be only one component that holds it in the state.

So you need to pick: either it's Fetch (and you can only use what it gives you via the render prop), or it's App (and in this case you need a different Fetch API that would call handleChange when the data loads instead of trying to keep it in its own state).

@CharlesMangwa
Copy link
Author

CharlesMangwa commented Oct 5, 2017

I'm leaning toward the second option, just to see what I can achieve with it.

In the first place, I wanted to lift my state up (using Fetch) and share that data. But that's probably where my mistake was, because it would lead to a ‘strange‘ pattern: a sibling (Fetch) fetching data, lifting it up inside the parent's state so that its siblings could exploit that data... After a second thought, this doesn't seem ‘natural‘.

On another note: do we also have access to handleChange/onChange inside any React Native component or is it tied to the DOM? I was thinking to onLayout on the <View /> component, but maybe that's not exactly what I'd want.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

But that's probably where my mistake was, because it would lead to a ‘strange‘ pattern: a sibling (Fetch) fetching data, lifting it up inside the parent's state so that its siblings could exploit that data... After a second thought, this doesn't seem ‘natural‘.

I think it's actually OK. Think Fetch is like an input.

The mistake was in trying to set the state in the render phase. Instead you could call this.props.onChange() with data from AJAX callback in Fetch, and render null from it. Then, in App, you would have a change handler that sets the state, just like you did.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

do we also have access to handleChange/onChange inside any React Native component or is it tied to the DOM?

Not sure what you mean. I suggest you to have Fetch accept onChange as a custom prop, and then have it call it when the data is fetched.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

Here's an example of the second approach I was referring to: https://codesandbox.io/s/j7jlyp07l9

It's very close to what you did originally, but without the state duplication or the update-state-during-render issue.

Hope this helps!

@CharlesMangwa
Copy link
Author

CharlesMangwa commented Oct 5, 2017

Okay! I got it now.

I thought you were referring to the handleChange method used here and I didn't get why.

The custom onFetch prop is exactly what I was trying to achieve, but naively doing it inside the render phase. Plus I like this approach because it's very similar to what I have now so I'll be able to incrementally adopt it without breaking anything.

It definitely solved my problem, thank you!

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

Happy to help!

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

3 participants