[Refactor] Improve Error Handling Consistency and Accessibility in TypeScript Error Classes

[pengooseDev]

[Refactor] Improve Error Handling Consistency and Accessibility in TypeScript Error Classes

[pengooseDev]

@Yang-33
Thx Team :)

Currently, the improvement on the error is completed.
I would like to proceed with further improvement on the test code. However, if we need to release it quickly, we will write PR right away. :)

Improve Unit Test management methods

I suggest further improvements to the Unit test, which is currently well written.

- Use the description to separate and manage the test suite.
- Each test case should be managed in an array.

This reduces the creation of repeated test codes and provides the convenience of easily adding and removing test cases.

Below is an example of @/test/middleware.spec.ts.
(If you think it's good, I'll try to improve it.)

Prev

  it("succeed with pre-parsed string", async () => {
    await http().post(`/mid-text`, {
      events: [webhook],
      destination: "Uaaaabbbbccccddddeeeeffff",
    });
    const req = getRecentReq();
    deepEqual(req.body.destination, "Uaaaabbbbccccddddeeeeffff");
    deepEqual(req.body.events, [webhook]);
  });

  it("succeed with pre-parsed buffer", async () => {
    await http().post(`/mid-buffer`, {
      events: [webhook],
      destination: "Uaaaabbbbccccddddeeeeffff",
    });
    const req = getRecentReq();
    deepEqual(req.body.destination, "Uaaaabbbbccccddddeeeeffff");
    deepEqual(req.body.events, [webhook]);
  });

  it("succeed with pre-parsed buffer in rawBody", async () => {
    await http().post(`/mid-rawbody`, {
      events: [webhook],
      destination: "Uaaaabbbbccccddddeeeeffff",
    });
    const req = getRecentReq();
    deepEqual(req.body.destination, "Uaaaabbbbccccddddeeeeffff");
    deepEqual(req.body.events, [webhook]);
  });

Refactored

  describe("Succeeds on parsing valid request", () => {
    const testCases = [
      {
        describe: "pre-parsed string",
        endpoint: `/mid-text`,
      },
      {
        describe: "pre-parsed buffer",
        endpoint: `/mid-buffer`,
      },
      {
        describe: "pre-parsed buffer in rawBody",
        endpoint: `/mid-rawbody`,
      },
    ];

    testCases.forEach(({ describe, endpoint }) => {
      it(describe, async () => {
        await http().post(endpoint, {
          events: [webhook],
          destination: DESTINATION,
        });

        const req = getRecentReq();
        deepEqual(req.body.destination, DESTINATION);
        deepEqual(req.body.events, [webhook]);
      });
    });
  });

The weather is getting warmer and spring is here. I hope you have a wonderful day!

[pengooseDev]

@Yang-33
Thank you for your response. :)

Point 1, 2

I will create a PR for the changes, leave the additional improvements considered as comments to receive a review of the direction, and complete the PR. :)

Point 3-a)

It doesn't seem to be much different from other error generation logic, so it would be good to teach the convention. :) I briefly looked at the commit log and history, but I don't see any notable problems yet.

It would be nice to synchronize the message convention to super with other messages and receive the parameters as structural decomposition allocation.

After applying the code changes, it would be good to discuss the error management direction in PR. I will leave PR as soon as I have time!

---

+a Refactoring

If you see the improvement as below, we will leave a comment and apply it to PR after receiving a review.

// good 👍

private async checkResponseStatus(response: Response) {
    const body = await response.text();
    
    if (!response.ok) {
      throw new HTTPFetchError(
        response.status,
        response.statusText,
        response.headers,
        body,
      );
    }
  }

// better 👍👍

private async checkResponseStatus({ ok, status, statusText, text, headers }: Response) {    
    if (!ok) {
      const body = await text();

      throw new HTTPFetchError({
        status,
        statusText,
        headers,
        body,
      });
    }
  }

Also, if you think the unit test is necessary, we will leave this in the comment and receive a review of the direction. :) Please allocate the issue Plz.

[Yang-33]

Thank you. I totally agree with you. Let me assign this to you~

[Yang-33]

I would like to ask if it is okay to add that option to tsconfig.json

no problem~

[Yang-33]

I would like to ask if it is okay to change the field name of the member variable to statusCode => status :)

For the sake of convention or consistency, right? If that's the case, I think it's a good idea.

[Yang-33]

Fixed (statusCode => status)

Please also fix examples and documentation(https://github.com/line/line-bot-sdk-nodejs/pull/734/files might help you.), not only code itself~

[Yang-33]

Note: After this issue is resolve, we plan to release v9.0.0 🎉

[Yang-33]

Thank you for all the wonderful suggestions. I greatly appreciate it.

Preferably, changes should be kept as small as possible. While I think the original issue can be addressed in a single PR, I would be happy if the test improvements were in a separate PR.

There is no rush for the changes. I look forward to your patch!

[pengooseDev]

@Yang-33 After migration from axios to fetch, if you need to process the issue, I would appreciate it if you could assign and call the issue. :)

[Yang-33]

Hi, @pengooseDev! Thank you for using line-bot-sdk-nodejs and proposing this awesome issue.

I apologies for the delayed response to you.

Regarding points 1 and 2, we welcome patches from you 😄

For point 3, in the upcoming release, we've decided to remove the axios dependency for maintained clients (meaning those that are not deprecated) and introduce a new HTTPFetchError.(thrown here) This error type will not encapsulate an internal error, while HTTPError does. These changes have been implemented in #727. I have two questions for you:

(3-a) What are your thoughts on the HTTPFetchError in the context of point 3? Please let us know if you see any issues.
(3-b) Do you use a deprecated client? We will not make changes to the deprecated client that rely on axios. However, if you need, we welcome patches.

Thank you for your engagement with the project, and we look forward to your feedback!

[pengooseDev]

@Yang-33
In local test environment with Mocha, the settings for @types/mocha are not applied to tsconfig.
The build itself is fine, but ts error is occurring in an environment where mocha is not global installed.
To solve this problem, I would like to ask if it is okay to add that option to tsconfig.json

// tsconfig.json
{
  "compilerOptions": {
    // ...options
    "types": ["mocha"] // ⛳️ Added
  },
  "include": [
    "lib/**/*.ts",
    "test/**/*.spec.ts" // ⛳️ Added
  ]
}

[pengooseDev]

I tried to apply the changes to the samples/echo-bot-ts/index.ts file, but the code convention is not applied because eslint is not applied. The examples should be changed as follows.

          if (err instanceof HTTPFetchError) {
            console.error(err.status); // ⛳️ Fixed (statusCode => status)
            console.error(err.headers.get('x-line-request-id'));
            console.error(err.body);
          } else if (err instanceof Error) {
            console.error(err);
          }

• In addition, I would like to ask if it is okay to change the field name of the member variable to statusCode => status :)

[pengooseDev]

@Yang-33
Thx for your feedback. I’ll remain PR soon! ☺️