Resolving inside a promise catch block

Question

I found this in some code. Is there ever a good reason to do this?

}).catch(() => {
  resolve();
});

I'm curious about whether there are any conceivable ways that this is a good thing to do, but in this case the code I'm reviewing is:

function checkExtendedPackage(config, packagePath) {
  return new Promise((resolve, reject) => {
    extend.check(packagePath)
      .then(() => {
        extend.validate(packagePath, config.allowExtends, config.scope)
          .then(packageToExtend => {
            showOutput.log([{
              type: 'success',
              description: 'validating',
              message: packageToExtend
            }]);
            resolve();
          }).catch(err => {
            reject(err);
          });
      }).catch(() => {
        resolve();
      });
  });
}

Looks like a manifestation of the [explicit promise construction antipattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it), so no, probably not. — JLRishe, Jul 17 '18 at 12:52
We'd need more context, but on the face of it, I'm with @JLRishe. It's *probably* an example of the `new Promise` anti-pattern. (And probably not great error handling, either...) — T.J. Crowder, Jul 17 '18 at 12:56

JLRishe · Answer 1 · 2018-07-17T13:23:51.523

As you seem to have surmised, that's just poorly written code. There's nothing good about the part you initially pointed out and it is indeed an example of the explicit promise construction antipattern. It seems to have been written by someone who didn't really understand how to use promises.

It can be improved like this:

function checkExtendedPackage(config, packagePath) {
    return extend.check(packagePath)
        .then(() => extend.validate(packagePath, config.allowExtends, config.scope)
            .then(packageToExtend => {
                showOutput.log([{
                    type: 'success',
                    description: 'validating',
                    message: packageToExtend
                }]);
            })
        , () => null);
}

As with the original code, this function will return a promise that:

Will not reject if extend.check rejects (resolves to null in that case)
Will reject if extend.validate rejects (with the same error)
Will execute showOutput.log with the necessary values if both succeed.

To reduce the nesting and complexity, I would suggest breaking this out into two functions:

function validateAndLogExtendedPackage(config, packagePath) {
    return extend.validate(packagePath, config.allowExtends, config.scope)
        .then(packageToExtend => {
            showOutput.log([{
                type: 'success',
                description: 'validating',
                message: packageToExtend
            }]);
        });
}

function checkExtendedPackage(config, packagePath) {
    return extend.check(packagePath)
        .then(
            () => validateAndLogExtendedPackage(config, packagePath),
            () => null
        );
}

Don't you need to use `return new Promise` somewhere for it to be thenable? — Ollie Williams, Jul 17 '18 at 13:47
@o-t-w No, as evidenced by the `.then()`s already present in the original code, `extend.check` and `extend.validate` both return thenables. There is no need to redundantly bundle them inside a promise. — JLRishe, Jul 17 '18 at 14:02

Resolving inside a promise catch block

1 Answers1