Skip to content

Commit

Permalink
Enforce type checking indexed accesses
Browse files Browse the repository at this point in the history
Without explicit config TypeScript allows to access a element of a object or array by just accessing it using a index.
But in reality you might get back `undefined` because the object doesn't contain anything with that key or the array isn't long enough.
To force these checks I've typed `ObjectMap` in a way that all values might always be undefined.
This is not good because you might use e.g. `Object.entries` to iterate
a object when using it as a map that ensures you'll only get entries with set values.

TypeScript has a built in solution to this: `noUncheckedIndexedAccess`.
When this compiler setting is enabled it will ensure that you'll check values you get from index access for `undefined`.
This also allows us to use `Object.entries` to get rid of a lot of redudant undefined checks which makes the code a bit cleaner.
Also forces services to do the same because it is nicer to get a error that you might get `undefined` instead of the user catching this
because we never used it in a way that produces `undefined` and the typings didn't contain it.

Also enabling this has brought up some issues with the dashboard code that could have resulted in using `undefined` as a object key.
  • Loading branch information
hlxid committed Aug 17, 2021
1 parent 74a8001 commit bbce484
Show file tree
Hide file tree
Showing 19 changed files with 73 additions and 57 deletions.
2 changes: 1 addition & 1 deletion nodecg-io-android/extension/android.ts
Expand Up @@ -1291,7 +1291,7 @@ export class SmsManager {
addressStr = address.address;
} else if (address instanceof Contact) {
const phone_numbers = await address.getData("phone");
if (phone_numbers.length <= 0) {
if (phone_numbers.length <= 0 || phone_numbers[0] === undefined) {
throw new Error(
`Can't use contact as sms receiver: No phone number available for contact ${address.displayName}`,
);
Expand Down
19 changes: 14 additions & 5 deletions nodecg-io-core/dashboard/bundles.ts
Expand Up @@ -30,6 +30,10 @@ export function renderBundleDeps(): void {
}

const bundle = selectBundle.options[selectBundle.selectedIndex]?.value;
if (bundle === undefined) {
return;
}

const bundleDependencies = config.data.bundles[bundle];
if (bundleDependencies === undefined) {
return;
Expand Down Expand Up @@ -69,9 +73,14 @@ export function renderInstanceSelector(): void {

// Selecting option of current set instance
const bundle = selectBundle.options[selectBundle.selectedIndex]?.value;
if (bundle === undefined) {
return;
}

const currentInstance = config.data.bundles[bundle]?.find(
(dep) => dep.serviceType === serviceType,
)?.serviceInstance;

let index = 0;
for (let i = 0; i < selectBundleInstance.options.length; i++) {
if (selectBundleInstance.options.item(i)?.value === currentInstance) {
Expand All @@ -86,6 +95,9 @@ export async function setSelectedServiceDependency(): Promise<void> {
const bundle = selectBundle.options[selectBundle.selectedIndex]?.value;
const instance = selectBundleInstance.options[selectBundleInstance.selectedIndex]?.value;
const type = selectBundleDepTypes.options[selectBundleDepTypes.selectedIndex]?.value;
if (bundle === undefined || type === undefined) {
return;
}

await setServiceDependency(bundle, instance === "none" ? undefined : instance, type);
}
Expand All @@ -96,11 +108,8 @@ export function unsetAllBundleDependencies(): void {
return;
}

Object.keys(bundles).forEach((bundleName) => {
const b = bundles[bundleName];
b?.forEach((dep) => {
setServiceDependency(bundleName, undefined, dep.serviceType);
});
Object.entries(bundles).forEach(([bundleName, dependencies]) => {
dependencies.forEach((dep) => setServiceDependency(bundleName, undefined, dep.serviceType));
});
}

Expand Down
9 changes: 8 additions & 1 deletion nodecg-io-core/extension/__tests__/bundleManager.ts
Expand Up @@ -107,7 +107,7 @@ describe("BundleManager", () => {

expect(res).toBe(true);
expect(provider?.getClient()).toBeUndefined();
expect(bundleManager.getBundleDependencies()[testBundle]?.[0].serviceInstance).toBeUndefined();
expect(bundleManager.getBundleDependencies()[testBundle]?.[0]?.serviceInstance).toBeUndefined();
expect(changeCb).toHaveBeenCalled();
});

Expand All @@ -119,6 +119,13 @@ describe("BundleManager", () => {
expect(changeCb).not.toHaveBeenCalled();
});

test("should return false if bundle doesn't exists", () => {
// Bundle has not registered any dependencies and is unknown to BundleManager
const res = bundleManager.unsetServiceDependency(testBundle, testService.serviceType);
expect(res).toBe(false);
expect(changeCb).not.toHaveBeenCalled();
});

test("should trigger re-registering handlers of previous service instance", () => {
registerTestServiceDep();
const reRegisterCb = jest.fn();
Expand Down
4 changes: 2 additions & 2 deletions nodecg-io-core/extension/__tests__/instanceManager.ts
Expand Up @@ -169,8 +169,8 @@ describe("InstanceManager", () => {
expect(instanceManager.deleteServiceInstance(testInstance)).toBe(true);

const deps = bundleManager.getBundleDependencies();
expect(deps[testBundle]?.[0].serviceInstance).toBeUndefined();
expect(deps[testBundle2]?.[0].serviceInstance).toBe(testInstance2);
expect(deps[testBundle]?.[0]?.serviceInstance).toBeUndefined();
expect(deps[testBundle2]?.[0]?.serviceInstance).toBe(testInstance2);
});

test("should error service instance doesn't exists", () => {
Expand Down
8 changes: 4 additions & 4 deletions nodecg-io-core/extension/__tests__/persistenceManager.ts
Expand Up @@ -151,8 +151,8 @@ describe("PersistenceManager", () => {
expect(deps).toBeDefined();
if (!deps) return;
expect(deps).toHaveLength(1);
expect(deps[0].serviceType).toBe(testService.serviceType);
expect(deps[0].serviceInstance).toBe(testInstance);
expect(deps[0]?.serviceType).toBe(testService.serviceType);
expect(deps[0]?.serviceInstance).toBe(testInstance);
});
});

Expand Down Expand Up @@ -184,8 +184,8 @@ describe("PersistenceManager", () => {

expect(data.result.instances[testInstance]?.serviceType).toBe(testService.serviceType);
expect(data.result.instances[testInstance]?.config).toBe(testService.defaultConfig);
expect(data.result.bundleDependencies[testBundle]?.[0].serviceType).toBe(testService.serviceType);
expect(data.result.bundleDependencies[testBundle]?.[0].serviceInstance).toBe(testInstance);
expect(data.result.bundleDependencies[testBundle]?.[0]?.serviceType).toBe(testService.serviceType);
expect(data.result.bundleDependencies[testBundle]?.[0]?.serviceInstance).toBe(testInstance);
});
});

Expand Down
7 changes: 3 additions & 4 deletions nodecg-io-core/extension/bundleManager.ts
Expand Up @@ -133,10 +133,9 @@ export class BundleManager extends EventEmitter {
*/
handleInstanceUpdate(serviceInstance: ServiceInstance<unknown, unknown>, instName: string): void {
// Iterate over all bundles
Object.keys(this.bundles).forEach((bundle) => {
// Get their dependencies and if they have this instance set somewhere then update the bundle.
const svcDependencies = this.bundles[bundle];
svcDependencies?.forEach((dep) => {
Object.entries(this.bundles).forEach(([_bundleName, svcDependencies]) => {
// If they have this instance set somewhere in their dependencies then update the bundle.
svcDependencies.forEach((dep) => {
if (dep.serviceInstance === instName) {
dep.provider.updateClient(serviceInstance.client);
}
Expand Down
10 changes: 5 additions & 5 deletions nodecg-io-core/extension/instanceManager.ts
Expand Up @@ -118,11 +118,11 @@ export class InstanceManager extends EventEmitter {

// Remove any assignment of a bundle to this service instance
const deps = this.bundles.getBundleDependencies();
Object.keys(deps).forEach(
(bundle) =>
deps[bundle]
?.filter((d) => d.serviceInstance === instanceName) // Search for bundle dependencies using this instance
.forEach((d) => this.bundles.unsetServiceDependency(bundle, d.serviceType)), // unset all these
Object.entries(deps).forEach(
([bundleName, deps]) =>
deps
.filter((d) => d.serviceInstance === instanceName) // Search for bundle dependencies using this instance
.forEach((d) => this.bundles.unsetServiceDependency(bundleName, d.serviceType)), // unset all these
);

return true;
Expand Down
22 changes: 6 additions & 16 deletions nodecg-io-core/extension/persistenceManager.ts
Expand Up @@ -144,22 +144,17 @@ export class PersistenceManager {
* @param instances the service instances that should be loaded.
*/
private loadServiceInstances(instances: ObjectMap<ServiceInstance<unknown, unknown>>): Promise<void>[] {
return Object.keys(instances).map((instanceName) => {
const inst = instances[instanceName];
if (inst === undefined) {
return Promise.resolve();
}

return Object.entries(instances).map(([instanceName, instance]) => {
// Re-create service instance.
const result = this.instances.createServiceInstance(inst.serviceType, instanceName);
const result = this.instances.createServiceInstance(instance.serviceType, instanceName);
if (result.failed) {
this.nodecg.log.info(
`Couldn't load instance "${instanceName}" from saved configuration: ${result.errorMessage}`,
);
return Promise.resolve();
}

const svc = this.services.getService(inst.serviceType);
const svc = this.services.getService(instance.serviceType);
if (!svc.failed && svc.result.requiresNoConfig) {
return Promise.resolve();
}
Expand All @@ -169,7 +164,7 @@ export class PersistenceManager {
// before getting saved to disk.
// This results in faster loading when the validation takes time, e.g. makes HTTP requests.
return this.instances
.updateInstanceConfig(instanceName, inst.config, false)
.updateInstanceConfig(instanceName, instance.config, false)
.then((result) => {
if (result.failed) {
this.nodecg.log.info(
Expand All @@ -190,13 +185,8 @@ export class PersistenceManager {
* @param bundles the bundle dependencies that should be set.
*/
private loadBundleDependencies(bundles: ObjectMap<ServiceDependency<unknown>[]>): void {
Object.keys(bundles).forEach((bundleName) => {
if (!Object.prototype.hasOwnProperty.call(bundles, bundleName)) {
return;
}

const deps = bundles[bundleName];
deps?.forEach((svcDep) => {
Object.entries(bundles).forEach(([bundleName, deps]) => {
deps.forEach((svcDep) => {
// Re-setting bundle service dependencies.
// We can ignore the case of undefined, because the default is that the bundle doesn't get any service
// which is modeled by undefined. We are assuming that there was nobody setting it to something different.
Expand Down
2 changes: 1 addition & 1 deletion nodecg-io-core/extension/service.ts
Expand Up @@ -12,7 +12,7 @@ import { ServiceProvider } from "./serviceProvider";
* A normal es6 map would use a iterator which can't be serialized by the NodeCG Replicant and thus
* can't be used to give the gui access to the data in this map.
*/
export type ObjectMap<V> = Record<string, V | undefined>;
export type ObjectMap<V> = Record<string, V>;

/**
* Models a service that a bundle can depend upon and use to access e.g. a twitch chat or similar.
Expand Down
2 changes: 1 addition & 1 deletion nodecg-io-curseforge/extension/curseforgeClient.ts
Expand Up @@ -998,7 +998,7 @@ export class MagicValues {
value: number | string,
map: Record<string, number>,
inverse: Record<number, string>,
): number | string {
): number | string | undefined {
if (typeof value === "number") {
return inverse[value];
} else {
Expand Down
13 changes: 6 additions & 7 deletions nodecg-io-debug/extension/debugHelper.ts
Expand Up @@ -56,13 +56,12 @@ export class DebugHelper extends EventEmitter {

private static hexToRGB(hex: string): Color {
const result = /^#?([a-f\d]{2})([a-f\d]{2})([a-f\d]{2})$/i.exec(hex);
return result
? {
red: parseInt(result[1], 16),
green: parseInt(result[2], 16),
blue: parseInt(result[3], 16),
}
: { red: 0, green: 0, blue: 0 };

return {
red: result?.[1] ? parseInt(result[1] ?? "0", 16) : 0,
green: result?.[2] ? parseInt(result[2] ?? "0", 16) : 0,
blue: result?.[3] ? parseInt(result[3] ?? "0", 16) : 0,
};
}

static createClient(nodecg: NodeCG): DebugHelper {
Expand Down
7 changes: 6 additions & 1 deletion nodecg-io-reddit/tsconfig.json
@@ -1,3 +1,8 @@
{
"extends": "../tsconfig.common.json"
"extends": "../tsconfig.common.json",
"compilerOptions": {
// reddit-ts uses unchecked index accesses and we need to disable it here
// to be able to use reddit-ts.
"noUncheckedIndexedAccess": false
}
}
2 changes: 1 addition & 1 deletion nodecg-io-serial/extension/SerialClient.ts
Expand Up @@ -66,7 +66,7 @@ export class SerialServiceClient extends SerialPort {
}

// Check if result isn't empty or ambiguous
if (result.length < 1) {
if (result[0] === undefined) {
return error("No device matched the provided criteria!");
} else if (result.length > 1) {
return error("The provided criteria were ambiguous!");
Expand Down
5 changes: 3 additions & 2 deletions nodecg-io-twitch-addons/extension/twitchAddonsClient.ts
Expand Up @@ -77,8 +77,9 @@ export class TwitchAddonsClient {
const ffzGlobalSets: FFZEmoteSet[] = [];
if (ffzGlobal !== undefined) {
for (const set of ffzGlobal.default_sets) {
if (set.toString() in ffzGlobal.sets) {
ffzGlobalSets.push(ffzGlobal.sets[set.toString()]);
const setObj = ffzGlobal.sets[set.toString()];
if (setObj !== undefined) {
ffzGlobalSets.push(setObj);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion samples/curseforge/extension/index.ts
Expand Up @@ -14,7 +14,7 @@ module.exports = function (nodecg: NodeCG) {
const addons: CurseAddon[] = await client.getMultipleAddons(addonIds);
nodecg.log.info("Here are the projects which belongs to the ids:");
addons.forEach((addon) => {
nodecg.log.info(`- '${addon.info.name}' (${addon.info.gameName} addon) by ${addon.info.authors[0].name}`);
nodecg.log.info(`- '${addon.info.name}' (${addon.info.gameName} addon) by ${addon.info.authors[0]?.name}`);
});

const query = {
Expand Down
7 changes: 6 additions & 1 deletion samples/reddit-msg-read/tsconfig.json
@@ -1,3 +1,8 @@
{
"extends": "../../tsconfig.common.json"
"extends": "../../tsconfig.common.json",
"compilerOptions": {
// reddit-ts, which is the underlying library of nodecg-io-reddit doesn't support compiling with
// this option. We need to disable it to make this sample bundle compile.
"noUncheckedIndexedAccess": false
}
}
6 changes: 3 additions & 3 deletions samples/streamdeck-rainbow/extension/index.ts
Expand Up @@ -34,9 +34,9 @@ module.exports = function (nodecg: NodeCG) {
try {
deck.fillColor(
i % deck.NUM_KEYS,
colors[i % colors.length][0],
colors[i % colors.length][1],
colors[i % colors.length][2],
colors[i % colors.length]?.[0] ?? 0,
colors[i % colors.length]?.[1] ?? 0,
colors[i % colors.length]?.[2] ?? 0,
);
i += 1;
} catch (err) {
Expand Down
2 changes: 1 addition & 1 deletion samples/youtube-playlist/extension/index.ts
Expand Up @@ -15,7 +15,7 @@ module.exports = function (nodecg: NodeCG) {
id: ["PL9oBXB6tQnlX013V1v20WkfzI9R2zamHi"],
});
const items = resp.data.items;
if (items) {
if (items && items[0]) {
const { title, channelTitle, publishedAt, description } = items[0]
.snippet as youtube_v3.Schema$PlaylistItemSnippet;
nodecg.log.info(
Expand Down
1 change: 1 addition & 0 deletions tsconfig.common.json
Expand Up @@ -19,6 +19,7 @@
"noImplicitThis": true,
"strictNullChecks": true,
"skipLibCheck": true,
"noUncheckedIndexedAccess": true,

"module": "CommonJS"
},
Expand Down

0 comments on commit bbce484

Please sign in to comment.