Add docs for using Promises and making AJAX requests

[rogeliog]

#521 This adds some simple documentation for the fetch and Promise polyfills.

@gaearon Should we also add a simple example of how to use this with React? Or do you think it falls out of scope?

class MyComponent extends Component {
  state = {}

  componentDidMount() {
    fetch('/todos.json')
    .then((response) => response.json())
    .then((todos) => this.setState({ todos: todos }))
    .catch((error) => console.log('Error', error));
  }

  render() {
    return <TodoList todos={this.state.todos} />;
  }
}

[ghost]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[ghost]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

Let’s make this one section: Fetching AJAX Requests. Let’s also put it before Integrating with a Node Backend.

[gaearon]

Let's say "the global fetch function" instead to make it clear you don't need to import it.

[gaearon]

Let's add all necessary imports here so people can copy and paste. Same in next block.

[gaearon]

Can we make this a drop-in replacement for App.js that ships with this project? Instead of UserGists maybe map over their names in a <ul>.

[gaearon]

Let's remove propTypes. Not essential to this example and introduces class properties which we don't want for now.

[hnordt]

Please consider https://github.com/smalldots/smalldots#fetch :)

[gaearon]

There is a subtle problem here. If setState() throws (for example due to an error in render()), we will get into the catch handler. It is then easy to miss or ignore that error, and now React is in an inconsistent state.

Instead, let's put catch() before the second then(). This way we only catch network errors. We want errors in setState() to stay unhandled.

Let's make catch() return an empty array. Then we can safely use its result in setState() whether it succeeded or failed.

[fson]

Also note that fetch doesn't reject/throw for an error status, so if GitHub is down and you get a 503 status, or if you get rate limited and they return 403, none of these errors will end up in the catch() – setState() gets called with the parsed JSON body of the error response.

So to handle HTTP errors, you'll also have to check response.status, e.g. response.status >= 200 && response.status < 300 (response.ok is a handy shortcut for that), and treat a non-OK status as an error.

[gaearon]

Yep, @insin mentioned this later in a comment too. We should handle both !ok and actual errors in the same way, that is, by showing the message.

[gaearon]

If the person copies and pastes this component into App.js, user prop will be undefined. Can you make it so this is actually copy and pasteable with no modifications?

I would suggest that, rather than display repos by user, let's display stargazers of facebook/react repo. Then we can hardcode it.

[gaearon]

Thank you so much for bearing with me and making these changes. I want to make this really good. 👍

[insin]

Should this include examples of displaying a loading indicator and suitable error messages on failure?

e.g. if you're rate-limited - which can happen very quickly with unauthenticated GitHub API calls - response.ok will be false and response.json() will resolve with object with a message property.

[gaearon]

Yes, this sounds like a great idea. Let's do it.

[rogeliog]

Hey! I'm so sorry for the late response, I've been really busy... Sorry once again

[rogeliog]

I tried to keep it at simple as possible since I'm a bit worried that the loading and error handling might introduce noise to the example

[rogeliog]

@gaearon just a friendly ping 😄, I'm more than happy to do any changes needed!

[gaearon]

Sorry I left this hanging. I was very busy with React in the past few months, and so I left some PRs neglected.

We tried to do something similar in React docs, and it didn’t go well. See the discussion in facebook/react#8098. There are so many possible edge cases and race conditions, all because fetch() API doesn’t support cancellation. (Which, for example, is necessary if props update.)

I’m not sure we should add examples that might set users on the wrong path. At this point I have a lot of doubts about fetch() being a good API for React apps, and I think axios is a better choice.

What do you think?

[hnordt]

In my experience axios is a better option. Fetch can be considered lower level

[bondz]

CRA does polyfill fetch for apps, most users would just use that anyway without considering the edge cases. Using observables or axios makes cancellation easy, just have to inform users not to use fetch in lifecycle methods.

[stereobooster]

Latest specification for fetch adds cancelation. Would be nice to see example of cancel.

[gaearon]

Thanks again. I'm sorry we didn't reach consensus here and let it slide.
For now I picked some parts from this PR without an extensive example, and merged that as d1adff0.