Code Review Comments Typescript

useTranslation()

Translations are now being translated in the common translation file.

We're using react-i18next for localisations. Collections of translatable items are grouped into namespaces so that we can reduce bundle sizes and keep files contained to specific areas.

useTranslation() with plurals

When translating strings, it's common to need a singular and plural option. This can be accomplished using a _one and _other suffix on the translation key and passing a parameter to the translation request called 'count'.

Example

common.json

{
  "permissions_one": "Permission",
  "permissions_other": "Permissions"
}

example.ts

const t = useTranslation();

let permissionList = ['Permission a'];

let translation = t("permissions", {count: permissionList.length});
// translation will contain 'Permission' 

let permissionList = ['Permission a', 'Permission b'];

let translation = t("permissions", {count: permissionList.length});
// translation will now contain 'Permissions' 

Avoid using type assertions ('as' keyword)

Use of type assertion should be avoided, it can cause runtime errors in what looks like safe code. If type assertions have to be used, the resulting type should be consumed entirely in the same scope (with maximum amount of safety checks):

const myCanvas = document.getElementById("main_canvas") as HTMLCanvasElement;
if (!myCanvas) throw new Error("no canvas");
if (!myCanvas.getContext) throw new Error("not a canvas");

const gl = myCanvas.getContext("webgl", {
  antialias: false,
  depth: false,
});

Example of why type assertions can be dangerous

interface Check {
   id: string,
   willBeMissing: string
}

// ..

const doCheck = (check: Check) => {
   console.log(check.id)
   // ..
   helper(check)
}

const helper = (check: Check) => {
   console.log(check.willBeMissing.length) // runtime error (if check not Check)
}

// .. 

const getLooksLikeCheck = () => ({ id: 'one' });

doCheck(getLooksLikeCheck() as Check) // runtime error

Union alternative to the above (with minimum changes)

Imports

If using co-pilot ( and possibly other helpers 🤷 ), you may have an auto generated import appear which has the format 'packages/..' Do not use these! While it will work as a standard runtime import, it will cause jest tests to fail.

Prefer

import { InternalSupplierSearchModal, NameRowFragment } from '@openmsupply-client/system';

Instead of

import { InternalSupplierSearchModal, InternalSupplierSearchModal, NameRowFragment, NameRowFragment } from 'packages/system/src';

You can generally find an aliased path to use instead, or a reference which is relative to the current file.

When exporting from packages, if an item is to be used outside of the package, export it from the package itself. This may require exporting from the root index.ts of the package and back up the tree of index files.

Prefer

import { InternalSupplierSearchModal, NameRowFragment } from '@openmsupply-client/system';

Instead of

import { NameRowFragment } from '@openmsupply-client/system/src/Name/api';

This makes is clear exactly what is being exported, and therefore depended on, out of the package. It's also neater ( personal opinion! ) and less fragile. If you are exporting from the root of the package then the internal structure can safely change.