From 4dad8c75e38252d2ecafe1a4349cc5b0d19d68ac Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 24 Jan 2024 13:52:18 -0600 Subject: [PATCH 1/2] fix: correct typings for configStore `get` --- src/config/configStore.ts | 10 ++++---- src/org/scratchOrgCreate.ts | 7 ++++-- test/unit/config/configStoreTest.ts | 38 +++++++++++++++++------------ test/unit/config/ttlConfigTest.ts | 4 +-- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/config/configStore.ts b/src/config/configStore.ts index 010f1ea2c4..dd313ba26e 100644 --- a/src/config/configStore.ts +++ b/src/config/configStore.ts @@ -20,8 +20,8 @@ import { ConfigContents, ConfigEntry, ConfigValue, Key } from './configStackType export interface ConfigStore

{ // Map manipulation methods entries(): ConfigEntry[]; - get>(key: K, decrypt: boolean): P[K]; - get(key: string, decrypt: boolean): T; + get>(key: K, decrypt: boolean): P[K] | undefined; + get(key: string, decrypt: boolean): T | undefined; getKeysByValue(value: ConfigValue): Array>; has(key: string): boolean; keys(): Array>; @@ -88,9 +88,9 @@ export abstract class BaseConfigStore< * @param decrypt If it is an encrypted key, decrypt the value. * If the value is an object, a clone will be returned. */ - public get>(key: K, decrypt?: boolean): P[K]; - public get(key: string, decrypt?: boolean): V; - public get>(key: K | string, decrypt = false): P[K] | ConfigValue { + public get>(key: K, decrypt?: boolean): P[K] | undefined; + public get(key: string, decrypt?: boolean): V | undefined; + public get>(key: K | string, decrypt = false): P[K] | ConfigValue | undefined { const rawValue = this.contents.get(key as K); if (this.hasEncryption() && decrypt) { diff --git a/src/org/scratchOrgCreate.ts b/src/org/scratchOrgCreate.ts index 414a8fcc9c..4a09a6e283 100644 --- a/src/org/scratchOrgCreate.ts +++ b/src/org/scratchOrgCreate.ts @@ -110,9 +110,12 @@ export const scratchOrgResume = async (jobId: string): Promise { const config = new TestConfig<{ '1': { a: string } }>(); config.set('1', { a: 'a' }); - config.get('1').a = 'b'; + config.get('1')!.a = 'b'; - expect(config.get('1').a).to.equal('b'); + expect(config.get('1')!.a).to.equal('b'); }); it('updates the object reference', async () => { @@ -64,8 +64,13 @@ describe('ConfigStore', () => { config.update('1', { b: 'c' }); - expect(config.get('1').a).to.equal('a'); - expect(config.get('1').b).to.equal('c'); + expect(config.get('1')!.a).to.equal('a'); + expect(config.get('1')!.b).to.equal('c'); + }); + + it('undefined keys return undefined', async () => { + const config = new TestConfig<{ '1': { a: string } }>(); + expect(config.get('not-a-thing')).to.equal(undefined); }); describe('encryption', () => { @@ -111,9 +116,9 @@ describe('ConfigStore', () => { }); const owner = config.get('owner'); // encrypted - expect(owner.creditCardNumber).to.not.equal(expected); + expect(owner?.creditCardNumber).to.not.equal(expected); // decrypted - expect(config.get('owner', true).creditCardNumber).to.equal(expected); + expect(config.get('owner', true)?.creditCardNumber).to.equal(expected); }); it('encrypts nested key using regexp', async () => { @@ -129,9 +134,9 @@ describe('ConfigStore', () => { }); const owner = config.get('owner'); // encrypted - expect(owner.superPassword).to.not.equal(expected); + expect(owner?.superPassword).to.not.equal(expected); // decrypted - expect(config.get('owner', true).superPassword).to.equal(expected); + expect(config.get('owner', true)?.superPassword).to.equal(expected); }); it('decrypt returns copies', async () => { @@ -142,11 +147,12 @@ describe('ConfigStore', () => { config.set('owner', owner); const decryptedOwner = config.get('owner', true); + assert(decryptedOwner); // Because we retrieved an decrypted object on a config with encryption, // it should return a clone so it doesn't accidentally save decrypted data. decryptedOwner.creditCardNumber = 'invalid'; - expect(config.get('owner').creditCardNumber).to.not.equal('invalid'); - expect(config.get('owner', true).creditCardNumber).to.equal(expected); + expect(config.get('owner')?.creditCardNumber).to.not.equal('invalid'); + expect(config.get('owner', true)?.creditCardNumber).to.equal(expected); }); // Ensures accessToken and refreshToken are both decrypted upon config.get() @@ -170,13 +176,13 @@ describe('ConfigStore', () => { const owner = { name: 'Bob', creditCardNumber: expected }; // @ts-expect-error incomplete owner config.set('owner', owner); - const encryptedCreditCardNumber = config.get('owner').creditCardNumber; + const encryptedCreditCardNumber = config.get('owner')?.creditCardNumber; const contents = config.getContents(); contents.owner.name = 'Tim'; // @ts-expect-error private method config.setContents(contents); - expect(config.get('owner').name).to.equal(contents.owner.name); - expect(config.get('owner').creditCardNumber).to.equal(encryptedCreditCardNumber); + expect(config.get('owner')?.name).to.equal(contents.owner.name); + expect(config.get('owner')?.creditCardNumber).to.equal(encryptedCreditCardNumber); }); it('updates encrypted object', async () => { @@ -188,8 +194,8 @@ describe('ConfigStore', () => { config.update('owner', { creditCardNumber: expected }); - expect(config.get('owner').name).to.equal(owner.name); - expect(config.get('owner', true).creditCardNumber).to.equal(expected); + expect(config.get('owner')?.name).to.equal(owner.name); + expect(config.get('owner', true)?.creditCardNumber).to.equal(expected); }); }); }); diff --git a/test/unit/config/ttlConfigTest.ts b/test/unit/config/ttlConfigTest.ts index 59bf3823e9..144b5bd8d3 100644 --- a/test/unit/config/ttlConfigTest.ts +++ b/test/unit/config/ttlConfigTest.ts @@ -85,14 +85,14 @@ describe('TTLConfig', () => { it('should return true if timestamp is older than TTL', async () => { const config = await TestConfig.create(); config.set('1', { one: 'one' }); - const isExpired = config.isExpired(new Date().getTime() + Duration.days(7).milliseconds, config.get('1')); + const isExpired = config.isExpired(new Date().getTime() + Duration.days(7).milliseconds, config.get('1')!); expect(isExpired).to.be.true; }); it('should return false if timestamp is not older than TTL', async () => { const config = await TestConfig.create(); config.set('1', { one: 'one' }); - const isExpired = config.isExpired(new Date().getTime(), config.get('1')); + const isExpired = config.isExpired(new Date().getTime(), config.get('1')!); expect(isExpired).to.be.false; }); }); From 7cb8fe96053f5d0e34c9acc7c0bdb2f385ba8f0b Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 24 Jan 2024 16:11:22 -0600 Subject: [PATCH 2/2] refactor: revert breaking change, leave as comments for next major --- src/config/configStore.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/config/configStore.ts b/src/config/configStore.ts index dd313ba26e..70a9913f93 100644 --- a/src/config/configStore.ts +++ b/src/config/configStore.ts @@ -20,8 +20,10 @@ import { ConfigContents, ConfigEntry, ConfigValue, Key } from './configStackType export interface ConfigStore

{ // Map manipulation methods entries(): ConfigEntry[]; - get>(key: K, decrypt: boolean): P[K] | undefined; - get(key: string, decrypt: boolean): T | undefined; + // NEXT_RELEASE: update types to specify return can be P[K] | undefined + get>(key: K, decrypt: boolean): P[K]; + // NEXT_RELEASE: update types to specify return can be T | undefined + get(key: string, decrypt: boolean): T; getKeysByValue(value: ConfigValue): Array>; has(key: string): boolean; keys(): Array>; @@ -88,9 +90,13 @@ export abstract class BaseConfigStore< * @param decrypt If it is an encrypted key, decrypt the value. * If the value is an object, a clone will be returned. */ - public get>(key: K, decrypt?: boolean): P[K] | undefined; - public get(key: string, decrypt?: boolean): V | undefined; - public get>(key: K | string, decrypt = false): P[K] | ConfigValue | undefined { + // NEXT_RELEASE: update types to specify return can be | undefined + public get>(key: K, decrypt?: boolean): P[K]; + // NEXT_RELEASE: update types to specify return can be | undefined + // NEXT_RELEASE: consider getting rid of ConfigValue and letting it just use the Key<> approach + public get(key: string, decrypt?: boolean): V; + // NEXT_RELEASE: update types to specify return can be | undefined + public get>(key: K | string, decrypt = false): P[K] | ConfigValue { const rawValue = this.contents.get(key as K); if (this.hasEncryption() && decrypt) { @@ -100,6 +106,7 @@ export abstract class BaseConfigStore< return this.decrypt(rawValue) as P[K] | ConfigValue; } } + // NEXT_RELEASE: update types to specify return can be | undefined return rawValue as P[K] | ConfigValue; }