line
[Refactor] Improve Error Handling Consistency and Accessibility in TypeScript Error Classes
Issue Summary:
In our current TypeScript error management implementation, there are opportunities to enhance consistency and debuggability. Specifically, ensuring error names match class names for easier tracing, and improving the accessibility of original error information.
Proposed Improvements:
1. Set the name property of each custom error class to match its class name for better error tracing.
export class SignatureValidationFailed extends Error {
constructor(
message: string,
public signature?: string,
) {
super(message);
// #FIXME: #1. Error name should be consistent with the class name to trace the error source easily
this.name = this.constructor.name;
}
}
prev:
Error: The provided signature is invalid.
at someFunction (/path/to/file.js:10:11)
at async someOtherFunction (/path/to/anotherFile.js:5:9)
refactor:
SignatureValidationFailed: The provided signature is invalid.
at someFunction (/path/to/file.js:10:11)
at async someOtherFunction (/path/to/anotherFile.js:5:9)
2. Convert the constructor arguments of HTTPError into an options object to improve readability and maintainability.
interface HTTPErrorOptions {
message: string;
status: number;
statusText: string;
originalError: any;
}
export class HTTPError extends Error {
public status: number;
public statusText: string;
public originalError: any;
// #FIXME: #2. If the argument is more than 3, it's better to use an object to pass the arguments to avoid confusion and human error
constructor({
message,
status,
statusText,
originalError,
}: HTTPErrorOptions) {
super(message);
this.name = this.constructor.name;
this.status = status;
this.statusText = statusText;
this.originalError = originalError;
}
}
prev:
if (err.response) {
return new HTTPError(
err.message,
err.response.status,
err.response.statusText,
err,
);
}
refactor:
if (err.response) {
const { message } = err;
const { status, statusText } = err.response;
return new HTTPError({ message, status, statusText, err }); // ⛳️
}
3. The originalError field in the RequestError class deviates from the convention of the Error objects.
Even if you pass the Error type for some reason, you can maintain the convention to some extent with Destructive assistance.(Even if #531Issue merge, we expect no significant change.)
private wrapError(err: AxiosError): Error {
// ...codes
} else if (err.config) {
// unknown, but from axios
return new ReadError(err); // ⛳️
}
// otherwise, just rethrow
return err;
}
prev:
export class ReadError extends Error {
// #FIXME: #3. `{ message }: Error`or`message: string` is better than originalError: Error
constructor(private originalError: Error) {
super(originalError.message);
this.name = this.constructor.name;
}
}
refactor:
export class ReadError extends Error {
constructor({ message }: Error) {
super(message);
this.name = this.constructor.name;
}
}
or
export class ReadError extends Error {
constructor(message: string) {
super(message);
this.name = this.constructor.name;
}
}
Expected Outcome:
These changes aim to make error handling more intuitive and to enhance the developer experience by providing clearer error information and simplifying the debugging process.
Additional Context:
The proposed improvements were drafted with best practices in mind and are intended to align our error handling strategy with standard TypeScript conventions.
- we can introduce a static factory method pattern that centralizes the error generation code. However, hasty abstraction can be poisonous, so we will put that aside. If you agree, please take care of the ticket. We will take care of it as soon as possible.
@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. :)
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 https://github.com/line/line-bot-sdk-nodejs/pull/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!
@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 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
]
}
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:)
I would like to ask if it is okay to add that option to tsconfig.json
no problem~
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.
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 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!
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!