fileId isn't looping properly

Question

Quick explanation of this block: I have a files object, which is all the files I'm uploading, then I have a signedUrls object which has all of the signed URLs from an earlier S3 function. The objects have matching indexes.

The first axios.put uploads the file, and the second axios.post saves the file key to my DB. (I don't want to save it to my DB unless it was uploaded successfully, hence the axios.post's location in the callback.)

The files are uploading just fine, but the fileId isn't looping properly and typically saves the same fileId over and over again. I.e., if I upload five files, they'll upload to S3, but they'll all have the same id in my DB. Ideas why this is happening?

fileIds = {"1": "someFileId", "2": "someOtherId" }    

for (let i = 0; i < files.length; i++) {
  axios.put(signedUrls[i], files[i], config).then(res => {
    axios.post('https://myapi.com/add-file', {
      fileId: fileIds[i]
    }).then(res => {
      // success
    });

Try adding `const id = fileId` inside the loop and use that instead of `fileId`. Or at least show us where it's assigned. — pishpish, Dec 27 '17 at 22:58
@destoryer see my edit - it works something like this where the file ids are also an object with numbered keys. I over-simplified my original post. — Glenn, Dec 27 '17 at 23:05
Are you running this code in node.js or in a browser? Using `axios` and a separate `post` call to save data to DB look suspicious. — Bsalex, Dec 28 '17 at 00:29

score 1 · Answer 1 · answered Dec 27 '17 at 23:41

It's because you're doing an async call inside of a synchronous for loop.

By the time the post request is invoked, your loop has already finished.

You can use Promise.all to solve this:

const promises = files.map((file, i) => {

  // create a new promise with correct index, but don't call it yet
  return new Promise((resolve, reject) => {
    return axios.put(signedUrls[i], file, config)
    .then(res => {
      return axios.post('https://myapi.com/add-file', {
        fileId: fileIds[i]
      }).then(res => {
        resolve(res)
        // todo: also handle errors here
      })
    })
  })

})

// actually invoke your calls here
Promise.all(promises).then(res => /* success */ )

Essentially, what you'll be doing is creating your promise calls synchronously (but not actually calling them yet) so that you can use the correct indexes, and then you'll use Promise.all to actually invoke the array of promises.

Downvoting - why use `Promise.all` if each promise acts for itself and there is no action afterwards? — pishpish, Dec 28 '17 at 00:09

Bsalex · Answer 2 · 2017-12-28T00:14:07.280

-1

The problem is that i is bound to the same value in your .post section.
To fix this - you could use a self executed anonymous function.
Like this:

for (let i = 0; i < files.length; i++) {
  (function(i) {
    axios.put(signedUrls[i], files[i], config).then(res => {
      axios.post('https://myapi.com/add-file', {
        fileId: fileIds[i]
      }).then(res => {
        // success
      });
  })(i);
}

edited Dec 28 '17 at 00:14

answered Dec 28 '17 at 00:07

Bsalex

2,847
1
15
22

You can't send a number by reference in JavaScript. – pishpish Dec 28 '17 at 00:09
@destoryer https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example – Bsalex Dec 28 '17 at 00:11
If you actually read the answers to the question, I'm sure you'd find that using `let` actually scopes the variable to each iteration. – pishpish Dec 28 '17 at 00:14
@destoryer If you actually read the answers to the question, I'm sure you'd find that using `let` in IE9-IE11 and Edge prior to Edge 14 will not help. – Bsalex Dec 28 '17 at 00:19
I appreciate the information, I had no idea Edge failed to implement correcly. In either case, the op is using node, not IE. – pishpish Dec 28 '17 at 00:22

fileId isn't looping properly

2 Answers2