feat/type-safe: Encourage type-safe coding practices with eslint-plug…

…in-typesafe (#943)

* test: incorporate typesafe/no-throw-sync-func rule.

* refactor: remove need for assertUnreachable in switch statements as can simply rely on type checking against the function return signature. For instance, type checks will fail with  "Not all code paths return a value" in the case below:

```typescript
type MyNumber = 1 | 2

function test(x: MyNumber) {
    switch(x) {
        case 1:
            return 1
    }
}
```

* allow throws from createCustomLogger due to logger imports from JS files
* allows validateS3BucketUrl to throw exceptions because of convict integration
* excludes FeatureManager from no-throw-sync-func

Co-authored-by: Yuanruo Liang <[email protected]>

.eslintrc

@@ -24,7 +24,7 @@
         },
         "project": "./tsconfig.json"
       },
-      "plugins": ["@typescript-eslint", "import", "simple-import-sort"],
+      "plugins": ["@typescript-eslint", "import", "simple-import-sort", "typesafe"],
       "extends": ["plugin:@typescript-eslint/recommended"],
       "rules": {
         // Rules for auto sort of imports
@@ -54,7 +54,8 @@
         "import/newline-after-import": "error",
         "import/no-duplicates": "error",
         "@typescript-eslint/no-floating-promises": 2,
-        "@typescript-eslint/no-unused-vars": 2
+        "@typescript-eslint/no-unused-vars": 2,
+        "typesafe/no-throw-sync-func": "error"
       }
     },
     { "files": ["*.spec.ts"], "extends": ["plugin:jest/recommended"] }

package.json

@@ -215,6 +215,7 @@
     "eslint-plugin-jest": "^24.1.3",
     "eslint-plugin-prettier": "^3.3.0",
     "eslint-plugin-simple-import-sort": "^6.0.1",
+    "eslint-plugin-typesafe": "^0.2.0",
     "form-data": "^3.0.0",
     "google-fonts-plugin": "4.1.0",
     "html-loader": "~0.5.5",

src/app/models/form_statistics_total.server.model.ts

@@ -41,8 +41,8 @@ const compileFormStatisticsTotalModel = (db: Mongoose) => {
   // Hooks
   FormStatisticsTotalSchema.pre<IFormStatisticsTotalSchema>(
     'save',
-    function () {
-      throw new Error('FormStatisticsTotal schema is read only')
+    function (next) {
+      next(new Error('FormStatisticsTotal schema is read only'))
     },
   )
 

src/app/modules/form/admin-form/admin-form.utils.ts

@@ -3,7 +3,6 @@ import { err, ok, Result } from 'neverthrow'
 
 import { createLoggerWithLabel } from '../../../../config/logger'
 import { IPopulatedForm, ResponseMode, Status } from '../../../../types'
-import { assertUnreachable } from '../../../utils/assert-unreachable'
 import {
   ApplicationError,
   DatabaseConflictError,
@@ -205,8 +204,6 @@ export const getAssertPermissionFn = (level: PermissionLevel): AssertFormFn => {
       return assertHasWritePermissions
     case PermissionLevel.Delete:
       return assertHasDeletePermissions
-    default:
-      return assertUnreachable(level)
   }
 }
 

src/app/modules/form/form.service.ts

@@ -4,7 +4,6 @@ import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow'
 import { createLoggerWithLabel } from '../../../config/logger'
 import { IFormSchema, IPopulatedForm, Status } from '../../../types'
 import getFormModel from '../../models/form.server.model'
-import { assertUnreachable } from '../../utils/assert-unreachable'
 import { getMongoErrorMessage } from '../../utils/handle-mongo-error'
 import { ApplicationError, DatabaseError } from '../core/core.errors'
 
@@ -116,7 +115,5 @@ export const isFormPublic = (
       return err(new FormDeletedError())
     case Status.Private:
       return err(new PrivateFormError(form.inactiveMessage, form.title))
-    default:
-      return assertUnreachable(form.status)
   }
 }

src/app/modules/submission/__tests__/submission/submission.utils.spec.ts

@@ -44,14 +44,5 @@ describe('submission.utils', () => {
       )
       expect(actual.length).toEqual(2)
     })
-
-    it('should throw error if called with invalid responseMode', async () => {
-      // Arrange
-      const invalidResponseMode = 'something' as ResponseMode
-      // Act + Assert
-      expect(() => getModeFilter(invalidResponseMode)).toThrowError(
-        `This should never be reached in TypeScript: "${invalidResponseMode}"`,
-      )
-    })
   })
 })

src/app/modules/submission/submission.utils.ts

@@ -3,7 +3,6 @@ import { keyBy } from 'lodash'
 import { BasicField, IFieldSchema, ResponseMode } from '../../../types'
 import { isEmailField } from '../../../types/field/utils/guards'
 import { AutoReplyMailData } from '../../services/mail/mail.types'
-import { assertUnreachable } from '../../utils/assert-unreachable'
 import { FIELDS_TO_REJECT } from '../../utils/field-validation/config'
 
 import { ProcessedFieldResponse } from './submission.types'
@@ -20,8 +19,6 @@ export const getModeFilter = (
       return emailModeFilter
     case ResponseMode.Encrypt:
       return encryptModeFilter
-    default:
-      return assertUnreachable(responseMode)
   }
 }
 

src/app/modules/verification/verification.service.ts

@@ -332,5 +332,7 @@ const getFieldFromTransaction = (
 const throwError = (message: string, name?: string): never => {
   const error = new Error(message)
   error.name = name || message
+  // TODO(#941) Convert this service to use neverthrow, and re-examine type assertions made
+  // eslint-disable-next-line
   throw error
 }

src/app/services/sms/sms.factory.ts

@@ -65,9 +65,11 @@ export const createSmsFactory = (
     const errorMessage = 'SMS feature must be enabled in Feature Manager first'
     return {
       sendAdminContactOtp: () => {
+        //eslint-disable-next-line
         throw new Error(`sendAdminContactOtp: ${errorMessage}`)
       },
       sendVerificationOtp: () => {
+        //eslint-disable-next-line
         throw new Error(`sendVerificationOtp: ${errorMessage}`)
       },
       sendFormDeactivatedSms: () =>

src/config/feature-manager/util/FeatureManager.class.ts

@@ -14,6 +14,10 @@ const logger = createLoggerWithLabel(module)
 convict.addFormat(validator.url)
 
 export default class FeatureManager {
+  /* eslint-disable typesafe/no-throw-sync-func
+    --------
+    This class bootstraps the application and should
+    cause a crash in case of errors. */
   public states: Partial<Record<FeatureNames, boolean>>
   // Map some feature names to some env vars
   private properties: Partial<IFeatureManager>
@@ -115,4 +119,5 @@ export default class FeatureManager {
       props: this.props<K>(name),
     }
   }
+  /* eslint-enable typesafe/no-throw-sync-func */
 }

src/config/logger.ts

@@ -197,6 +197,7 @@ const createCustomLogger = (logger: Logger) => {
         const { message, meta } = params
         // Not the expected shape, throw to catch block.
         if (!message || isEmpty(meta)) {
+          //eslint-disable-next-line
           throw new Error('Wrong shape')
         }
         return logger.info(message, { meta })
@@ -209,6 +210,7 @@ const createCustomLogger = (logger: Logger) => {
         const { message, meta, error } = params
         // Not the expected shape, throw to catch block.
         if (!message || isEmpty(meta)) {
+          //eslint-disable-next-line
           throw new Error('Wrong shape')
         }
         if (error) {
@@ -224,6 +226,7 @@ const createCustomLogger = (logger: Logger) => {
         const { message, meta, error } = params
         // Not the expected shape, throw to catch block.
         if (!message || isEmpty(meta)) {
+          //eslint-disable-next-line
           throw new Error('Wrong shape')
         }
         if (error) {

src/config/schema.ts

@@ -34,14 +34,17 @@ convict.addFormat({
 /**
  * Verifies that S3 bucket url is a valid url with or without trailing slash
  */
-const validateBucketUrl = (
+const validateS3BucketUrl = (
   val: string,
   {
     isDev,
     hasTrailingSlash,
     region,
   }: { isDev: boolean; hasTrailingSlash: boolean; region: string },
 ) => {
+  /* eslint-disable typesafe/no-throw-sync-func
+    --------
+    The convict package expects format validation functions to throw Errors */
   if (!validator.isURL(val, { require_tld: !isDev })) {
     throw new Error('must be a url')
   }
@@ -59,6 +62,7 @@ const validateBucketUrl = (
   if (!isDev && !isRegionCorrect.test(val)) {
     throw new Error(`region should be ${region}`)
   }
+  /* eslint-enable typesafe/no-throw-sync-func */
 }
 
 // If the default value does not match the format specified, the configuration built from this schema
@@ -352,27 +356,27 @@ export const loadS3BucketUrlSchema = ({
     endPoint: {
       doc: 'Endpoint for S3 buckets',
       format: (val) =>
-        validateBucketUrl(val, { isDev, hasTrailingSlash: false, region }),
+        validateS3BucketUrl(val, { isDev, hasTrailingSlash: false, region }),
       default: 'https://s3.ap-southeast-1.amazonaws.com', // NOTE NO TRAILING / AT THE END OF THIS URL!
       env: 'AWS_ENDPOINT',
     },
     attachmentBucketUrl: {
       doc:
         'Url of attachment S3 bucket derived from S3 endpoint and bucket name',
       format: (val) =>
-        validateBucketUrl(val, { isDev, hasTrailingSlash: true, region }),
+        validateS3BucketUrl(val, { isDev, hasTrailingSlash: true, region }),
       default: null,
     },
     logoBucketUrl: {
       doc: 'Url of logo S3 bucket derived from S3 endpoint and bucket name',
       format: (val) =>
-        validateBucketUrl(val, { isDev, hasTrailingSlash: false, region }),
+        validateS3BucketUrl(val, { isDev, hasTrailingSlash: false, region }),
       default: null,
     },
     imageBucketUrl: {
       doc: 'Url of images S3 bucket derived from S3 endpoint and bucket name',
       format: (val) =>
-        validateBucketUrl(val, { isDev, hasTrailingSlash: false, region }),
+        validateS3BucketUrl(val, { isDev, hasTrailingSlash: false, region }),
       default: null,
     },
   }