Refactoring Exception

[pengooseDev]

PR Context

This PR is based on Issue #699 to Refactoring exception(Error class)

Need Feedbacks

1. unused Field (originalError)

export class RequestError extends Error {
  public code: string;

  private originalError: Error; // ⛳️ Not used private field

  constructor(message: Message, { code, originalError }: AxiosErrorDetails) {
    super(message);
    this.name = this.constructor.name;

    Object.assign(this, { code, originalError });
  }
}

RequestError's originalError is a private field, but it is not being used.
Looking at the commit history, it seems that there are cases where the originalError field that was private is converted to public. I need feedback whether to keep or remove the field. (It doesn't matter if you keep the current code, but if you think you don't need it, we will remove it.)

2. CodeConvention

In the process of improving the class, there was no information on the code convention, so we applied Airbnb's code convention (such as applying the extension to the class member variable) lightly.
However, if you believe that excessive modification rather harms readability, I will remove all modifications. Please feedback on this as well!

export class HTTPFetchError extends Error {
  public status: number;

  public statusText: string;

  public headers: Headers;

  public body: string;

  constructor(
    message: Message,
    { status, statusText, headers, body }: FetchErrorDetails,
  ) {
    super(message);
    this.name = this.constructor.name;

    Object.assign(this, { status, statusText, headers, body });
  }
}

3. Interface

I tried to declare all the attributes of each error inside one interface, but I removed the interface. If you think it is excessive, I will manage it on one interface. :)

type Message = string;

interface Status {
  status: number;
  statusText: string;
}

interface ErrorDetails {
  signature?: string;
  raw?: any;
}

interface FetchErrorDetails extends Status {
  headers: Headers;
  body: string;
}

// Deprecated
interface AxiosErrorDetails extends Partial<Status> {
  originalError: Error;
  code?: string;
}

---

After reviewing the code, I would appreciate it if you could discuss the direction of the code and request further changes.

[Yang-33]

Thank you for the patch, @pengooseDev ! I left minor comments, but almost LGTM.

Need Feedbacks

1. unused Field (originalError)

Since this is only used in a deprecated client, honestly, either way is fine. It's an unused private field, so there is no problem with removing it.

2, 3:

It seems good. Thank you.

[Yang-33]

Unless there is a special reason, for the sake of simplifying the differences, I would like you to move HTTPFetchError back to its original position (at the bottom).

[pengooseDev]

RequestError, ReadError, and HTTPError are error classes that are currently used only in http-axios.ts (deprecated) files. All three classes were deprecated with Axios and moved to /* Deprecated */ bottom of the annotation.

1. Move back to original position.
2. Retain current position (the three recoded classes are located at the bottom of the annotation)

There may be a reason for the code I haven't checked, so please check and give me feedback! :)

[Yang-33]

This error type is also deprecated, because it's only used in http-axio.ts(it's deprecated). I don't want to change public field in deprecated code, because we don't want sdk users to modify their code with deprecated libraries by breaking change basically. Please keep statusCode as it is only in HTTPError.

(Renaming statusCode in HTTPFetchError is nice, because it's not released and no one uses this. Please keep the change! )

[pengooseDev]

I didn't think of that part! :) Thank you for the great feedback.
The basis for the naming change was as follows.

1. Use the field name provided by the response header as it is.
2. Error Classes should use the same field naming.

As you said, it seems good to separate the interfaces of the deprecated objects and make a difference in naming. I will apply it right away. :)

[Yang-33]

Please keep statusMessage (same reason as statusCode) in HTTPError

[pengooseDev]

I'll double check the changes and return the changed field name of the Deprecated Class! :)

[pengooseDev]

Sorry, I didn't understand the status field naming of HTTPFetchError.
What field name should I use? :)

- 1. status
- 2. statusCode

• The naming of the status field of the depreciated HTTPError has been restored to the 'statusCode' as your feedback thx! :)

[Yang-33]

The Response class uses status to handle the HTTP status code. I also think statusCode is clear, and httpStatusCode would be fine too. I have no particular preference. Considering that it is status: Number, it is not too ambiguous that this represents the HTTP status code.

Let's do this: Using status is fine since it's consistent with the Response class. However, if @pengooseDev, as one of an SDK user, prefers it, then statusCode is equally good. I will approve either.

[Yang-33]

CI failed and it said it wanted you to fix

line-bot-sdk-nodejs/examples/echo-bot-ts/index.ts

Line 85 in a774538

console.error(err.statusCode);

[pengooseDev]

CI failed and it said it wanted you to fix

line-bot-sdk-nodejs/examples/echo-bot-ts/index.ts

Line 85 in a774538

console.error(err.statusCode);

---

@Yang-33

CI Issue

The CI error causes for samples/echo-bot-ts/index.ts are as follows.

---

Cause and Flow of CI Issue

HTTPFetchError in the @line/bot-sdk is not currently deployed code. (So we could easily change the field naming of HTTPFetchError)

1. In the PR #734 HTTPFetchError is added.

2. CI rule was added In this PR #736

3. Create current PR(CI test Fail 🥲)

---

My reasoning may be wrong, please give me feedback plz :)

[Yang-33]

When I reverted changes in tsconfig.json of this pull request, it worked. (#743)

[Yang-33]

Hmm... Adding "test/**/*.spec.ts" to "include" in tsconfig.json causes this error.

[pengooseDev]

I'll find out more about the cause. :)

[Yang-33]

Thank you for your contribution and your patience, @pengooseDev!

[Yang-33]

In terms of Refactoring Exception, this change seems completed. I'll merge this into master.

[pengooseDev]

I'm also thx for your nice feedback! :)