Code works, but is it best practice to have a try/catch block inside a promise?

Question

Is it good practice to wrap a try/catch with a Promise? I was reviewing the code below, I cannot seem to understand the necessity of having a try/catch block inside a Promise. Also, why would there be no place where the Promise rejects, as you can see in the catch block, the Promise resolves. Please help me understand this piece of code, in terms of best practice and efficiency the point of having the try/catch inside the Promise.

I would also really appreciate any suggestions regarding cleaning the code up where it is redundant.

getRoute(): any {
    return async (request: any, reply: any) => {
      const result = new Promise<string>( async (resolve, reject) => {
        try {
          let userIsValid = await this.validator.validate(request.payload, RegisterUserValidator);
          if (userIsValid.error) {
            resolve(responseHelper.getErrorResponse(ResponseErrorCode.joiValidatorError, userIsValid.error));
            throw new ControlFlowNonError();
          }
          let results = await this.authUser.registerUser(request.payload);
          resolve(responseHelper.getSuccessResponse(results, null));
        }
        catch (error) {
          let message: ILogMessage = {
            code: ResponseErrorCode.unknownError,
            message: ResponseErrorCode.unknownError.toString(),
            meta: error,
            sourceFunction : 'ApiDataCreateCredentials: getRoute()'
          };
          this.logHelper.error(message);
          resolve(error);
        }
      });
      reply(result);
    };
  };

Answer 1

Is it best practice to have a try/catch block inside a promise [executor function]?

No, it's not best practice. There's pretty much no reason to use promises inside a promise executor function. Because when you're doing that, you don't need the outer, manually created promise at all. You can just return the inner promise. That's the Promise constructor anti-pattern.

FYI, though it isn't your case here, it is reasonable to use try/catch to handle regular exceptions inside a promise executor (if the manually created promise executor is actually needed in the first place and if regular exceptions are of concern and you want to handle them locally).

---

Here's another idea for how to accomplish your same logic. This removes the promise anti-pattern of surrounding a promise with another manually created one and uses promise flow control to make userIsValid.error go to the log error code. I didn't see any particular advantage of using await here so I switched back to just using .then() and .catch(). I don't know TypeScript so this is a regular Javascript version of the code, but you can presumably add the slight syntax differences yourself to turn it back to TypeScript:

getRoute(): function() {
    return function(request, reply) {
        return this.validator.validate(request.payload, RegisterUserValidator).then(userIsValid => {
            if (userIsValid.error) {
                // treat this as an error condition
                throw responseHelper.getErrorResponse(ResponseErrorCode.joiValidatorError, userIsValid.error);
            }
            return this.authUser.registerUser(request.payload).then(results => responseHelper.getSuccessResponse(results, null));
        }).catch(error => {
          let message: ILogMessage = {
            code: ResponseErrorCode.unknownError,
            message: ResponseErrorCode.unknownError.toString(),
            meta: error,
            sourceFunction : 'ApiDataCreateCredentials: getRoute()'
          };
          this.logHelper.error(message);
          // turn back into resolved promise with error as the result
          return error;
        }).then(reply);        // always call reply
    }
}    

Things that did not seem ideal with your implementation:

1. Promise anti-pattern (creating an unnecessary wrapper promise).
2. Calling resolve() multiple times (the second one will be ignored, but it seems less than ideal to code it that way)
3. There does not seem to be any particular benefit to using async/await here. It seems to complicate the flow (in my opinion).
4. resolve() followed by throw is a real head scratcher when reading the code. Yes, one can eventually figure out what you're trying to do, but this is an odd way to do it. My scheme is a bit more semantic. if (userIsValid.error) is really just an error condition for you so coding it this way just makes that more obvious. Then, since you want ALL errors to proceed as if there was no error (after logging), we just make the .catch() handler return normally which allows the last .then() handler to always get called.

Other references related to your topic:

Can't throw error from within an async promise executor function

Is it an anti-pattern to use async/await inside of a new Promise() constructor?