Skip to content

Commit 1bccab7

Browse files
authored
Refactoring Exception (#739)
## PR Context This PR is based on Issue #699 to `Refactoring exception(Error class)` ## Need Feedbacks ### 1. unused Field (originalError) ```ts 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! ```ts 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. :) ```ts 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.
1 parent ec41239 commit 1bccab7

File tree

10 files changed

+118
-49
lines changed

10 files changed

+118
-49
lines changed

docs/guide/client.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ client
111111
})
112112
.catch((err) => {
113113
if (err instanceof HTTPFetchError) {
114-
console.error(err.statusCode);
114+
console.error(err.status);
115115
console.error(err.headers.get('x-line-request-id'));
116116
console.error(err.body);
117117
}

examples/echo-bot-ts/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ app.post(
8282
await textEventHandler(event);
8383
} catch (err: unknown) {
8484
if (err instanceof HTTPFetchError) {
85-
console.error(err.statusCode);
85+
console.error(err.status);
8686
console.error(err.headers.get('x-line-request-id'));
8787
console.error(err.body);
8888
} else if (err instanceof Error) {

lib/exceptions.ts

Lines changed: 78 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,109 @@
1+
type Message = string;
2+
3+
interface Status {
4+
status: number;
5+
statusText: string;
6+
}
7+
8+
interface ErrorDetails {
9+
signature?: string;
10+
raw?: any;
11+
}
12+
13+
interface FetchErrorDetails extends Status {
14+
headers: Headers;
15+
body: string;
16+
}
17+
18+
// Deprecated
19+
interface AxiosErrorDetails {
20+
originalError: Error;
21+
code?: string;
22+
statusCode?: number;
23+
statusMessage?: string;
24+
}
25+
126
export class SignatureValidationFailed extends Error {
2-
constructor(
3-
message: string,
4-
public signature?: string,
5-
) {
27+
public signature?: string;
28+
29+
constructor(message: Message, { signature }: ErrorDetails = {}) {
630
super(message);
31+
this.name = this.constructor.name;
32+
33+
Object.assign(this, { signature });
734
}
835
}
936

1037
export class JSONParseError extends Error {
11-
constructor(
12-
message: string,
13-
public raw: any,
14-
) {
38+
public raw: any;
39+
40+
constructor(message: Message, { raw }: ErrorDetails = {}) {
1541
super(message);
42+
this.name = this.constructor.name;
43+
44+
Object.assign(this, { raw });
1645
}
1746
}
1847

48+
/* Deprecated */
1949
export class RequestError extends Error {
20-
constructor(
21-
message: string,
22-
public code: string,
23-
private originalError: Error,
24-
) {
50+
public code: string;
51+
52+
private originalError: Error;
53+
54+
constructor(message: Message, { code, originalError }: AxiosErrorDetails) {
2555
super(message);
56+
this.name = this.constructor.name;
57+
58+
Object.assign(this, { code, originalError });
2659
}
2760
}
2861

2962
export class ReadError extends Error {
30-
constructor(private originalError: Error) {
31-
super(originalError.message);
63+
public originalError: Error;
64+
65+
constructor(message: Message, { originalError }: AxiosErrorDetails) {
66+
super(message);
67+
this.name = this.constructor.name;
68+
69+
Object.assign(this, { originalError });
3270
}
3371
}
3472

3573
export class HTTPError extends Error {
74+
public statusCode: number;
75+
76+
public statusMessage: string;
77+
78+
public originalError: any;
79+
3680
constructor(
37-
message: string,
38-
public statusCode: number,
39-
public statusMessage: string,
40-
public originalError: any,
81+
message: Message,
82+
{ statusCode, statusMessage, originalError }: AxiosErrorDetails,
4183
) {
4284
super(message);
85+
this.name = this.constructor.name;
86+
87+
Object.assign(this, { statusCode, statusMessage, originalError });
4388
}
4489
}
4590

4691
export class HTTPFetchError extends Error {
92+
public status: number;
93+
94+
public statusText: string;
95+
96+
public headers: Headers;
97+
98+
public body: string;
99+
47100
constructor(
48-
public statusCode: number,
49-
public statusMessage: string,
50-
public headers: Headers,
51-
public body: string,
101+
message: Message,
102+
{ status, statusText, headers, body }: FetchErrorDetails,
52103
) {
53-
super(`${statusCode} - ${statusMessage}`);
104+
super(message);
105+
this.name = this.constructor.name;
106+
107+
Object.assign(this, { status, statusText, headers, body });
54108
}
55109
}

lib/http-axios.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -164,17 +164,22 @@ export default class HTTPClient {
164164

165165
private wrapError(err: AxiosError): Error {
166166
if (err.response) {
167-
return new HTTPError(
168-
err.message,
169-
err.response.status,
170-
err.response.statusText,
171-
err,
172-
);
167+
const { status, statusText } = err.response;
168+
const { message } = err;
169+
170+
return new HTTPError(message, {
171+
statusCode: status,
172+
statusMessage: statusText,
173+
originalError: err,
174+
});
173175
} else if (err.code) {
174-
return new RequestError(err.message, err.code, err);
176+
const { message, code } = err;
177+
return new RequestError(message, { code, originalError: err });
175178
} else if (err.config) {
176179
// unknown, but from axios
177-
return new ReadError(err);
180+
const { message } = err;
181+
182+
return new ReadError(message, { originalError: err });
178183
}
179184

180185
// otherwise, just rethrow

lib/http-fetch.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,13 +165,18 @@ export default class HTTPFetchClient {
165165
}
166166

167167
private async checkResponseStatus(response: Response) {
168-
if (!response.ok) {
169-
throw new HTTPFetchError(
170-
response.status,
171-
response.statusText,
172-
response.headers,
173-
await response.text(),
174-
);
168+
const { ok, status, statusText, headers } = response;
169+
const message = `${status} - ${statusText}`;
170+
171+
if (!ok) {
172+
const body = await response.text();
173+
174+
throw new HTTPFetchError(message, {
175+
status,
176+
statusText,
177+
headers,
178+
body,
179+
});
175180
}
176181
}
177182
}

lib/middleware.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ export default function middleware(config: Types.MiddlewareConfig): Middleware {
5555

5656
if (!validateSignature(body, secret, signature)) {
5757
next(
58-
new SignatureValidationFailed("signature validation failed", signature),
58+
new SignatureValidationFailed("signature validation failed", {
59+
signature,
60+
}),
5961
);
6062
return;
6163
}
@@ -66,7 +68,9 @@ export default function middleware(config: Types.MiddlewareConfig): Middleware {
6668
req.body = JSON.parse(strBody);
6769
next();
6870
} catch (err) {
69-
next(new JSONParseError(err.message, strBody));
71+
const { message } = err;
72+
73+
next(new JSONParseError(message, { raw: strBody }));
7074
}
7175
};
7276
return (req, res, next): void => {

lib/utils.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export function ensureJSON<T>(raw: T): T {
88
if (typeof raw === "object") {
99
return raw;
1010
} else {
11-
throw new JSONParseError("Failed to parse response body as JSON", raw);
11+
throw new JSONParseError("Failed to parse response body as JSON", { raw });
1212
}
1313
}
1414

test/helpers/test-server.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
} from "../../lib/exceptions";
1010
import * as finalhandler from "finalhandler";
1111

12-
let server: Server = null;
12+
let server: Server | null = null;
1313

1414
function listen(port: number, middleware?: express.RequestHandler) {
1515
const app = express();
@@ -83,8 +83,9 @@ function listen(port: number, middleware?: express.RequestHandler) {
8383
function close() {
8484
return new Promise(resolve => {
8585
if (!server) {
86-
resolve(undefined);
86+
return resolve(undefined);
8787
}
88+
8889
server.close(() => resolve(undefined));
8990
});
9091
}

test/http-fetch.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ describe("http(fetch)", () => {
217217
} catch (err) {
218218
ok(err instanceof HTTPFetchError);
219219
equal(scope.isDone(), true);
220-
equal(err.statusCode, 404);
220+
equal(err.status, 404);
221221
equal(err.headers.get("content-type"), "application/json");
222222
equal(err.body, '{"reason":"not found"}');
223223
}

tsconfig.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@
55
"noImplicitAny": true,
66
"outDir": "dist",
77
"rootDirs": ["lib", "test"],
8-
"declaration": true
8+
"declaration": true,
99
},
1010
"include": [
11-
"lib/**/*.ts"
11+
"lib/**/*.ts",
1212
]
1313
}

0 commit comments

Comments
 (0)