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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+134 −43
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e4305e
chore: enable `useUnknownInCatchVariables` rule in tsconfig
jkuester Nov 14, 2024
77ab33e
Fix linting issues
jkuester Nov 14, 2024
e7e4239
Fix type issues in unit tests
jkuester Nov 14, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Member

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.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { GlobalActions } from '@mm-actions/global';
import { LineageModelGeneratorService } from '@mm-services/lineage-model-generator.service';
import { PerformanceService } from '@mm-services/performance.service';
import { ExtractLineageService } from '@mm-services/extract-lineage.service';
import { getProperty } from '../../libs/schema';

@Component({
templateUrl: './tasks.component.html',
Expand Down Expand Up @@ -154,7 +155,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions webapp/src/ts/services/migrations/target-checkpointer.migration.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,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 { getProperty } from '../libs/schema';

@Injectable({
providedIn: 'root'
Expand Down Expand Up @@ -31,7 +32,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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