CODEWITHSUNDEEP
javascriptreactjstypescriptasynchronouspromise
Fernando Calvo
Fernando Calvo

Reputation: 83

How to use Promise.all properly fetching data for React app

I'm working on a React app, and I need to fetch some data when the component mounts. Since I need to make 3 different calls, I thought it would be better to use Promise.all in order to do it in a parallel fashion. The code down here works perfectly fine, it does exactly what I intend it to do, however, the person that revised the code flagged it as a bug with the message "you are not returning the promise here. Either await or promise but not both". I don't really understand what he meant with this. I'm just a beginner developer and he has quite a lot more experience than me, so I assume he's right and this piece of code is wrong, but I just can't figure out why.

async componentDidMount() {   
    const start = Date.now();

    // TODO: BUG! you are not returning the promise here! Either await or promise but not both!
    const promises = [this.fetchReports({}), this.fetchEntities(), this.fetchCodePulseRange()];

    await Promise.all(promises);
    console.log("Data fetching time: ", Date.now() - start)
    this.setState({ inited: true });
  }  

  async fetchEntities() {
    const res = await insightService.fetchEntities(this.props.profile.organisationId);
    this.setState({ allEntities: res, entityType: 'product' });
  }

  async fetchReports(entities: { [key: string]: string }) {
    const res = await insightService.fetchInsights(reports, entities, this.props.profile.organisationId, JSON.stringify(entities));
    this.setState({ reportsWithData: res });
  }

  async fetchCodePulseRange() {
    const res = await insightService.fetchCodePulseRange(this.props.profile.organisationId);
    this.setState({ codePulseRange: res });
  }

Upvotes: 2

Views: 2637

Answers (1)

T.J. Crowder
T.J. Crowder

Reputation: 1074335

Your best bet is to ask the person what they mean. I can't see any issue with your having put the promises in an array and then awaiting Promise.all so you know when they've all been fulfilled (or one of them has been rejected). It's possible that the reviewer isn't entirely clear on what Promise.all does and why it does make sense even in an async function (for exactly the reason you specified: starting the operations, then waiting for them all to finish, allowing them to run in parallel).

There is a problem that nothing in your code handles or reports errors from the fetch operations. That may be what they were trying to flag up, somewhat indirectly. One of the rules of promises is: Either handle errors, or propagate the promise chain to something that will. React won't handle a promise rejection from componentDidMount, so you need to handle the error yourself.

I would also modify he code so that you only make a single state change instead of four of them:

componentDidMount() {   
    const start = Date.now();

    const promises = [this.fetchReports({}), this.fetchEntities(), this.fetchCodePulseRange()];
    Promise.all(promises)
    .then(([reportsWithData, allEntities, codePulseRange]) => {
        console.log("Data fetching time: ", Date.now() - start)
        this.setState({
            reportsWithData,
            allEntities,
            entityType: "product",
            codePulseRange,
            inited: true,
        });
    })
    .catch(error => {
        // ...handle/report error...
    });
    this.setState({ inited: true });
}  

fetchEntities() {
    return insightService.fetchEntities(this.props.profile.organisationId);
}

fetchReports(entities: { [key: string]: string }) {
    return insightService.fetchInsights(reports, entities, this.props.profile.organisationId, JSON.stringify(entities));
}

fetchCodePulseRange() {
    return insightService.fetchCodePulseRange(this.props.profile.organisationId);
}

But if you want to keep the incremental updates (the four separate state updates), you can do that, just be sure to handle errors, both in componentDidMount and anywhere else that uses those fetchXyz operations. For instance, if you leave the fetchXyz operations alone, then:

componentDidMount() {   
    const start = Date.now();

    const promises = [this.fetchReports({}), this.fetchEntities(), this.fetchCodePulseRange()];
    Promise.all(promises)
    .then(() => {
        console.log("Data fetching time: ", Date.now() - start)
        this.setState({ inited: true });
    })
    .catch(error => {
        // ...handle/report error...
    });
}

or

async componentDidMount() {   
    const start = Date.now();

    try {
        const promises = [this.fetchReports({}), this.fetchEntities(), this.fetchCodePulseRange()];
        await Promise.all(promises);
        console.log("Data fetching time: ", Date.now() - start)
        this.setState({ inited: true });
    } catch (error) {
        // ...handle/report error...
    }
}

I have to admit I'm not keen on passing promises to things that don't expect them (such as the React code calling componentDidMount), but it's a relatively minor style thing and I wouldn't call it a "bug" as in the feedback.

Upvotes: 3

Related Questions