I can't seem to figure out why the third function runs before the other two even though I am using Promises. What am I doing wrong?

Question

When I console.log this.state.events and this.state.meets the terminal logs that these two states are empty arrays. This suggests that this function executes before function 2 and 3 finish and that I am doing my promises wrong. I can't seem to figure out my issue though and I am very confused. I know that the setState is working as well since once the page renders I have a button whose function console.logs those states and it executes properly.

I have tried formatting my promises in various ways but I have no idea how they resolve before my functions finish.

obtainEventsAndMeetsRefs = () => {
  return Promise.resolve(
   this.userRef.get().then(
     doc => {
       this.setState(
         {
           eventRefs: doc.data().Events,
           meetingsRef: doc.data().Meets
         }
       )
     }
   )
 )
}

obtainEvents() {

var that = this

return new Promise(function (resolve, reject) {
  for (i = 0; i < that.state.eventRefs.length; i++) {

    docRef = that.state.eventRefs[i]._documentPath._parts[1];
    firebase.firestore().collection('All Events').doc(docRef).get().then(
      doc => {
        that.setState({
          events: update(that.state.events, {
            $push: [
              {
                eventName: doc.data().eventName,
                eventDescription: doc.data().eventDescription,
                startDate: doc.data().startDate,
                endDate: doc.data().endDate,
                location: doc.data().location,
                privacy: doc.data().privacy,
                status: doc.data().status,
                attending: doc.data().attending
              }
            ]
          })
        })
      }
    )

    if (i == that.state.eventRefs.length - 1) {
      console.log('1')
      resolve(true)
    }
  }
})
}

obtainMeetings() {

var that = this

return new Promise(function (resolve, reject) {

  for (i = 0; i < that.state.meetingsRef.length; i++) {

    userRef = that.state.meetingsRef[i]._documentPath._parts[1];
    meetRef = that.state.meetingsRef[i]._documentPath._parts[3];
    ref = firebase.firestore().collection('Users').doc(userRef)
      .collection('Meets').doc(meetRef)

    ref.get().then(
      doc => {

        that.setState({
          meets: update(that.state.meets, {
            $push: [
              {
                headline: doc.data().headline,
                agenda: doc.data().agenda,
                startTime: doc.data().startTime,
                endTime: doc.data().endTime,
                date: doc.data().date,
                name: doc.data().name
              }
            ]
          })
        })

      }
    )
    if (i == that.state.meetingsRef.length - 1) {
      console.log('2')
      resolve(true)
    }
  }
})
}

orderByDate = () => {
  console.log(this.state.events)
  console.log(this.state.meets)
}

componentDidMount() {
  this.obtainEventsAndMeetsRefs().then(
    () => this.obtainEvents().then(
      () => this.obtainMeetings().then(
        () => this.orderByDate()
      )
    )
  )
}

I expected the output to be an array filled with data that I inputted using setState, not an empty array.

I apologize I meant why does the fourth function execute before functions two and three. — Kurnal saini, Jul 19 '19 at 18:14
How are you counting? Can you please be more descriptive of exactly what runs before something else? — jfriend00, Jul 19 '19 at 18:16
You are resolving `obtainEvents` at a rather weird place. In it's current place, you mightaswell remove the conditional and put it after the loop. Note however that it isn't going to wait for all of those async actions inside the loop to complete, not with the conditional and not without it. — Kevin B, Jul 19 '19 at 18:16
Because you're using a loop to do further async operations which would be much better done with a `Promise.all`. The loop finishes and the promise resolved before those operations have completed. — Andy, Jul 19 '19 at 18:19
Inside the loop, do you mean to run those operations serially (one after the other where the 2nd iteration of the loop waits for the first) or do you just want to run them all in parallel and know when they are all done. It's a different design depending upon the goal. — jfriend00, Jul 19 '19 at 18:21
orderDates() executes before obtainEvents() and obtainMeetings() @jfriend00 — Kurnal saini, Jul 19 '19 at 18:21
i would like to run orderEvents and orderMeetings in parallel I suppose and then know when they are finished for orderDates() — Kurnal saini, Jul 19 '19 at 18:23
OH wait I understand. how do I make the loop stop until the setState function completes? — Kurnal saini, Jul 19 '19 at 18:24
For starters, avoid the [promise construction anti-pattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it). — jfriend00, Jul 19 '19 at 18:25
You would make the function be `async` and use `await` on a promise inside the `for` loop. — jfriend00, Jul 19 '19 at 18:25
And, you would stop wrapping everything in it's own `new Promise()`. Just use the promises you already have. Pardon me for saying so, but you need to do a lot more learning about how to properly use and return the promises you already have and stop making your own `new Promise()` wrappers. The only time you should be making a `new Promise()` wrapper is when you have an async operation that does not return a promise. In that case, you should promisify it outside of the use and use the promisified version. — jfriend00, Jul 19 '19 at 18:28

jfriend00 · Answer 1 · 2019-07-19T20:25:19.840

The crux of the issue is that obtainMeetings() and obtainEvents() don't return promises that resolve when their asynchronous operations are done. Thus, you can't know when their work is done and thus you can't sequence them properly.

There are all sorts of things wrong in this code. To start with, you don't need to wrap existing promises in another promise (the promise construction anti-pattern) because that leads to errors (which you are making). Instead, you can just return the existing promise.

For example, change this:

obtainEventsAndMeetsRefs = () => {
  return Promise.resolve(
   this.userRef.get().then(
     doc => {
       this.setState(
         {
           eventRefs: doc.data().Events,
           meetingsRef: doc.data().Meets
         }
       )
     }
   )
 )
}

to this:

obtainEventsAndMeetsRefs = () => {
   return this.userRef.get().then(doc => {
       this.setState({
           eventRefs: doc.data().Events,
           meetingsRef: doc.data().Meets
       });
   });
}

Then, chain rather than nest and return a promise that is linked to the completion/error of the operations.

So change this:

componentDidMount() {
  this.obtainEventsAndMeetsRefs().then(
    () => this.obtainEvents().then(
      () => this.obtainMeetings().then(
        () => this.orderByDate()
      )
    )
  )
}

to this:

componentDidMount() {
  return this.obtainEventsAndMeetsRefs()
    .then(this.obtainEvents.bind(this))
    .then(this.obtainMeetings.bind(this))
    .then(this.orderByDate.bind(this))
}

or if you prefer:

componentDidMount() {
  return this.obtainEventsAndMeetsRefs()
    .then(() => this.obtainEvents())
    .then(() => this.obtainMeetings())
    .then(() => this.orderByDate())
}

These chain instead of nest and they return a promise that tracks the whole chain so that caller knows when the chain completed and/or if there was an error.

Then, if you want your for loop to execute serially, you can make it async and use await on the promise inside the loop like this:

async obtainMeetings() {

    for (let i = 0; i < this.state.meetingsRef.length; i++) {

        let userRef = this.state.meetingsRef[i]._documentPath._parts[1];
        let meetRef = this.state.meetingsRef[i]._documentPath._parts[3];
        let ref = firebase.firestore().collection('Users').doc(userRef)
            .collection('Meets').doc(meetRef)

        // make the for loop wait for this operation to complete
        await ref.get().then(doc => {
            this.setState({
                meets: update(this.state.meets, {
                    $push: [{
                        headline: doc.data().headline,
                        agenda: doc.data().agenda,
                        startTime: doc.data().startTime,
                        endTime: doc.data().endTime,
                        date: doc.data().date,
                        name: doc.data().name
                    }]
                });
            });
        });
        // this return here isn't making a whole lot of sense
        // as your for loop already stops things when this index reaches
        // the end value
        if (i == this.state.meetingsRef.length - 1) {
            console.log('2')
            return true;
        }
    }
    // it seems like you probably want to have a return value here
    return true;
}

This also looks like there's a problem with the missing declarations of i, userRef, meetRef and ref in your implementation. Those should be locally declared variables.

obtainEvents() needs the same redesign.

If you can't use async/await, then you can transpile or you'd have to design the loop to work differently (more of a manual asynchronous iteration).

If you can run all the async operations in the loop in parallel and just want to know when they are all done, you can do something like this:

obtainMeetings() {

    return Promise.all(this.state.meetingsRef.map(item => {
        let userRef = item._documentPath._parts[1];
        let meetRef = item._documentPath._parts[3];
        let ref = firebase.firestore().collection('Users').doc(userRef)
            .collection('Meets').doc(meetRef)

        // make the for loop wait for this operation to complete
        return ref.get().then(doc => {
            this.setState({
                meets: update(this.state.meets, {
                    $push: [{
                        headline: doc.data().headline,
                        agenda: doc.data().agenda,
                        startTime: doc.data().startTime,
                        endTime: doc.data().endTime,
                        date: doc.data().date,
                        name: doc.data().name
                    }]
                });
            });
        });

    })).then(allResults => {
        // process all results, decide what the resolved value of the promise should be here
        return true;
    });
}

`this.orderByDate().bind(this)` will fail as it should be binding the function itself: `this.orderByDate.bind(this)`. Other than that, it wouldn't be necessary if OP used class properties for all the callbacks. It could then be just `.then(this.orderByDate)` since the params are unused anyway. — Emile Bergeron, Jul 19 '19 at 19:05
@EmileBergeron - Corrected the `.bind()` error. I didn't recommend `.then(this.orderByDate)` because I assumed that `this` needs to be correct inside the functions (which it does for some of them) and this wouldn't do that. The OP doesn't show to overall class definition to see how that could be restructured or not. — jfriend00, Jul 19 '19 at 19:12
It would have the right context since (if) OP used arrow function in class properties like he did with `orderByDate = () =>`. — Emile Bergeron, Jul 19 '19 at 19:21
Though I wouldn't recommend it either since it could lead to bugs if OP added params and reused these functions. I'd go with `() => this.callback()` most of the time. — Emile Bergeron, Jul 19 '19 at 19:31

I can't seem to figure out why the third function runs before the other two even though I am using Promises. What am I doing wrong?

1 Answers1