Promise.all().then() goes to catch() because then is not recognized

Question

The code is running well until the Promise.all and then is goes right to the catch saying 'then is not defined'.

I've been trying to figure this out without success for hours :(.

Any help are welcome.

Here is a simplified example of code:

// Save
return new Promise((fulfillSave, rejectSave) => {
  // Get
  this._getObjects().then((object) => {
    var promises = [];

    // For each value
    object.values.forEach((value) => {
        promises.push(
          // Create promise
          new Promise((fulfill, reject) => {
            // Create MDB object + assign value
            valueMongoDB.value = value; 
            // Save
            valueMongoDB.save((err, results) => {
              if (err) {
                reject('Error in saving');
              } else {
                fulfill();
              }
            });
          })
        );
      });

      // Wait for all promises
      Promise.all(promises).then(() => {
        // Nothing to do
        fulfillSave();
      }, then((err) => {
        // Err
        rejectSave(err);
      }));
    }
  }).catch((err) => {
    rejectSave(`Error: ${err.message}`);
  });
});

Thanks in advance! Serge.

Well, on a small level you don't want `then((err)=>{..})` - you just want `(err)=>{...}`. But your bigger problem is the nesting of promises, which is not necessary. — lonesomeday, Apr 03 '17 at 17:46
Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it) - mongodb already returns promises when you don't pass a callback! — Bergi, Apr 03 '17 at 19:01
And avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it) in the overall save as well! — Bergi, Apr 03 '17 at 19:02

T.J. Crowder · Accepted Answer · 2017-04-03T18:08:00.707

This is incorrect:

// Wait for all promises
Promise.all(promises).then(() => {
  // Nothing to do
  fulfillSave();
}, then((err) => {
// ^^^^--------------------------- error here
  // Err
  rejectSave(err);
}));

It's trying to call a freestanding function called then and pass its return value into the then on the object returned by Promise.all.

I think you're trying to hook up a failure handler. If so, you don't say then, you just supply a second function:

Promise.all(promises).then(() => {
  // Nothing to do
  fulfillSave();
}, (err) => {
  // Err
  rejectSave(err);
}));

But of course, since you're not using the result of that chain and you're just passing the single argument your second function receives into rejectSave, you could just pass rejectSave directly:

Promise.all(promises).then(() => {
  // Nothing to do
  fulfillSave();
}, rejectSave);

If you told us what your overall code is meant to do and what its inputs are, my suspicion is that that code could be a lot simpler. It's common to create and nest promises unnecessarily, and I suspect that's happening here.

For instance, if you just want to do the saves and get back a promise that will resolve when they're all done successfully or reject on the first failure:

return this._getObjects()
  .then(objects => Promise.all(objects.map(value => {
      return new Promise((resolve, reject) => {
        // Create MDB object + assign value
        valueMongoDB.value = value; 
        // Save
        valueMongoDB.save((err, results) => {
          if (err) {
            reject('Error in saving');
          } else {
            fulfill();
          }
        });
      });
    })));

Or if we give ourselves a helper function for the Mongo bit:

function mongoSavePromise(value) {
  return new Promise((resolve, reject) => {
    // Create MDB object + assign value
    valueMongoDB.value = value; 
    // Save
    valueMongoDB.save((err, results) => {
      if (err) {
        reject('Error in saving');
      } else {
        fulfill();
      }
    });
  });
}

then:

return this._getObjects()
  .then(objects => Promise.all(objects.map(mongoSavePromise)));

Thanks a lot, this was the issue! I focused on the first 'then' and I didn't see the issue :( — LucasBrazi06, Apr 04 '17 at 22:01
Thanks also for the optimization of the code! This is always nice to get a better way of doing things! I'll apply this with the real code and see as I also like to keep it readable even if it's more code lines :-). — LucasBrazi06, Apr 04 '17 at 22:06
Just implemented it and it's readable and less code! Thanks for the gold bar advise :-) ! — LucasBrazi06, Apr 04 '17 at 22:36
I noticed that when returning a value like below the array was always returned instead the single Object. And the only way to solve this is to surround this code with 'new Promise" Any clue? return MDBObject.find({}, (err, objects) => { var object = objects[0]; return object; } Thanks. — LucasBrazi06, Apr 04 '17 at 23:47
@Serge: I have no idea what `MDNObject.find` does (other than a reasonable guess from the name), so probably can't help... — T.J. Crowder, Apr 05 '17 at 06:47

score 1 · Answer 2 · edited May 23 '17 at 12:02

1

Avoid the Promise constructor antipattern!

Your whole code should be a simple

return this._getObjects().then(object => {
    var promises = object.values.map(value => {
        // Create MDB object + assign value
        valueMongoDB.value = value; 
        // Save
        return valueMongoDB.save().catch(err => {
            throw 'Error in saving';
        });
    });
    // Wait for all promises
    return Promise.all(promises);
}, err => {
    throw `Error: ${err.message}`;
});

No unnecessary callbacks, no room for mistakes. Btw, you shouldn't throw strings.

edited May 23 '17 at 12:02

Community

1
1

answered Apr 03 '17 at 19:08

Bergi

630,263
148
957
1,375

Thanks for the advise, I'll try the 'map' and reduce the Promises. I already removed the usage of string in exceptions. But here you need a 'catch' on the other side instead the 'reject' function. Need to add it :-(. No quite simple to deal with Promises ! – LucasBrazi06 Apr 04 '17 at 22:10
What do you mean by "on the other side"? In general, you don't need any `catch` if you don't want to handle an error – Bergi Apr 04 '17 at 22:20
Maybe you know: I noticed that when returning a value like below the array was always returned instead the single Object. And the only way to solve this is to surround this code with 'new Promise" Any clue? return MDBObject.find({}, (err, objects) => { var object = objects[0]; return object; } Thanks. – LucasBrazi06 Apr 05 '17 at 00:13
Often enough you want to let your caller handle the errors and just `return` the promise - of course, at the end of a chain you always should handle any errors. – Bergi Apr 05 '17 at 04:40
You should not pass a callback to `find`. Just get the promise and chain a `then` to it: `MDBObject.find({}).then(objects => objects[0])` – Bergi Apr 05 '17 at 04:40
Perfect, I adapted the code ! Much clearer ! Thanks for all your advices! – LucasBrazi06 Apr 05 '17 at 22:49

Promise.all().then() goes to catch() because then is not recognized

2 Answers2