Merge pull request #1021 from forcedotcom/sm/get-can-be-undefined

refactor: configStore `get` can be undefined

src/config/configStore.ts

@@ -20,7 +20,9 @@ import { ConfigContents, ConfigEntry, ConfigValue, Key } from './configStackType
 export interface ConfigStore<P extends ConfigContents = ConfigContents> {
   // Map manipulation methods
   entries(): ConfigEntry[];
+  // NEXT_RELEASE: update types to specify return can be P[K] | undefined
   get<K extends Key<P>>(key: K, decrypt: boolean): P[K];
+  // NEXT_RELEASE: update types to specify return can be T | undefined
   get<T extends ConfigValue>(key: string, decrypt: boolean): T;
   getKeysByValue(value: ConfigValue): Array<Key<P>>;
   has(key: string): boolean;
@@ -88,8 +90,12 @@ 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.
    */
+  // NEXT_RELEASE: update types to specify return can be  | undefined
   public get<K extends Key<P>>(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<V = ConfigValue>(key: string, decrypt?: boolean): V;
+  // NEXT_RELEASE: update types to specify return can be  | undefined
   public get<K extends Key<P>>(key: K | string, decrypt = false): P[K] | ConfigValue {
     const rawValue = this.contents.get(key as K);
 
@@ -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;
   }
 

src/org/scratchOrgCreate.ts

@@ -110,9 +110,12 @@ export const scratchOrgResume = async (jobId: string): Promise<ScratchOrgCreateR
     emit({ stage: 'send request' }),
   ]);
   logger.debug(`resuming scratch org creation for jobId: ${jobId}`);
-  if (!cache.has(jobId)) {
+  const cached = cache.get(jobId);
+
+  if (!cached) {
     throw messages.createError('CacheMissError', [jobId]);
   }
+
   const {
     hubUsername,
     apiVersion,
@@ -122,7 +125,7 @@ export const scratchOrgResume = async (jobId: string): Promise<ScratchOrgCreateR
     alias,
     setDefault,
     tracksSource,
-  } = cache.get(jobId);
+  } = cached;
 
   const hubOrg = await Org.create({ aliasOrUsername: hubUsername });
   const soi = await queryScratchOrgInfo(hubOrg, jobId);

test/unit/config/configStoreTest.ts

@@ -4,7 +4,7 @@
  * Licensed under the BSD 3-Clause license.
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
-import { expect } from 'chai';
+import { expect, assert } from 'chai';
 import { AuthInfoConfig } from '../../../src/config/authInfoConfig';
 import { BaseConfigStore } from '../../../src/config/configStore';
 import { ConfigContents } from '../../../src/config/configStackTypes';
@@ -53,9 +53,9 @@ describe('ConfigStore', () => {
     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);
     });
   });
 });

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;
     });
   });