WebAPICallResult and the error handling is not intuitive

[lenkan]

Description

This is not really a bug, but rather a feature of the API that caught me off guard. I think there is room for improvement for how errors are propagated back to the user from the WebClient. See "Steps to reproduce" for an explanation of what surprised me.

What type of issue is this? (place an x in one of the [ ])

• bug
• enhancement (feature request)
• question
• documentation related
• testing related
• discussion

Requirements (place an x in each of the [ ])

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Packages:

Select all that apply:

• @slack/web-api
• @slack/events-api
• @slack/interactive-messages
• @slack/rtm-api
• @slack/webhooks
• I don't know

Reproducible in:

package version: 4.12.0

node version: 10.15.3

OS version(s): Arch Linux 5.0.7

Steps to reproduce:

1. Call users.lookupByEmail for a team where the email is not registered.

const result = await client.users.lookupByEmail({ email: '[email protected]' })

Expected result:

Since the interface WepAPICallResult has the following shape, in particular the error and ok properties:

export interface WebAPICallResult {
  ok: boolean;
  error?: string;
  response_metadata?: {
    warnings?: string[];
    next_cursor?: string;
    scopes?: string[];
    acceptedScopes?: string[];
    retryAfter?: number;
  };
  [key: string]: unknown;
}

I expect that the call to lookupByEmail should resolve to an object similar to:

{
   error: 'users_not_found',
   ok: false,
   response_metadata: [...]
}

I.e., the error property indicates that there was an error executing the particular lookup.

Actual result:

The call rejected to an Error object containing:

{ 
  message: 'An API error occurred: users_not_found',
  code: 'slackclient_platform_error',
  data: { 
     ok: false,
     error: 'users_not_found',
  } 
}

Discussion:

I believe the simplest solution would be to simply not include error in the interface WebAPICallResult. This would change the public interface as it is communicated, but not how it actually works (?).

As far as I can see from the source, the returned value never specifies a value for this property. When the client receives a non-OK HTTP response, it simply wraps this into an Error and throw instead. Please correct me if I am mistaking.

With that being said, I do not really see the value of rejecting the API-call unless there actually is an unexpected Error, such as a network error or a bug. That way, we can simply check the response.error or response.ok property in our application code and act accordingly. But that would obviously be a breaking change.

[aoberoi]

hi @lenkan, thanks for your keen insights, and taking the time to write them up for us to benefit.

As far as I can see from the source, the returned value never specifies a value for this property. When the client receives a non-OK HTTP response, it simply wraps this into an Error and throw instead. Please correct me if I am mistaking.

While this is true in success cases, that's not the only place where WebAPICallResult is used. Lets take a closer look at the Error you provided, and the interface which defines it:

node-slack-sdk/packages/web-api/src/errors.ts

Lines 24 to 29 in e754103

	export interface WebAPIPlatformError extends CodedError {
	code: ErrorCode.PlatformError;
	data: WebAPICallResult & {
	error: string;
	};
	}

The relevant part is that the data property is a WebAPICallResult. The only reason the error you provided has ok: false, error: 'users_not_found' included in that property is because of this fact. You'll also see that its intersected (&) with an object that has an error: string; property. That's is to let the typechecker know that while in general the error property is optional, when it's used in a WebAPIPlatformError, it will always be a string.

This structure was chosen because its possible that while Slack is telling you the request was not successful, there may be more data available in the body to tell you why it wasn't successful. It's important that we give you a way to access that the data Slack has sent back, even when there's an error; hence, the data property.

While your observation is true, that the error property isn't populated when the call succeeds, it doesn't make it any less true that this property is in fact optional. I cannot think of a situation where leaving the optional property there gets in the way of something you want to do. If I were to redesign this interface, I might express the success cases and error cases as two separate interfaces, or even better, use one interface with a generic parameter to express that there remains a relationship. However, I can't think of a good reason to add that complexity now. The fact that error is optional is useful at least for the implementation, if not valuable to the public access. But since it's not getting in anyone's way, as far as I can tell, that seems to be an okay tradeoff. Can you help me see why this would trip someone up?

I expect that the call to lookupByEmail should resolve to an object similar to:

This is an interesting point of view. At a coarse level, the assumption is that most developers care about whether something succeeded or not, which Promises are good at expressing as resolve or reject. We've taken the design approach to separate the reasons something didn't succeed using the code property, which you can "look up" using the ErrorCode dictionary. I believe it would be counter intuitive to have to check every resolved Promise once more for whether the result.ok was truthy to determine if the call succeeded. I think that would result in lots of if checks, that essentially places developers back in the era of callbacks. Are there advantages to this approach that I'm missing? Are there disadvantages to using the ErrorCode dictionary to differentiate a Platform error (like users_not_found) from a "protocol" error (like the non-OK HTTP response, or even a failed request)?

[lenkan]

Thanks for your comments. I can see your point of view more clearly.

I cannot think of a situation where leaving the optional property there gets in the way of something you want to do.

Let me first say that this is not a big issue at all. It does not "get in the way", it is just unexpected to me, it guides me to implement code that does not work as expected. I will try to explain with an example.

I am new to using the Slack API, and had implemented an application to simply post notifications to a particular channel. Essentially something like this:

async function postMessage(text) {
   const result = await client.chat.postMessage({ channel, text })
   // ...
}

Here I see that result is a WebAPICallResult that has an ok and an error property (albeit optional). This indicates to me that the error property is set when there was an error processing the request. In other words, I expect it to be set to a value when the API returns with a non-OK status code.

I also expect this value to be one of the values as indicated by the API documentation. So, I might go ahead and do something like this:

async function postMessage(text) {
  const result = await client.chat.postMessage({ channel, text })
  
  if(result.error) {
     throw new Error(result.error)
  }
  
  // Not really interested in the response if it was OK, so no return
}

Or I might go ahead and do this if I really don't care if the posting succeeds.

async function postMessage(text) {
  const result = await client.chat.postMessage({ channel, text })
  
  if(result.error) {
     console.warn(`Posting failed with ${result.error}`)
  }
}

The important point being that the interface of the return value communicates that the error-property will be set when there was an error. Which makes me think that I am supposed to handle error cases on my end like the above. So this is what I did, and I had this type of code running.

Obviously the code is simply redundant, because there are no circumstances where the error property is set. This should have been communicated through the return type. If I were to use typescript (or something like "// @ts-check"), it should have told me that WebAPICallResult does not contain a property error.

In the above example, I didn't really need to handle different error cases. It is true that in most use cases, all errors are handled equally, by throwing. Therefore I didn't really pay much attention at this point, it simply threw anyways.

One week later I decided to also do a users.lookupByEmail: If the user was registered in the team, I would mention that user in the message, otherwise just mention the provided email. I was still under the impression that the result object would indicate if there was an error. So I wrote a method like this to gracefully be able to fall back to posting the users email if not registered with the team.

async function findUserId(email) {
  const result = await client.users.lookupByEmail({ email })
  
  if(result.error === 'users_not_found') {
     return undefined
  }

  if(result.error) { // Other errors such as auth-fail or improper use of api -> throw
     throw new Error(result.error)
  }

  return result.user.id
}

In my opinion, this sort of error handling can be quite powerful, especially in typescript. You could explicitly type each methods possible error values. Which makes it dead-easy to explore the API using an intellisense enabled editor. From there, I can investigate which error cases that I should handle, and which cases I should simply throw. There might be some if-statements, that is true, but in the simplest of cases, it would be handled with an assertSuccess type of function, effectively only introducing if's for when it is actually needed.

Let's look at how the above error must be handled currently instead:

async function findUser (email) {
  try {
    const result = await client.users.lookupByEmail({ email })
    return result.user.id
  } catch (error) {
    if (error.code === 'slack_webapi_platform_error') {
      if(error.data && error.data.error === 'users_not_found') {
        return undefined
      }
    }

    throw error
  }
}

I find this way of handling the error cases to be much more verbose and error prone. I do not automatically get typings for a caught error so I cannot explore the API to dig out which error I can handle and so on.

---

I guess my point is two-fold. I do like what the interface WebAPICallResult is communicating, because it encourages me to think about the possible error cases returned from the Web API. The problem is that it is not correct. It should not indicate to me that it has an optional error property if it is not being used.

From your explanation I do see that the property is used internally, in the typing of the error. So yes, perhaps it should simply be separated.

That's not the only place where WebAPICallResult is used.

Maybe this is the issue, it would be more communicative to "rebrand" this interface without the error property before being used in the public API as a return value. An interface like this:

export interface WebAPIResult {
  response_metadata?: {
    warnings?: string[];
    next_cursor?: string;
    scopes?: string[];
    acceptedScopes?: string[];
    retryAfter?: number;
  };
  [key: string]: unknown;
}

would communicate to me that the error is handled internally, and when this function returns a promise that resolves, it was successful. Therefore I would immediately handle the error correctly.

---

In an ideal world, I would expect a return value similar to this:

interface ResponseMetadata {
  warnings?: string[];
  next_cursor?: string;
  scopes?: string[];
  acceptedScopes?: string[];
  retryAfter?: number;
}

interface SuccessResult<TData> {
  ok: true
  data: TData
  response_metadata: ResponseMetadata
}

interface ErrorResult<TError> {
  error: TError
  ok: false
  response_metadata: ResponseMetadata
}

export type WebAPICallResult<TData, TError> = SuccessResult<TData> | ErrorResult<TError>

where a set error and ok: true simply cannot co-exist.

Edit: Which is what you mentioned too if you were to redesign it.

[clavin]

Just to echo what was already said (and somewhat agreed on), imagine seeing this in your editor:

Seeing this for the first time (or even just looking past it for the 100th time), the word "error" just jumps out at me.

🤔 From a strict perspective, the current exposed type of apiCall is simply just wrong. Unless you can come up with an intended case of ok: true, error: string, any current outside dev code that points to the definition of error in WebAPICallResult is always incorrect code.

• Check error from a successful API call? Always wrong because the resolved value will always be ok: true and have no error.
• Check error while paginating? Always wrong for the same reason as above.

Overloads like WebAPICallResult & { error: string; } (an "error result") are excluded because their definition of error doesn't point to the one defined in the interface. Any case where you have an error result, you have an error. Any case you don't have an error result, you don't have an error.

...Yet the exported type for apiCall implies otherwise. This is misleading. It adds ambiguity where there should be none. 🙁

It's true, the WebAPICallResult properly models a "result" from a "call" to the "web API", but that's not what the developer really gets from a resolved Method call with the current implementation. There's some processing of that raw result before the developer gets to it.

That's is to let the typechecker know that while in general the error property is optional, when it's used in a WebAPIPlatformError, it will always be a string.

Then why is the signature for apiCall not Promise<WebAPICallResult & { error: never; }>? It feels as though the inverse is also true: while error is optional, when it's used as a result of an API call, it will never be defined.

🙂 Let me know if I've missed something while thinking about this.

---

@lenkan I cannot deny that your findUserId(email) example is definitely appealing. I feel like you can mimic the behavior you want with a helper function:

/** Includes Web API platform errors as resolved values. */
async function withPlatformError(p: Promise<WebAPICallResult>): Promise<WebAPICallResult & { ok: boolean; error?: string; }> {
  try {
    // pass successes through
    return await p;
  } catch (err) {
    if (err.code === 'slack_webapi_platform_error' && err.data !== undefined) {
      // resolve Web API Platform errors
      resolve(err.data);
    } else {
      // reject other errors
      reject(err);
    }
  }
}

(Note: untested.)

The usage would be something like your original (clean) function:

async function findUserId(email) {
  // throws for HTTP errors (and such), but not for Web API errors
  const result = await withPlatformError(client.users.lookupByEmail({ email }));
  
  if(result.error === 'users_not_found') {
     return undefined;
  }
  if (result.error) { // other API errors
     throw new Error(result.error);
  }

  return result.user.id;
}

I can't think of a good way this might be implemented into the web-api package, however.

I'd also be concerned about unhandled errors. Maybe a map function is more appropriate?

async function findUserId(email) {
  // "map some platform errors"
  const result = await mapPlatformErrors(
    client.users.lookupByEmail({ email }),

    // maps a `users_not_found` error to return `undefined`;
    // throws for all other platform errors
    { 'users_not_found': undefined },
  );

  return result && result.user.id;
}

If we ever get optional chaining, then this gets even shorter and sweeter:

const findUserId = async (email) => await mapPlatformErrors(
  client.users.lookupByEmail({ email }),
  { 'users_not_found': undefined },
)?.user.id;

But I'm just dreaming now. 😊

[oligriffiths]

I'd like to chime in here that it would be useful to include more info in the WebAPIPlatformError object.

Here's an example error

{ Error: An API error occurred: not_in_channel
    at Object.platformErrorFromResult (/Users/goli/Projects/Tumblr/houston/app/node_modules/@slack/web-api/src/errors.ts:94:5)
    at WebClient.apiCall (/Users/goli/Projects/Tumblr/houston/app/node_modules/@slack/web-api/src/WebClient.ts:186:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  code: 'slack_webapi_platform_error',
  data:
   { ok: false,
     error: 'not_in_channel',
     response_metadata: { scopes: [Array], acceptedScopes: [Array] } }

This gives no clues about what the channel is or what the request parameters were/request method being used.

Here's what I think would help:

export interface WebAPIPlatformError extends CodedError {
  code: ErrorCode.PlatformError;
  data: WebAPICallResult & {
    error: string;
  };
  method?: string;
  options?: WebAPICallOptions;
}

Here's an example: main...oligriffiths:oli/error-options

that produces:

{ Error: An API error occurred: not_in_channel
    at Object.platformErrorFromResult (/Users/goli/Projects/Tumblr/houston/app/node_modules/@slack/web-api/src/errors.ts:94:5)
    at WebClient.apiCall (/Users/goli/Projects/Tumblr/houston/app/node_modules/@slack/web-api/src/WebClient.ts:186:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  code: 'slack_webapi_platform_error',
  data:
   { ok: false,
     error: 'not_in_channel',
     response_metadata: { scopes: [Array], acceptedScopes: [Array] } },
  method: 'chat.postMessage',
  options:
   { channel: 'C01494KGQA2',
     as_user: true,
     text: null,
     attachments: [ [Object] ] } }

Thoughts?

[github-actions]

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.

[filmaj]

After many years of inactivity here, I am reviewing the typing of the web-api library and I agree, the behaviour described in this issue is not ideal.

Using the original poster's example of calling users.lookupByEmail, if that call fails, an error will be thrown:

    at platformErrorFromResult (/Users/fmaj/src/node-slack-sdk/packages/web-api/src/errors.ts:117:5)
    at WebClient.apiCall (/Users/fmaj/src/node-slack-sdk/packages/web-api/src/WebClient.ts:308:13)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at <anonymous> (/Users/fmaj/src/node-slack-sdk/packages/web-api/test.ts:8:18)

Additionally, the error will have a structure like so:

 {
  code: 'slack_webapi_platform_error',
  data: {
    ok: false,
    error: 'users_not_found',
    response_metadata: { ... }
  }

.. while a successful call's result will look like:

{
  ok: true,
  user: { ... },
  response_metadata: { ... }
}

IMHO, the root of this issue is that, in the rejection/error case, we "wrap" the API result in an error object and nest the API result under a data property. We do NOT do this when the API result is successful. This leads to the difference between the two cases and the root of this bug which is that the error API response does not adhere to the WebAPICallResult type at runtime. So, while the library promises a specific result shape (WebAPICallResult) it violates this promise at runtime for errors.

However, I could easily see the other side, too: because an exception is thrown if the API result is not successful, TypeScript offers no type guarantees for exceptions (AFAICT exceptions are all of type any or unknown). So, really, type safety in exceptional cases is not guaranteed.

Ultimately, any change we decide on to improve the developer experience for this issue would be a breaking change, so would not land until (at least) v8 of web-api. That said, perhaps we could come up with a better developer experience here and inch towards that future major version release. Some questions to guide a potential future solution:

• should errored API calls throw an exception? If we change the behaviour here to "no", then we could offer type safety for unsuccessful API responses, but I imagine it would be a big pain in the ass for developers of existing apps using this library to change their code to remove try/catch blocks to handle unsuccessful API responses.
• we could simply not wrap unsuccessful API calls in an error and return the result directly. This would at least conform to the WebAPICallResult interface, though you would not get editor autocomplete niceties / typing in your editor unless you write code to do the type narrowing yourself.