refactor: steve review

Author: mshanemc

MIGRATING_V5-V6.md

@@ -6,7 +6,7 @@ We've had a series of bugs where files controlled by the config stack could lose
 
 V6 introducing a file locking mechanism previously only available on the `alias` file and a process of resolving conflicts based on timestamps.
 
-But that comes with breaking changes to to reduce the risk of "getting around" the safeties.
+But that comes with breaking changes to reduce the risk of "getting around" the safeties.
 
 ## Breaking changes related to the Config Stack
 

src/config/config.ts

@@ -387,7 +387,7 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {
 
     await config.read();
 
-    if (value == null || value === undefined) {
+    if (value == null) {
       config.unset(propertyName);
     } else {
       config.set(propertyName, value);
@@ -598,8 +598,8 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {
 }
 
 /**
- * If toOld is specified: migrate all deprecated configs back to their original key.
- * - For example, target-org will be renamed to defaultusername.
+ * convert from "new" to "old" config names
+ * - For example, `target-org` will be renamed to `defaultusername`.
  */
 const translateToSfdx = (sfContents: ConfigProperties): ConfigProperties =>
   Object.fromEntries(
@@ -610,8 +610,8 @@ const translateToSfdx = (sfContents: ConfigProperties): ConfigProperties =>
   );
 
 /**
- * If toOld is specified: migrate all deprecated configs to the new key.
- * - For example, target-org will be renamed to defaultusername.
+ * convert from "old" to "new" config names
+ * - For example, `defaultusername` will be renamed to `target-org`
  */
 const translateToSf = (sfdxContents: ConfigProperties, SfConfig: Config): ConfigProperties =>
   Object.fromEntries(
@@ -642,20 +642,23 @@ const writeToSfdx = async (path: string, contents: ConfigProperties): Promise<vo
     const translated = translateToSfdx(contents);
     await fs.promises.mkdir(pathDirname(path), { recursive: true });
     await fs.promises.writeFile(path, JSON.stringify(translated, null, 2));
-  } catch (error) {
-    /* Do nothing */
+  } catch (e) {
+    const logger = Logger.childFromRoot('core:config:writeToSfdx');
+    logger.debug(`Error writing to sfdx config file at ${path}: ${e instanceof Error ? e.message : ''}`);
   }
 };
 
 /** turn the sfdx config file into a LWWState based on its contents and its timestamp */
-const stateFromSfdxFileSync = (filePath: string, config: Config): LWWState<ConfigProperties> => {
+const stateFromSfdxFileSync = (path: string, config: Config): LWWState<ConfigProperties> => {
   try {
-    const fileContents = fs.readFileSync(filePath, 'utf8');
-    const mtimeNs = fs.statSync(filePath, { bigint: true }).mtimeNs;
-    const translatedContents = translateToSf(parseJsonMap<ConfigProperties>(fileContents, filePath), config);
+    const fileContents = fs.readFileSync(path, 'utf8');
+    const mtimeNs = fs.statSync(path, { bigint: true }).mtimeNs;
+    const translatedContents = translateToSf(parseJsonMap<ConfigProperties>(fileContents, path), config);
     // get the file timestamp
     return stateFromContents(translatedContents, mtimeNs);
   } catch (e) {
+    const logger = Logger.childFromRoot('core:config:stateFromSfdxFileSync');
+    logger.debug(`Error reading state from sfdx config file at ${path}: ${e instanceof Error ? e.message : ''}`);
     return {};
   }
 };

src/config/configFile.ts

@@ -254,7 +254,6 @@ export class ConfigFile<
         );
       }
 
-      // get the file modstamp.  Do this after the lock acquisition in case the file is being written to.
       const fileTimestamp = (await fs.promises.stat(this.getPath(), { bigint: true })).mtimeNs;
       const fileContents = parseJsonMap<P>(await fs.promises.readFile(this.getPath(), 'utf8'), this.getPath());
       this.logAndMergeContents(fileTimestamp, fileContents);
@@ -263,11 +262,13 @@ export class ConfigFile<
     }
     // write the merged LWWMap to file
     this.logger.debug(`Writing to config file: ${this.getPath()}`);
-    await fs.promises.writeFile(this.getPath(), JSON.stringify(this.getContents(), null, 2));
-
-    // unlock the file
-    if (typeof unlockFn !== 'undefined') {
-      await unlockFn();
+    try {
+      await fs.promises.writeFile(this.getPath(), JSON.stringify(this.getContents(), null, 2));
+    } finally {
+      // unlock the file
+      if (typeof unlockFn !== 'undefined') {
+        await unlockFn();
+      }
     }
 
     return this.getContents();
@@ -301,12 +302,15 @@ export class ConfigFile<
     // write the merged LWWMap to file
 
     this.logger.trace(`Writing to config file: ${this.getPath()}`);
-    fs.writeFileSync(this.getPath(), JSON.stringify(this.toObject(), null, 2));
-
-    if (typeof unlockFn !== 'undefined') {
-      // unlock the file
-      unlockFn();
+    try {
+      fs.writeFileSync(this.getPath(), JSON.stringify(this.toObject(), null, 2));
+    } finally {
+      if (typeof unlockFn !== 'undefined') {
+        // unlock the file
+        unlockFn();
+      }
     }
+
     return this.getContents();
   }