chore(#8437): enable useUnknownInCatchVariables rule in tsconfig

[jkuester]

Description

#8437

The useUnknownInCatchVariables rule basically just makes it so that the TS compiler considers errors in a catch block to have the unknown type (instead of the any type). This necessitates proper type checking when referencing properties on the error. In general I think the "correct" way to handle unknown types is to establish a formal expected type for the value and then create a type guard (or type assertion) to make sure we have the data we expect. However, for catch blocks in particular this is tricky since really the error could be anything, coming from anywhere. For the most part our catch logic just want to grab a (possibly undefined) property value off of of the error and then move on. It does not seem super useful to make the error a formal data type. With that in mind, my approach here was just to create a couple utility functions to make it easier to read properties from an unknown in a type-safe way. Nothing fancy, but it works with minimal additional code needed in the catch blocks. However, I am far from being convinced this is the best approach, so I am very open to other suggestions for better ways to handle this.

Since, we do not have any formal schema validation library, I had to roll my own logic here for checking the properties. I had trouble figuring out where in the webapp code base I should put these functions. All the other code seems to be organized/categorized by specific Angular features with no where that I could just add some general-purpose utility functions. So, I ended up creating webapp/src/ts/libs and adding the schema.ts file to hold my functions. Once again, very open to alternate suggestions here...

Then I had the issue of how to test the webapp/src/ts/libs. It does not need karma tests since it is not Angular-specific. Instead I just wanted plan mocha tests in TS files. Thankfully, in a separate branch I had already been working on adding support for this. So, I pulled that code in here. In general, this is all very excessive just to get these two little schema functions. However, my thought is that this is not so much a one-off as it is a new core pattern that can be leveraged by more code in the future. (As noted, I already have plans to add additional tests like this in a separate branch....) Main thing is that it is providing the opportunity for adding code/tests that is not tightly coupled to Angular.

Code review checklist

• Readable: Concise, well named, follows the style guide, documented if necessary.
• Tested: Unit and/or e2e where appropriate
• Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

• Core
• CouchDB Single
• CouchDB Cluster

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[m5r]

I'm not sold on this change yet... I agree we shouldn't assume anything thrown/caught is an Error object and the unknown type is best for this purpose because it forces us to do proper validation in the catch clause. But this change barely gets us halfway there: it checks that the property exists but it doesn't help with type-safety.
I don't think a generic lib file can solve this problem on its own. I think this sort of validation should be limited to its respective catch clause. For example here this could look like:

import { HttpErrorResponse } from '@angular/common/http';

try {
	// ...
} catch {
	if (error instanceof HttpErrorResponse) {
		const status = error.status;
//			^? number
	}
}

[jkuester]

🙌 This is just the kind of push back I was hoping for! I am very interested in digging in deeper here to find the best solution.

The challenge I see with doing something like error instanceof HttpErrorResponse in existing code is that we would have to be absolutely certain of what is in scope for the try-block and what kind of errors might be thrown. Taking this code as a good example, there is quite a bit of logic wrapped in the try-block and I am not confident that anything that was being thrown with a status property, is definitely going to be instanceof HttpErrorResponse. For example, even with a local Pouch db, if you call get with a docId that does not exist, it will throw an error with status: 404, but I do not think it will be a HttpErrorResponse...

This was the main reason I settled on this half-baked pattern matching approach. 😬

his change barely gets us halfway there: it checks that the property exists but it doesn't help with type-safety.

100% agree. But, I really think that if we are going to do nested type-checking, we should revisit the idea of adding a schema validation library.... 😬 I have found effect/schema to be super powerful in chtoolbox and I think we could reduce a lot of manual logic that we have by pulling in something like that. (FTR, not saying we go with effect/schema here in cht-core.... 😅 Zod, seems like the default option, but I find https://github.com/fabian-hiller/valibot to be very interesting as well, particularly with its claims of superior tree-shaking....)

[jkuester]

Did a simple side-by-side comparison of zod vs valibot for evaluating the schema of this error. The code is nearly identical:

zod:

      const MyErrorSchema = z.object({
        status: z.number().optional(),
      });
      const myError = MyErrorSchema.parse(error);

valibot:

      const MyErrorSchema = v.object({
        status: v.optional(v.number()),
      });
      const myError = v.parse(MyErrorSchema, error);

Then, I used npm run build to build the prod assets (with tree-shaking) and compared the sizes of api/build/static/webapp:

• Current: 8384K
• Zod: 8444K - +60K
• Valibot: 8388K - +4K

This is inline with valibot's claims to be significantly more efficient than Zod because the structure of the project is optimized for tree shaking.

IMHO +4K is worth it for sane schema validation. In reality, we should be able to replace a lot of janky manual code checks with valibot.