Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(#8437): enable useUnknownInCatchVariables rule in tsconfig #9646

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion webapp/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
},
"scripts": {
"postinstall": "patch-package && ng cache clean",
"unit:mocha": "UNIT_TEST_ENV=1 mocha 'tests/mocha/**/*.spec.js'",
"unit:mocha": "UNIT_TEST_ENV=1 mocha 'tests/mocha/**/*.spec.js' && mocha --config tests/mocha/ts/.mocharc.js",
"unit:mocha:tz": "TZ=Canada/Pacific npm run unit:mocha && TZ=Africa/Monrovia npm run unit:mocha && TZ=Pacific/Auckland npm run unit:mocha",
"unit:cht-form": "ng test cht-form",
"unit": "UNIT_TEST_ENV=1 ng test webapp",
Expand Down
10 changes: 10 additions & 0 deletions webapp/src/ts/libs/schema.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export const hasProperty = <T extends string> (obj: unknown, prop: T): obj is Record<T, unknown> => {
return typeof obj === 'object' && obj !== null && prop in obj;
};

export const getProperty = <T extends string> (obj: unknown, prop: T): unknown => {
if (hasProperty(obj, prop)) {
return obj[prop];
}
return undefined;
};
8 changes: 5 additions & 3 deletions webapp/src/ts/modals/edit-user/update-password.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { UserSettingsService } from '@mm-services/user-settings.service';
import { UpdatePasswordService } from '@mm-services/update-password.service';
import { UserLoginService } from '@mm-services/user-login.service';
import { TranslateService } from '@mm-services/translate.service';
import { getProperty } from '../../libs/schema';

const PASSWORD_MINIMUM_LENGTH = 8;
const PASSWORD_MINIMUM_SCORE = 50;
Expand Down Expand Up @@ -64,7 +65,7 @@ export class UpdatePasswordComponent {
try {
await this.userLoginService.login(username, newPassword);
} catch (err) {
if (err.status === 302) {
if (getProperty(err, 'status') === 302) {
this.close();
const snackText = await this.translateService.get('password.updated');
this.globalActions.setSnackbarContent(snackText);
Expand All @@ -73,12 +74,13 @@ export class UpdatePasswordComponent {
}
}
} catch (error) {
if (error.status === 0) { // Offline status
const status = getProperty(error, 'status');
if (status === 0) { // Offline status
Comment on lines +77 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌 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....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Mokhtar does have a point in finding the object type of err. It is the general pattern in other programming languages as well. For eg:

  1. Java
    try {  
        // Code that may throw an exception  
    } catch (IOException e) {  
        // Handle IOException  
    } catch (NumberFormatException e) {  
        // Handle NumberFormatException  
    } catch (Exception e) {  
        // Handle any other exceptions  
    }  
  1. python
try:
    n = 0
    res = 100 / n
    
except ZeroDivisionError:
    print("You can't divide by zero!")
    
except ValueError:
    print("Enter a valid number!")
    
else:
    print("Result is", res)
    
finally:
    print("Execution complete.")

I think we should apply these here as well and I agree that we probably won't know what kind of error the error object is. We would check for the most likely errors in the conditionals. If we are not sure what the error object would be a type of then, the getProperty function could be used in the default clause. For eg: when making a network call to an endpoint it's most likely not gonna throw a FileNotFound kind of error.

I'm not sure if zod and valibot are required here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finding the object type of err. It is the general pattern in other programming languages as well.

One reason this is a general practice in other programming languages, though, is because those languages support things like checked exceptions and type-specific catch blocks (neither of which exist in JS). (Effect has some really cool features for handling this kind of thing, but that is not super helpful here.)

What we really need is some proper duck type checking that would give us a clean way to say "do this if we have the kind of thing that has a status property"....

I am happy with checking the error class, particular when catching errors that we produce ourselves (like we did with the InvalidArgumentError in the api code), or in narrowly scoped places where we can be confident of exactly what to expect. However, I think we will still end up with situations as I pointed out above where it seems risky to assume the the actual interface for the error. Like you said, in those cases we need to fall back to some kind of duck-typing.

I'm not sure if zod and valibot are required here.

Absolutly true that they are not required to solve this problem. My janky getProperty code is sufficient to make things "work". The bigger question for me is if we should stop writing janky code like getProperty (and all the crazy core field checking code in cht-datasource) and instead just adopt a proper schema validation lib. Maybe it is worth converting this PR to use valibot just to give us a feel for what that might look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought of this differently. We may not need to know the exception type unless we are doing something different for different types. Looking through the changes there is no special handling for exceptions and getting the properties from err object from catch might be sufficient for which the current implementation of getProperty is good enough. When there arises a case for special handling for different exceptions, we can consider valibot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current implementation of getProperty is good enough. When there arises a case for special handling for different exceptions, we can consider valibot.

This is fair. I think this PR is not sufficient to provide a good enough rational for validot on its own. That being said, I think there is good enough justification for valibot elsewhere in our codebase (specifically in cht-datasource). Instead of moving ahead with the hacky getProperty solution here, I am going to go ahead and move this PR back to Draft.

Then, I plan to open a new PR where I leverage valibot functionality in cht-datasource in a meaningful way. That will provide a much better opportunity to weight the costs/benefits of a schema validation lib. Depending on the outcome of that PR, I can circle back here and update this branch accordingly....

const message = await this.translateService.get('online.action.message');
this.setError(ErrorType.SUBMIT, message);
return;
}
if (error.status === 401) {
if (status === 401) {
const message = await this.translateService.get('password.incorrect');
this.setError(ErrorType.CURRENT_PASSWORD, message);
return;
Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/modules/contacts/contacts-edit.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Selectors } from '@mm-selectors/index';
import { GlobalActions } from '@mm-actions/global';
import { PerformanceService } from '@mm-services/performance.service';
import { TranslateService } from '@mm-services/translate.service';
import { getProperty } from '../../libs/schema';

@Component({
templateUrl: './contacts-edit.component.html'
Expand Down Expand Up @@ -173,7 +174,7 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit {

this.globalActions.setLoadingContent(false);
} catch (error) {
this.errorTranslationKey = error.translationKey || 'error.loading.form';
this.errorTranslationKey = getProperty(error, 'translationKey') || 'error.loading.form';
this.globalActions.setLoadingContent(false);
this.contentError = true;
console.error('Error loading contact form.', error);
Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/modules/reports/reports.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { XmlFormsService } from '@mm-services/xml-forms.service';
import { PerformanceService } from '@mm-services/performance.service';
import { ExtractLineageService } from '@mm-services/extract-lineage.service';
import { ButtonType } from '@mm-components/fast-action-button/fast-action-button.component';
import { getProperty } from '../../libs/schema';

const PAGE_SIZE = 50;
const CAN_DEFAULT_FACILITY_FILTER = 'can_default_facility_filter';
Expand Down Expand Up @@ -335,7 +336,7 @@ export class ReportsComponent implements OnInit, AfterViewInit, OnDestroy {
const userContact = await this.userContactService.get();
return userContact?.parent;
} catch (error) {
console.error(error.message, error);
console.error(getProperty(error, 'message'), error);
}
}

Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/modules/tasks/tasks.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { LineageModelGeneratorService } from '@mm-services/lineage-model-generat
import { PerformanceService } from '@mm-services/performance.service';
import { ExtractLineageService } from '@mm-services/extract-lineage.service';
import { UserContactService } from '@mm-services/user-contact.service';
import { getProperty } from '../../libs/schema';

@Component({
templateUrl: './tasks.component.html',
Expand Down Expand Up @@ -156,7 +157,7 @@ export class TasksComponent implements OnInit, OnDestroy {

} catch (exception) {
console.error('Error getting tasks for all contacts', exception);
this.errorStack = exception.stack;
this.errorStack = getProperty(exception, 'stack');
this.hasTasks = false;
this.tasksActions.setTasksList([]);
} finally {
Expand Down
13 changes: 7 additions & 6 deletions webapp/src/ts/services/android-api.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { GeolocationService } from '@mm-services/geolocation.service';
import { MRDTService } from '@mm-services/mrdt.service';
import { SessionService } from '@mm-services/session.service';
import { NavigationService } from '@mm-services/navigation.service';
import { getProperty } from '../libs/schema';

/**
* An API to provide integration with the medic-android app.
Expand Down Expand Up @@ -176,9 +177,9 @@ export class AndroidApiService {
try {
this.mrdtService.respond(JSON.parse(response));
} catch (e) {
return console.error(
new Error(`Unable to parse JSON response from android app: "${response}", error message: "${e.message}"`)
);
return console.error(new Error(
`Unable to parse JSON response from android app: "${response}", error message: "${getProperty(e, 'message')}"`
));
}
}

Expand All @@ -190,9 +191,9 @@ export class AndroidApiService {
try {
this.mrdtService.respondTimeTaken(JSON.parse(response));
} catch (e) {
return console.error(
new Error(`Unable to parse JSON response from android app: "${response}", error message: "${e.message}"`)
);
return console.error(new Error(
`Unable to parse JSON response from android app: "${response}", error message: "${getProperty(e, 'message')}"`
));
}
}

Expand Down
5 changes: 3 additions & 2 deletions webapp/src/ts/services/form.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { reduce as _reduce } from 'lodash-es';
import { ContactTypesService } from '@mm-services/contact-types.service';
import { TargetAggregatesService } from '@mm-services/target-aggregates.service';
import { ContactViewModelGeneratorService } from '@mm-services/contact-view-model-generator.service';
import { getProperty } from '../libs/schema';

/**
* Service for interacting with forms. This is the primary entry-point for CHT code to render forms and save the
Expand Down Expand Up @@ -176,11 +177,11 @@ export class FormService {
}
return await this.enketoService.renderForm(formContext, doc, userSettings);
} catch (error) {
if (error.translationKey) {
if (getProperty(error, 'translationKey')) {
throw error;
}
const errorMessage = `Failed during the form "${formDoc.internalId}" rendering : `;
throw new Error(errorMessage + error.message);
throw new Error(errorMessage + getProperty(error, 'message'));
}
}

Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/services/indexed-db.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Inject, Injectable } from '@angular/core';
import { DOCUMENT } from '@angular/common';

import { DbService } from '@mm-services/db.service';
import { getProperty } from '../libs/schema';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -84,7 +85,7 @@ export class IndexedDbService {
try {
localDoc = await this.loadingLocalDoc;
} catch (error) {
if (error.status !== 404) {
if (getProperty(error, 'status') !== 404) {
throw error;
}
console.debug('IndexedDbService :: Local doc not created yet. Ignoring error.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { default as generateReplicationId } from 'pouchdb-generate-replication-i

import { Migration } from './migration';
import { DbService } from '@mm-services/db.service';
import { getProperty } from '../../libs/schema';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -30,7 +31,7 @@ export class TargetCheckpointerMigration extends Migration {
try {
return await this.dbService.get().get(replicationId);
} catch (err) {
if (err?.status === 404) {
if (getProperty(err, 'status') === 404) {
return;
}
throw err;
Expand All @@ -48,7 +49,7 @@ export class TargetCheckpointerMigration extends Migration {
await this.dbService.get({ remote: true }).put(localDoc);
return true;
} catch (err) {
if (err?.status === 409) {
if (getProperty(err, 'status') === 409) {
// dont fail on conflicts
return true;
}
Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/services/telemetry.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { v4 as uuidv4 } from 'uuid';
import { DbService } from '@mm-services/db.service';
import { SessionService } from '@mm-services/session.service';
import { IndexedDbService } from '@mm-services/indexed-db.service';
import { getProperty } from '../libs/schema';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -196,7 +197,7 @@ export class TelemetryService {
.get({ meta: true })
.put(aggregateDoc);
} catch (error) {
if (error.status === 409) {
if (getProperty(error, 'status') === 409) {
return this.storeConflictedAggregate(aggregateDoc);
}
throw error;
Expand Down
3 changes: 2 additions & 1 deletion webapp/src/ts/services/user-contact.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Person, Qualifier } from '@medic/cht-datasource';
import { UserSettingsService } from '@mm-services/user-settings.service';
import { CHTDatasourceService } from '@mm-services/cht-datasource.service';
import { AuthService } from '@mm-services/auth.service';
import { getProperty } from '../libs/schema';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -47,7 +48,7 @@ export class UserContactService {
try {
return await this.userSettingsService.get();
} catch (err) {
if (err.code === 404) {
if (getProperty(err, 'code') === 404) {
return null;
}
throw err;
Expand Down
3 changes: 2 additions & 1 deletion webapp/tests/karma/js/enketo/medic-xpath-extensions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { expect } from 'chai';
import sinon from 'sinon';

import * as medicXpathExtensions from '../../../../src/js/enketo/medic-xpath-extensions.js';
import { getProperty } from '../../../../src/ts/libs/schema';

describe('Medic XPath Extensions', () => {
it('should have expected attributes', () => {
Expand Down Expand Up @@ -78,7 +79,7 @@ describe('Medic XPath Extensions', () => {
try {
extensionLib({ v: 'myfunc' }, { t: 'string', v: 'hello' });
} catch (e) {
expect(e.message).to.equal('Form configuration error: no extension-lib with ID "myfunc" found');
expect(getProperty(e, 'message')).to.equal('Form configuration error: no extension-lib with ID "myfunc" found');
return;
}
throw new Error('Expected exception to be thrown.');
Expand Down
3 changes: 2 additions & 1 deletion webapp/tests/karma/ts/providers/parse.provider.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { PhonePipe } from '@mm-pipes/phone.pipe';
import { FormatDateService } from '@mm-services/format-date.service';
import { RelativeDateService } from '@mm-services/relative-date.service';
import { XmlFormsContextUtilsService } from '@mm-services/xml-forms-context-utils.service';
import { getProperty } from '../../../../src/ts/libs/schema';

describe('Parse provider', () => {
let provider:ParseProvider;
Expand Down Expand Up @@ -47,7 +48,7 @@ describe('Parse provider', () => {
result = parse('2 ===== 3');
assert.fail('should have thrown');
} catch (e) {
expect(e.message.startsWith('Parser Error: Unexpected token')).to.equal(true);
expect((getProperty(e, 'message') as string).startsWith('Parser Error: Unexpected token')).to.equal(true);
expect(result).to.equal(undefined);
}
});
Expand Down
5 changes: 3 additions & 2 deletions webapp/tests/karma/ts/services/delete-docs.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { SessionService } from '@mm-services/session.service';
import { ChangesService } from '@mm-services/changes.service';
import { DeleteDocsService } from '@mm-services/delete-docs.service';
import { ExtractLineageService } from '@mm-services/extract-lineage.service';
import { getProperty } from '../../../../src/ts/libs/schema';

describe('DeleteDocs service', () => {

Expand Down Expand Up @@ -266,7 +267,7 @@ describe('DeleteDocs service', () => {
try {
JSON.stringify(report);
} catch (e) {
if (e.message.startsWith('Converting circular structure to JSON')) {
if ((getProperty(e, 'message') as string).startsWith('Converting circular structure to JSON')) {
isCircularBefore = true;
}
}
Expand All @@ -277,7 +278,7 @@ describe('DeleteDocs service', () => {
try {
JSON.stringify(bulkDocs.args[0][0][0]);
} catch (e) {
if (e.message.startsWith('Converting circular structure to JSON')) {
if ((getProperty(e, 'message') as string).startsWith('Converting circular structure to JSON')) {
isCircularAfter = true;
}
}
Expand Down
3 changes: 2 additions & 1 deletion webapp/tests/karma/ts/services/enketo.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { TranslateService } from '@mm-services/translate.service';
import { EnketoFormContext, EnketoService } from '@mm-services/enketo.service';
import { ExtractLineageService } from '@mm-services/extract-lineage.service';
import * as FileManager from '../../../../src/js/enketo/file-manager.js';
import { getProperty } from '../../../../src/ts/libs/schema';

describe('Enketo service', () => {
// return a mock form ready for putting in #dbContent
Expand Down Expand Up @@ -130,7 +131,7 @@ describe('Enketo service', () => {
expect.fail('Should throw error');
} catch (error) {
expect(enketoInit.callCount).to.equal(1);
expect(error.message).to.equal('["nope","still nope"]');
expect(getProperty(error, 'message')).to.equal('["nope","still nope"]');
}
}));

Expand Down
5 changes: 3 additions & 2 deletions webapp/tests/karma/ts/services/form.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { EnketoTranslationService } from '@mm-services/enketo-translation.servic
import * as FileManager from '../../../../src/js/enketo/file-manager.js';
import { TargetAggregatesService } from '@mm-services/target-aggregates.service';
import { ContactViewModelGeneratorService } from '@mm-services/contact-view-model-generator.service';
import { getProperty } from '../../../../src/ts/libs/schema';

describe('Form service', () => {
// return a mock form ready for putting in #dbContent
Expand Down Expand Up @@ -297,7 +298,7 @@ describe('Form service', () => {
{ doc: { _id: '123-patient-contact' }, contactSummary: { pregnant: false }, shouldEvaluateExpression: true },
]);
expect(enketoInit.callCount).to.equal(1);
expect(error.message).to.equal(expectedErrorMessage);
expect(getProperty(error, 'message')).to.equal(expectedErrorMessage);
expect(consoleErrorMock.callCount).to.equal(0);
}
}));
Expand Down Expand Up @@ -585,7 +586,7 @@ describe('Form service', () => {
flush();
expect.fail('Should throw error');
} catch (error) {
expect(error.message).to.equal('Failed during the form "myform" rendering : invalid user');
expect(getProperty(error, 'message')).to.equal('Failed during the form "myform" rendering : invalid user');
expect(UserContact.calledOnce).to.be.true;
expect(renderForm.notCalled).to.be.true;
expect(enketoInit.notCalled).to.be.true;
Expand Down
Loading
Loading