Merge pull request #2461 from Automattic/fix/sonarcloud

fix: code smells detected by SonarCloud

Author: sjinks

__tests__/lib/cli/config.js

@@ -1,8 +1,12 @@
+import fs from 'node:fs';
+
+import { loadConfigFile } from '../../../src/lib/cli/config';
+
 describe( 'utils/cli/config', () => {
-	beforeEach( () => {
-		jest.resetModules();
-		jest.clearAllMocks();
+	afterEach( () => {
+		jest.restoreAllMocks();
 	} );
+
 	it.each( [
 		{
 			description: 'should return development if config.local.json is present',
@@ -17,28 +21,26 @@ describe( 'utils/cli/config', () => {
 		{
 			description: 'should throw error if config.local.json and config.publish.json are missing',
 			files: { local: false, publish: false },
-			expected: Error,
+			expected: null,
 		},
 	] )( '$description', ( { files, expected } ) => {
-		// An array of files would've been nicer but it doesn't play well with jest.doMock
-		if ( ! files.local ) {
-			jest.doMock( '../../../config/config.local.json', () => {
-				throw new Error();
-			} );
-		}
-		if ( ! files.publish ) {
-			jest.doMock( '../../../config/config.publish.json', () => {
+		const origReadFileSync = fs.readFileSync;
+		jest.spyOn( fs, 'readFileSync' ).mockImplementation( ( filePath, ...params ) => {
+			if ( typeof filePath !== 'string' || ! filePath.includes( 'config.' ) ) {
+				return origReadFileSync( filePath, ...params );
+			}
+
+			if (
+				( filePath.includes( 'config.local.json' ) && ! files.local ) ||
+				( filePath.includes( 'config.publish.json' ) && ! files.publish )
+			) {
 				throw new Error();
-			} );
-		}
+			}
+
+			return JSON.stringify( expected );
+		} );
 
-		if ( ! files.local && ! files.publish ) {
-			// eslint-disable-next-line jest/no-conditional-expect
-			expect( () => require( '../../../src/lib/cli/config' ) ).toThrow( expected );
-		} else {
-			const config = require( '../../../src/lib/cli/config' );
-			// eslint-disable-next-line jest/no-conditional-expect
-			expect( config.default ).toMatchObject( expected );
-		}
+		const actual = loadConfigFile();
+		expect( actual ).toStrictEqual( expected );
 	} );
 } );

src/bin/vip-app.js

@@ -70,10 +70,10 @@ command( { requiredArgs: 1, format: true } )
 		await trackEvent( 'app_command_success' );
 
 		// Clone the read-only response object so we can modify it
-		const clonedResponse = Object.assign( {}, res );
+		const clonedResponse = { ...res };
 
 		clonedResponse.environments = clonedResponse.environments.map( env => {
-			const clonedEnv = Object.assign( {}, env );
+			const clonedEnv = { ...env };
 
 			clonedEnv.name = getEnvIdentifier( env );
 

src/bin/vip-import-sql.js

@@ -113,8 +113,8 @@ function isValidMd5( md5 ) {
 }
 
 /**
- * @param {AppForImport} app
- * @param {EnvForImport} env
+ * @param {import('../lib/site-import/db-file-import').AppForImport} app
+ * @param {import('../lib/site-import/db-file-import').EnvForImport} env
  * @param {string} fileNameOrURL
  * @param {boolean} isUrl
  * @param {string|null} md5
@@ -427,7 +427,7 @@ const displayPlaybook = ( {
 	}
 };
 
-void command( {
+command( {
 	appContext: true,
 	appQuery,
 	envContext: true,

src/commands/phpmyadmin.ts

@@ -148,13 +148,13 @@ export class PhpMyAdminCommand {
 		this.progressTracker.stopPrinting();
 	}
 
-	public async openUrl( url: string ): Promise< void > {
+	public async openUrl( url: string ): Promise< unknown > {
 		const { default: open } = await import( 'open' );
-		void open( url, { wait: false } );
+		return open( url, { wait: false } );
 	}
 
 	public async getStatus(): Promise< string > {
-		return await getPhpMyAdminStatus( this.app.id as number, this.env.id as number );
+		return getPhpMyAdminStatus( this.app.id as number, this.env.id as number );
 	}
 
 	private async maybeEnablePhpMyAdmin(): Promise< void > {

src/lib/cli/command.js

@@ -374,21 +374,19 @@ args.argv = async function ( argv, cb ) {
 	}
 
 	// Negotiate flag values
-	switch ( _opts.module ) {
-		case 'import-media':
-			if ( [ true, 'true', 'yes' ].includes( options.saveErrorLog ) ) {
-				options.saveErrorLog = 'true';
-			} else if ( [ false, 'false', 'no' ].includes( options.saveErrorLog ) ) {
-				options.saveErrorLog = 'false';
-			} else {
-				options.saveErrorLog = 'prompt';
-			}
-			break;
+	if ( _opts.module === 'import-media' ) {
+		if ( [ true, 'true', 'yes' ].includes( options.saveErrorLog ) ) {
+			options.saveErrorLog = 'true';
+		} else if ( [ false, 'false', 'no' ].includes( options.saveErrorLog ) ) {
+			options.saveErrorLog = 'false';
+		} else {
+			options.saveErrorLog = 'prompt';
+		}
 	}
 
 	// Prompt for confirmation if necessary
 	if ( _opts.requireConfirm && ! options.force ) {
-		/** @type {Tuple[]} */
+		/** @type {import('./format').Tuple[]} */
 		const info = [];
 
 		if ( options.app ) {

src/lib/cli/config.ts

@@ -1,4 +1,6 @@
 import debugLib from 'debug';
+import { readFileSync } from 'node:fs'; // I don't like using synchronous versions, but until we migrate to ESM, we have to.
+import path from 'node:path';
 
 interface Config {
 	tracksUserType: string;
@@ -9,19 +11,35 @@ interface Config {
 
 const debug = debugLib( '@automattic/vip:lib:cli:config' );
 
-let configFromFile: Config;
-try {
-	// Get `local` config first; this will only exist in dev as it's npmignore-d.
-	// eslint-disable-next-line @typescript-eslint/no-var-requires
-	configFromFile = require( '../../../config/config.local.json' ) as Config;
+export function loadConfigFile(): Config | null {
+	const paths = [
+		// Get `local` config first; this will only exist in dev as it's npmignore-d.
+		path.join( __dirname, '../../../config/config.local.json' ),
+		path.join( __dirname, '../../../config/config.publish.json' ),
+	];
 
-	debug( 'Loaded config data from config.local.json' );
-} catch {
-	// Fall back to `publish` config file.
-	// eslint-disable-next-line @typescript-eslint/no-var-requires
-	configFromFile = require( '../../../config/config.publish.json' ) as Config;
+	for ( const filePath of paths ) {
+		try {
+			const data = readFileSync( filePath, 'utf-8' );
+			debug( `Found config file at ${ filePath }` );
+			return JSON.parse( data ) as Config;
+		} catch ( err ) {
+			if ( ! ( err instanceof Error ) || ! ( 'code' in err ) || err.code !== 'ENOENT' ) {
+				debug( `Error reading config file at ${ filePath }:`, err );
+			}
+		}
+	}
 
-	debug( 'Loaded config data from config.publish.json' );
+	return null;
 }
 
-export default configFromFile;
+const configFromFile = loadConfigFile();
+if ( null === configFromFile ) {
+	// This should not happen because `config/config.publish.json` is always present.
+	console.error( 'FATAL ERROR: Could not find a valid configuration file' );
+	process.exit( 1 );
+}
+
+// Without this, TypeScript will export `configFromFile` as `Config | null`
+const exportedConfig: Config = configFromFile;
+export default exportedConfig;

src/lib/cli/format.ts

@@ -118,17 +118,10 @@ export function keyValue( values: Tuple[] ): string {
 
 	for ( const { key, value } of values ) {
 		let formattedValue: string;
-
-		switch (
-			key.toLowerCase() // NOSONAR
-		) {
-			case 'environment':
-				formattedValue = formatEnvironment( value );
-				break;
-
-			default:
-				formattedValue = value;
-				break;
+		if ( key.toLowerCase() === 'environment' ) {
+			formattedValue = formatEnvironment( value );
+		} else {
+			formattedValue = value;
 		}
 
 		lines.push( `+ ${ key }: ${ formattedValue }` );

src/lib/config/software.ts

@@ -441,7 +441,8 @@ export const formatSoftwareSettings = (
 			.map( option => option.value );
 
 		if ( format !== 'json' ) {
-			result.available_versions = result.available_versions.sort().join( ',' );
+			result.available_versions.sort( ( lhs, rhs ) => lhs.localeCompare( rhs ) );
+			result.available_versions = result.available_versions.join( ',' );
 		}
 	}
 

src/lib/keychain.ts

@@ -4,19 +4,20 @@ import Insecure from './keychain/insecure';
 
 import type { Keychain, KeychainConstructor } from './keychain/keychain';
 
-let exportValue: Keychain;
+let keychain: Keychain;
 const debug = debugLib( '@automattic/vip:keychain' );
 
 try {
 	// Try using Secure keychain ("keytar") first
 	// eslint-disable-next-line @typescript-eslint/no-var-requires
 	const Secure = require( './keychain/secure' ) as KeychainConstructor;
-	exportValue = new Secure();
+	keychain = new Secure();
 } catch ( error ) {
 	debug( 'Cannot use Secure keychain; falling back to Insecure keychain (Details: %o)', error );
 
 	// Fallback to Insecure keychain if we can't
-	exportValue = new Insecure( 'vip-go-cli' );
+	keychain = new Insecure( 'vip-go-cli' );
 }
 
-export default exportValue;
+const exportedKeychain = keychain;
+export default exportedKeychain;

src/lib/validations/line-by-line.ts

@@ -65,7 +65,7 @@ export async function fileLineValidations(
 	return Promise.all(
 		validations.map( ( validation: PerLineValidationObject ) => {
 			if (
-				Object.prototype.hasOwnProperty.call( validation, 'postLineExecutionProcessing' ) &&
+				Object.hasOwn( validation, 'postLineExecutionProcessing' ) &&
 				typeof validation.postLineExecutionProcessing === 'function'
 			) {
 				return validation.postLineExecutionProcessing( {