How to remove the following promise catches without making the code hang?

Question

The following code loops through some form fields. If the field is a file that has to be uploaded it runs an api.uploadPhotofunction (setting the payload once the photos has been uploaded). If the field is a normal input when the payload is set directly:

formFields.forEach(field => {
  if (hasUploadFiles(field)) {
    uploadPhotoPromise = new Promise((resolve, reject) => {
      uploads.queued.push(file)
      api.uploadPhoto(file, field).then(uploadedPhoto => {
        uploads.finished.push(field)
        if (uploads.queued.length === uploads.finished.length) {
          payload[field.name] = uploadedPhoto
          resolve()
        } else {
          reject()
        }
      }).catch(error => {
        console.log('error:', error)
        reject()
      })
    }).catch(error => {
      console.log('error:', error)
    })
  } else {
    payload[field.name] = field.value
  }
})

Promise.all([uploadPhotoPromise]).then(values => {
  // update action
}

The code works. However, all those catch make it look a bit messy.

I tried removed them but the code hangs if I remove any of them (the code inside Promise.all never runs). Why is this? And how to refactor this code without all those catch statements without making the it hang?

Original code (plus Bergi's suggested modification):

const buildingFormPromise = utils.mapDeep(this.buildingForm.schema, field => {
  if (!field.name) return // fields not in the database
  else if (utils.hasUploadFiles(field)) {
    utils.eachCall(field.value, (file, index) => {
      field.userId = this.user.id
      this.uploads.queued.push(file)
      this.$set(this.uploads.queued, index, { progress: 30 })
      return api.uploadPhoto(file, field).then(uploadedPhoto => {
        this.$set(this.uploads.queued, index, { progress: 100 })
        return loadImage(uploadedPhoto, () => {
          this.uploads.finished.push(field)
          if (this.uploads.queued.length === this.uploads.finished.length) {
            console.log('This runs after the code inside Promise.all')
            buildingPayload[field.name] = uploadedPhoto
          }
        })
      })
    })
  } else {
    return Promise.resolve(buildingPayload[field.name] = field.value)
  }
})

Promise.all([buildingFormPromise]).then(values => {
  console.log('This runs before the files are uploaded')
})

Two things: 1) you don't create an array of Promises. You just have one `uploadPhotoPromise` over and over on each iteration. 2) You can't call `resolve()` in the first `catch()` since you don't have access to this function and you're outside the promise. This code most likely works because it doens't even get inside the `catch`, otherwise that's not how you use Promises. — vassiliskrikonis, Jun 15 '17 at 07:52
There's more wrong with this code than the unnecessary `catch`es. I don't think that `Promise.all` with `uploadPhotoPromise` will work, the queuing logic is very fishy (it's not a queue, but it doesn't work in parallel either), and you use the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it). What do you want the code to actually do with the `formFields`? — Bergi, Jun 15 '17 at 07:52
@vassiliskrikonis Actually the `reject` in the catch is in scope, that's not a problem — Bergi, Jun 15 '17 at 07:56
@Bergi just like my explanation: I want the code to wait for everything to be done (uploaded images and payload set) before triggering the update action. — alex, Jun 15 '17 at 08:11
@alex So you don't care whether they upload concurrently or sequentially, and you don't care about updating `uploads.queued` and `uploads.finished`? — Bergi, Jun 15 '17 at 08:15
@Bergi Yes, I don't care. And I'm only using `uploads.queued` and `uploads.finished` to know when I should trigger the update action. — alex, Jun 15 '17 at 08:27
@alex Ok, that attempt definitely failed. You currently are rejecting the promise when one file but not the last one are done. Gonna rewrite it in an answer. — Bergi, Jun 15 '17 at 08:29
@Bergi I see, could you give me a sample code that fixes that problem? — alex, Jun 15 '17 at 08:30

Bergi · Answer 1 · 2017-06-15T08:41:42.680

1

You need to pass an array of all the promises into Promise.all, and you should avoid the Promise constructor antipattern. You can move the .catch to the very end if you don't want to handle individual upload failures.

var fieldValuePromises = formFields.map(field => {
  if (hasUploadFiles(field)) {
    return api.uploadPhoto(file, field).then(uploadedPhoto => {
      return payload[field.name] = uploadedPhoto;
    });
  } else {
    return Promise.resolve(payload[field.name] = field.value);
  }
});

Promise.all(fieldValuePromises).then(values => {
  // update action
}).catch(error => {
  // at least one upload failed
  console.log('error:', error)
});

edited Jun 15 '17 at 08:41

answered Jun 15 '17 at 08:33

Bergi

630,263
148
957
1,375

Thanks I try it out! One question: so only the else statement need a `Promise.resolve`? What happens if all the fields are upload fields? – alex Jun 15 '17 at 08:36
Yes, the if case already returns a promise, the result of chaining `then()` to `uploadPhoto()`. Strictly the promise in the else case isn't necessary, as `Promise.all` does resolve plain values as well, but it's better to have the `fieldValuePromises` contain only promises. – Bergi Jun 15 '17 at 08:41
Weird, `Promise.all` runs before the files are uploaded. (I included the original code in my question). – alex Jun 15 '17 at 08:59
Well, you don't collect your promises in `fieldValuePromises`, so `Promise.all` cannot await them… What is that `utils.eachCall` doing in there?! And what is `loadImage` doing with that callback, if it's asynchronous why does it not return a promise? – Bergi Jun 15 '17 at 09:04
Here's a link with the relevant functions: https://gist.github.com/alexcheninfo/2660092edc1816854cd27e672341e3e0 (Thanks for your the time.) – alex Jun 15 '17 at 09:08
So how should I modify the code to address this issue? – alex Jun 15 '17 at 09:09
Make a `const buildingFormPromises = []`, and get your deep iteration to (immediately, before calling `Promise.all`) `push` all promises to it that you want to have awaited. – Bergi Jun 15 '17 at 09:13
If you also want the `loadImage` to be awaited, then [promisify](https://stackoverflow.com/q/22519784/1048572) it. – Bergi Jun 15 '17 at 09:15

How to remove the following promise catches without making the code hang?

1 Answers1