Refactor superfluous `Promise.resolve(value)` and `Promise.reject(error)` in `.then()` functions.

[martinlingstuyl]

There are quite some commands where values are returned from resolved promises using a Promise.resolve(value) call. The same goes for Throwing errors from within resolved Promises. This is often done using a returned Promise.reject(error).

Take the following code:

return request.get(requestOptions)
   .then((response) => {


      if(someCondition) {
         return Promise.reject("something went wrong@);
      }

      return Promise.resolve(response.property);

   });

This works fine and is okay, but it's also more verbose than simply writing return response.property; , and regarding errors wouldn't it be clearer to write throw new Error("something went wrong");?

Should we refactor this?

[milanholemans]

I'm in favor of this. What you are proposing is much cleaner in my opinion. Maybe we can make these functions async as well? But that is somewhat related to #2130. ☺️

[martinlingstuyl]

I'd really love that as well 😁

[martinlingstuyl]

@milanholemans, I wonder, will this issue be covered by your changes in #3430 by any change?

[milanholemans]

@milanholemans, I wonder, will this issue be covered by your changes in #3430 by any change?

Hmm not really. Only thing I refactor is the commandAction function. I leave all the other functions untouched because these will still work and can be refactored later on. So in the commandAction error throwing is used. In the other functions not yet.

I definitely support the idea to refactor it all, but maybe this can be done together when we refactor the remaining functions to async/await? (https://github.com/pnp/cli-microsoft365/issues/3430#issuecomment-1186592833)

[waldekmastykarz]

Throwing an exception inside a promise seems to me more counterintuitive than returning a rejected promise. If I see an exception being thrown in the promise, I immediately wonder what's needed to properly catch it. I think it's perfectly fine to move to exceptions when we move to async/await but as long we use Promises, I'd suggest that we won't introduce throwing exceptions to avoid additional complexity.

[martinlingstuyl]

Agreed, but I think it's best to close this one in favor of a separate issue for refactoring to Async/await @waldekmastykarz, as we're moving in that direction anyway, it won't matter if there are a few Promises returned for now.

[martinlingstuyl]

Referencing https://github.com/pnp/cli-microsoft365/issues/3618