enable useUnknownInCatchVariables typescript compiler #3361
Conversation
rwat17
requested review from
RafaelAPB,
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
| @@ -105,7 +105,7 @@ export class Containers { | |||
| return response; | |||
| } catch (ex) { | |||
| log.error("Failed to get diagnostics of Docker daemon", ex); | |||
| throw new RuntimeError("Failed to get diagnostics of Docker daemon", ex); | |||
| throw new RuntimeError(`Failed to get diagnostics of Docker daemon${ex}`); | |||
There was a problem hiding this comment.
@rwat17 This completely disappears all the useful information that we could gain from the exception object. Please make sure to preserve the inner exceptions.
I won't post the same comment everywhere in the diff but please fix it everywhere.
| message: "Internal Server Error", | ||
| error: ex?.stack || ex?.message, | ||
| }); | ||
| const errorMsg = `Internal server Error`; |
There was a problem hiding this comment.
@rwat17 The underlying method call could've throw an HTTP 400 exception. We don't know here if it's internal server error or not (the original code is wrong on this too). I've created a PR to showcase how to use the new error handling utilities in this type of scenario:
https://github.com/hyperledger/cacti/pull/2869/files
Specifically if you look at this file in that PR you'll see that it doesn't say either side to be at fault (internal server error vs. user error) and instead just let's the underlying utility function decide on it based on the suggested generic logic:
packages/cactus-plugin-ledger-connector-besu/src/main/typescript/web-services/deploy-contract-solidity-bytecode-endpoint.ts
I won't post this review comment everywhere, but please address it across the PR.
| if ( | ||
| typeof ex === "object" && | ||
| ex !== null && | ||
| "message" in ex && | ||
| ex["message"] && | ||
| typeof ex.message === "string" | ||
| ) { |
There was a problem hiding this comment.
@rwat17 You can achieve the same type narrowing with
if (ex instanceof Error) {
// now in here the compiler let's you have a
// truthy ex.message string property to work with.
}If for whatever reason the thrown exception wasn't an instance of Error then that's definitely an internal server error so it's OK for us to then fall through to the generic logic.
I won't post this comment at all the occurrences of this same code pattern but please make sure to address it across the board.
| @@ -28,6 +28,12 @@ import { SetKeychainEntryEndpointV1 } from "./web-services/set-keychain-entry-en | |||
| import { HasKeychainEntryEndpointV1 } from "./web-services/has-keychain-entry-endpoint-v1"; | |||
| import { DeleteKeychainEntryEndpointV1 } from "./web-services/delete-keychain-entry-endpoint-v1"; | |||
|
|
|||
| interface ErrorResponse extends Error { | |||
There was a problem hiding this comment.
@rwat17 Could the AxiosError type be used here instead of creating our own? The upside of the AxiosResponse type is that it ships with a custom type-guard function as well which you can use to narrow the type down with an if condition (isAxiosError())
There was a problem hiding this comment.
AxiosError has .status not .statusCode property. This is associated with http error response. But i cant find proper interface to handle this case.
| res.status(HttpStatus.INTERNAL_SERVER_ERROR); | ||
| res.statusMessage = ex.message; | ||
| res.json({ error: ex.stack }); | ||
| res.status(HttpStatus.INTERNAL_SERVER_ERROR).json({ |
There was a problem hiding this comment.
@rwat17 Please use the handleRestEndpointException utility instead of hardcoding the status code.
I won't post this everywhere but please address the same change across the PR
rwat17
force-pushed
the
flag-catch-rebase
branch
from
0be22af
to
af39d46
Compare
|
Thanks for your contribution. LGTM, please make sure to incorporate Peter's feedback. |
Closes: #2463