Improve ClientResponse typing

[ColinFrick]

Currently the Client Response is typed as:

export default class ClientResponse<T> {
  public statusCode: number;
  public response: T;
  public exception: Error;

  wasSuccessful() {
    return this.statusCode >= 200 && this.statusCode < 300;
  }
}

The thing is if a API function returns a value it is always a successful response, because in cause of an error the Error is thrown (ergo the function does not return normally).

try {
    const response = await client.retrieveRefreshTokens(user);
    if (response.wasSuccessful()) { // Always true
        const token = response.response.refreshToken; // response.response: RefreshTokenResponse
        const error = response.exception.message; // No compile error, but will throw a `exception is undefined` error
    }
} catch (e) {
    console.log(e); // e is a ClientResponse without response
}

So both response and exception properties should be marked as optional:

export default class ClientResponse<T> {
  public statusCode: number;
  public response?: T;
  public exception?: Error;

  wasSuccessful() {
    return this.statusCode >= 200 && this.statusCode < 300;
  }
}

But because either the function returns a response or throws an exception they should be the same type:

class ClientResponse<T> {
    public statusCode: number;
    public response: T;
}

class ErrorResponse<T> {
    public statusCode: number;
    public exception: Error;
}

wasSuccessful can be dropped in this case, because the function either returns something (wasSuccessful = true) or it throws an error (wasSuccessful = false).

As an alternative functions could never throw, but return either ClientResponse<T> or ErrorResponse:

type ClientResponse<T> = {
    statusCode: number;
    response: T;
    wasSuccessful: true;
}

type ErrorResponse = {
    statusCode: number;
    exception: Error;
    wasSuccessful: false;
}

type ClientResponse<T> = Response<T> | ErrorResponse;

I used types instead of classes because of type-hinting:

const response = await client.retrieveRefreshTokens(userId);
if (response.wasSuccessful) {
    const token = response.response.refreshToken; // response.response: RefreshTokenResponse
} else {
    throw new Error(response.exception.message); // response.exception: Error
}

[FrikkieSnyman]

Also running into this - we wrapped this library assuming the types are correct. However, we soon noticed that the wasSuccessful is never false, and instead, we get errors being thrown.

Hence, either the type of ClientResponse should either be corrected, as @ColinFrick noted above.

Alternatively, if the intention behind the type is correct, then that intention should be reflected correctly in the behaviour defined in the DefaultRESTClient here.

Notice how when response.ok is false, line 228 throws instead of returns the errorResponseHandler. To adhere to the intention behind the typings, this line should return instead.

The check on response.ok was introduced here - by @robotdan. This was years ago, so I don't imagine they can recall the reasoning behind it! But since they're still actively contributing, might be nice to get their input to make a decision on how to go forward with this?

[FrikkieSnyman]

In the meantime, behaviour that aligns with the types can be achieved by overriding the RESTClientBuilder:

class NonThrowableRESTClient<RT, ERT> extends DefaultRESTClient<RT, ERT> {
  override async go(): Promise<ClientResponse<RT>> {
    try {
      return await super.go();
    } catch (e) {
      return e as ClientResponse<RT>;
    }
  }
}
class NonThrowableRESTClientBuilder implements IRESTClientBuilder {
  build<RT, ERT>(host: string) {
    return new NonThrowableRESTClient<RT, ERT>(host);
  }
}

const fusionauthClient = new FusionAuthClient(
  validConfig.apiKey,
  validConfig.url,
);

fusionauthClient.clientBuilder = new NonThrowableRESTClientBuilder();