search

hyperledger / cacti

Hyperledger Cacti is a new approach to the blockchain interoperability problem
https://wiki.hyperledger.org/display/cactus
Apache License 2.0
338 stars 277 forks source link

buid(tsc): re-enable useUnknownInCatchVariables typescript compiler #2463

Open petermetz opened 1 year ago

petermetz commented 1 year ago

Description

As a maintainer I want the compiler to set to be as paranoid and strict as possible so that my job as a reviewer is easier because by the time the code makes it to review it passed more checks (by way of the compiler) that were done for me automatically.

TLDR: Enable globally the "useUnknownInCatchVariables" flag in tsconfig.json because it forces our error handling code to be audited for runtime type checks on the thrown exceptions (which is necessary because Javascript can throw literally anything as an exception...)

https://www.typescriptlang.org/tsconfig#useUnknownInCatchVariables

The reason why this is necessary because https://github.com/hyperledger/cacti/pull/2179 has turned the flag off just to get it over the line quicker.

Acceptance Criteria

  1. useUnknownInCatchVariables is set to true AND the project builds OK
  2. All the catch blocks were modified in a way that is correctly handling the fact that the thrown exceptions are of type unknown so there's no cop-put casts to any or Error without actual type checking. 2.1. There are no unhandled cases, 2.2. there are no empty catch blocks, 2.3. there is no data hiding/obscuring of the original exception just because it was easier to sweep in under the rug that way. 2.4. etc. the bottom line is: the exception handling must be robust and handle gracefully if the thrown value was not Error
rwat17 commented 4 months ago

@petermetz @jagpreetsinghsasan I can take care of it if no one is on it right now.

petermetz commented 4 months ago

@petermetz @jagpreetsinghsasan I can take care of it if no one is on it right now.

@rwat17 Works for me but I have one ask: Break it down to smaller pull requests. This change triggers many thousands of lines of refactoring needed in catch-blocks so the way you have to go about is this:

  1. Enable the check in the compiler configuration
  2. Fix the compiler errors in a limited scope (a single package or even better just a single code file)
  3. Turn the check off again so that the build is passing again
  4. Submit the PR with the changes from 2)

The reason I'm asking is because this way it will be easier to review. It looks like a trivial refactor but we'll probably get down to the weeds about error handling specifics, HTTP codes, accidentally hiding exception message/stack traces, etc.

rwat17 commented 4 months ago

@petermetz Sure, will do like you advised.

rwat17 commented 2 months ago

@petermetz made a PR, there is quite a lot to check. Made separated commits as requested.

petermetz commented 1 month ago

@petermetz made a PR, there is quite a lot to check. Made separated commits as requested.

@rwat17 I was asking to break it down to separate pull requests not commits though. Good thing you have at least the separate commits because that should make it easy to cherry pick those commits one by one into separate PRs!