From bbce484da02f24aaed918706e6f7d0ea71cc911d Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 17 Aug 2021 22:06:27 +0200 Subject: [PATCH] Enforce type checking indexed accesses 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. --- nodecg-io-android/extension/android.ts | 2 +- nodecg-io-core/dashboard/bundles.ts | 19 +++++++++++----- .../extension/__tests__/bundleManager.ts | 9 +++++++- .../extension/__tests__/instanceManager.ts | 4 ++-- .../extension/__tests__/persistenceManager.ts | 8 +++---- nodecg-io-core/extension/bundleManager.ts | 7 +++--- nodecg-io-core/extension/instanceManager.ts | 10 ++++----- .../extension/persistenceManager.ts | 22 +++++-------------- nodecg-io-core/extension/service.ts | 2 +- .../extension/curseforgeClient.ts | 2 +- nodecg-io-debug/extension/debugHelper.ts | 13 +++++------ nodecg-io-reddit/tsconfig.json | 7 +++++- nodecg-io-serial/extension/SerialClient.ts | 2 +- .../extension/twitchAddonsClient.ts | 5 +++-- samples/curseforge/extension/index.ts | 2 +- samples/reddit-msg-read/tsconfig.json | 7 +++++- samples/streamdeck-rainbow/extension/index.ts | 6 ++--- samples/youtube-playlist/extension/index.ts | 2 +- tsconfig.common.json | 1 + 19 files changed, 73 insertions(+), 57 deletions(-) diff --git a/nodecg-io-android/extension/android.ts b/nodecg-io-android/extension/android.ts index 64ec80f61..6b02ce4ab 100644 --- a/nodecg-io-android/extension/android.ts +++ b/nodecg-io-android/extension/android.ts @@ -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}`, ); diff --git a/nodecg-io-core/dashboard/bundles.ts b/nodecg-io-core/dashboard/bundles.ts index c68749ea5..e2530b646 100644 --- a/nodecg-io-core/dashboard/bundles.ts +++ b/nodecg-io-core/dashboard/bundles.ts @@ -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; @@ -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) { @@ -86,6 +95,9 @@ export async function setSelectedServiceDependency(): Promise { 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); } @@ -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)); }); } diff --git a/nodecg-io-core/extension/__tests__/bundleManager.ts b/nodecg-io-core/extension/__tests__/bundleManager.ts index 977503e39..6b4d25dec 100644 --- a/nodecg-io-core/extension/__tests__/bundleManager.ts +++ b/nodecg-io-core/extension/__tests__/bundleManager.ts @@ -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(); }); @@ -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(); diff --git a/nodecg-io-core/extension/__tests__/instanceManager.ts b/nodecg-io-core/extension/__tests__/instanceManager.ts index 0ff53b808..a3fc0c084 100644 --- a/nodecg-io-core/extension/__tests__/instanceManager.ts +++ b/nodecg-io-core/extension/__tests__/instanceManager.ts @@ -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", () => { diff --git a/nodecg-io-core/extension/__tests__/persistenceManager.ts b/nodecg-io-core/extension/__tests__/persistenceManager.ts index b33ee25be..2328965b6 100644 --- a/nodecg-io-core/extension/__tests__/persistenceManager.ts +++ b/nodecg-io-core/extension/__tests__/persistenceManager.ts @@ -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); }); }); @@ -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); }); }); diff --git a/nodecg-io-core/extension/bundleManager.ts b/nodecg-io-core/extension/bundleManager.ts index 8f71cfae2..78beee0c1 100644 --- a/nodecg-io-core/extension/bundleManager.ts +++ b/nodecg-io-core/extension/bundleManager.ts @@ -133,10 +133,9 @@ export class BundleManager extends EventEmitter { */ handleInstanceUpdate(serviceInstance: ServiceInstance, 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); } diff --git a/nodecg-io-core/extension/instanceManager.ts b/nodecg-io-core/extension/instanceManager.ts index af1209368..60d8e4b0f 100644 --- a/nodecg-io-core/extension/instanceManager.ts +++ b/nodecg-io-core/extension/instanceManager.ts @@ -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; diff --git a/nodecg-io-core/extension/persistenceManager.ts b/nodecg-io-core/extension/persistenceManager.ts index 468454796..b39c902a4 100644 --- a/nodecg-io-core/extension/persistenceManager.ts +++ b/nodecg-io-core/extension/persistenceManager.ts @@ -144,14 +144,9 @@ export class PersistenceManager { * @param instances the service instances that should be loaded. */ private loadServiceInstances(instances: ObjectMap>): Promise[] { - 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}`, @@ -159,7 +154,7 @@ export class PersistenceManager { 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(); } @@ -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( @@ -190,13 +185,8 @@ export class PersistenceManager { * @param bundles the bundle dependencies that should be set. */ private loadBundleDependencies(bundles: ObjectMap[]>): 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. diff --git a/nodecg-io-core/extension/service.ts b/nodecg-io-core/extension/service.ts index 334b5ce44..416899b79 100644 --- a/nodecg-io-core/extension/service.ts +++ b/nodecg-io-core/extension/service.ts @@ -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 = Record; +export type ObjectMap = Record; /** * Models a service that a bundle can depend upon and use to access e.g. a twitch chat or similar. diff --git a/nodecg-io-curseforge/extension/curseforgeClient.ts b/nodecg-io-curseforge/extension/curseforgeClient.ts index d288ccb1e..e9c120acf 100644 --- a/nodecg-io-curseforge/extension/curseforgeClient.ts +++ b/nodecg-io-curseforge/extension/curseforgeClient.ts @@ -998,7 +998,7 @@ export class MagicValues { value: number | string, map: Record, inverse: Record, - ): number | string { + ): number | string | undefined { if (typeof value === "number") { return inverse[value]; } else { diff --git a/nodecg-io-debug/extension/debugHelper.ts b/nodecg-io-debug/extension/debugHelper.ts index 34390acbe..0d3526e8f 100644 --- a/nodecg-io-debug/extension/debugHelper.ts +++ b/nodecg-io-debug/extension/debugHelper.ts @@ -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 { diff --git a/nodecg-io-reddit/tsconfig.json b/nodecg-io-reddit/tsconfig.json index 1c8405620..814ff881f 100644 --- a/nodecg-io-reddit/tsconfig.json +++ b/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 + } } diff --git a/nodecg-io-serial/extension/SerialClient.ts b/nodecg-io-serial/extension/SerialClient.ts index 22fc5d57a..2252e21eb 100644 --- a/nodecg-io-serial/extension/SerialClient.ts +++ b/nodecg-io-serial/extension/SerialClient.ts @@ -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!"); diff --git a/nodecg-io-twitch-addons/extension/twitchAddonsClient.ts b/nodecg-io-twitch-addons/extension/twitchAddonsClient.ts index e5d5e5ee5..959172926 100644 --- a/nodecg-io-twitch-addons/extension/twitchAddonsClient.ts +++ b/nodecg-io-twitch-addons/extension/twitchAddonsClient.ts @@ -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); } } } diff --git a/samples/curseforge/extension/index.ts b/samples/curseforge/extension/index.ts index 38917ad30..74390df54 100644 --- a/samples/curseforge/extension/index.ts +++ b/samples/curseforge/extension/index.ts @@ -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 = { diff --git a/samples/reddit-msg-read/tsconfig.json b/samples/reddit-msg-read/tsconfig.json index c8bb01bee..af3030ef7 100644 --- a/samples/reddit-msg-read/tsconfig.json +++ b/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 + } } diff --git a/samples/streamdeck-rainbow/extension/index.ts b/samples/streamdeck-rainbow/extension/index.ts index 94cfc27c5..eed14a194 100644 --- a/samples/streamdeck-rainbow/extension/index.ts +++ b/samples/streamdeck-rainbow/extension/index.ts @@ -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) { diff --git a/samples/youtube-playlist/extension/index.ts b/samples/youtube-playlist/extension/index.ts index 028482954..b97fcaa15 100644 --- a/samples/youtube-playlist/extension/index.ts +++ b/samples/youtube-playlist/extension/index.ts @@ -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( diff --git a/tsconfig.common.json b/tsconfig.common.json index f3d0ab772..f5078ddd2 100644 --- a/tsconfig.common.json +++ b/tsconfig.common.json @@ -19,6 +19,7 @@ "noImplicitThis": true, "strictNullChecks": true, "skipLibCheck": true, + "noUncheckedIndexedAccess": true, "module": "CommonJS" },