Repository: electron/fiddle
Pull request #613 (2021-03-20 16:17:55)

refactor: remove unnecessary 'async' from getIsDownloaded()
A minor refactor to `src/renderer/binary getIsDownloaded()`, which is async only because it uses fancy-import. However this isn't necessary in order for testing, so the code can be simplified a little by removing `async`.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/38108758/badge)](https://coveralls.io/builds/38108758)

Coverage increased (+0.01%) to 93.148% when pulling **ed4d8dba83d1bda1ef8839d432319607ad18752c on refactor/remove-superfluous-async-in-getIsDownloaded** into **adb4488a52df122faab135b6a2ae8f77161492d0 on master**.

ckerr: superceded by https://github.com/electron/fiddle/tree/refactor/use-esModuleInterop

Patch:
diff --git a/src/renderer/binary.ts b/src/renderer/binary.ts
index 2432e388..9af0bcab 100644
--- a/src/renderer/binary.ts
+++ b/src/renderer/binary.ts
@@ -1,13 +1,13 @@
-import * as fsType from 'fs-extra';
+import * as fs from 'fs-extra';
 import * as path from 'path';
 import extract from 'extract-zip';
 
 import { VersionState } from '../interfaces';
-import { fancyImport } from '../utils/import';
 import { normalizeVersion } from '../utils/normalize-version';
 import { USER_DATA_PATH } from './constants';
 import { removeTypeDefsForVersion } from './fetch-types';
 import { AppState } from './state';
+import { download as electronDownload } from '@electron/get';
 
 const pendingDownloads: Record<string, Promise<void>> = {};
 
@@ -18,17 +18,17 @@ const pendingDownloads: Record<string, Promise<void>> = {};
  * @param {string} iVersion
  * @returns {Promise<void>}
  */
-export async function setupBinary(
+export function setupBinary(
   appState: AppState,
   iVersion: string,
 ): Promise<void> {
   const version = normalizeVersion(iVersion);
 
   // If we already have it, then we're done
-  if (await getIsDownloaded(version)) {
+  if (getIsDownloaded(version)) {
     console.log(`Binary: Electron ${version} already downloaded.`);
     appState.versions[version].state = VersionState.ready;
-    return;
+    return Promise.resolve();
   }
 
   // Return a promise that resolves when the download completes
@@ -58,7 +58,6 @@ async function downloadBinary(
     appState.versions[version].state = VersionState.unzipping;
 
     // Ensure the target path exists
-    const fs = await fancyImport<typeof fsType>('fs-extra');
     await fs.mkdirp(extractPath);
 
     // Ensure the target path is empty
@@ -85,7 +84,6 @@ async function downloadBinary(
  */
 export async function removeBinary(iVersion: string) {
   const version = normalizeVersion(iVersion);
-  const fs = await fancyImport<typeof fsType>('fs-extra');
   let isDeleted = false;
 
   // utility to re-run removal functions upon failure
@@ -106,7 +104,7 @@ export async function removeBinary(iVersion: string) {
   };
 
   const binaryCleaner = async () => {
-    if (await getIsDownloaded(version)) {
+    if (getIsDownloaded(version)) {
       // This is necessary since we're messing with .asar files inside
       // the Electron binaries. Electron, powering Fiddle, will try to
       // "correct" our calls, but we don't want that right here.
@@ -135,13 +133,8 @@ export async function removeBinary(iVersion: string) {
  * @param {string} dir
  * @returns {boolean}
  */
-export async function getIsDownloaded(
-  version: string,
-  dir?: string,
-): Promise<boolean> {
+export function getIsDownloaded(version: string, dir?: string): boolean {
   const expectedPath = getElectronBinaryPath(version, dir);
-  const fs = await fancyImport<typeof fsType>('fs-extra');
-
   return fs.existsSync(expectedPath);
 }
 
@@ -183,23 +176,14 @@ export function getDownloadingVersions(appState: AppState) {
  * @returns {Promise<Array<string>>}
  */
 export async function getDownloadedVersions(): Promise<Array<string>> {
-  const fs = await fancyImport<typeof fsType>('fs-extra');
   const downloadPath = path.join(USER_DATA_PATH, 'electron-bin');
   console.log(`Binary: Checking for downloaded versions`);
 
   try {
-    const directories = await fs.readdir(downloadPath);
-    const knownVersions: Array<string> = [];
-
-    for (const directory of directories) {
-      if (await getIsDownloaded(directory)) {
-        knownVersions.push(directory);
-      }
-    }
-
-    return knownVersions;
+    const dirs = await fs.readdir(downloadPath);
+    return dirs.filter((dir) => dir && getIsDownloaded(dir));
   } catch (error) {
-    console.warn(`Could not read known Electron versions`);
+    console.warn(`Could not read known Electron versions`, error);
     return [];
   }
 }
@@ -212,7 +196,6 @@ export async function getDownloadedVersions(): Promise<Array<string>> {
  * @returns {Promise<string>}
  */
 async function download(appState: AppState, version: string): Promise<string> {
-  const { download: electronDownload } = await import('@electron/get');
   const getProgressCallback = (progress: Progress) => {
     const roundedProgress = Math.round(progress.percent * 100) / 100;
 
diff --git a/src/renderer/components/dialog-add-version.tsx b/src/renderer/components/dialog-add-version.tsx
index eea2d1d4..3f5776ae 100644
--- a/src/renderer/components/dialog-add-version.tsx
+++ b/src/renderer/components/dialog-add-version.tsx
@@ -70,9 +70,8 @@ export class AddVersionDialog extends React.Component<
    *
    * @param {React.ChangeEvent<HTMLInputElement>} event
    */
-  public async setFolderPath(folderPath: string) {
-    const isValidElectron = !!(await getIsDownloaded('custom', folderPath));
-
+  public setFolderPath(folderPath: string) {
+    const isValidElectron = getIsDownloaded('custom', folderPath);
     this.setState({ folderPath, isValidElectron });
   }
 
diff --git a/src/renderer/runner.ts b/src/renderer/runner.ts
index 6d616349..f0fa0c9f 100644
--- a/src/renderer/runner.ts
+++ b/src/renderer/runner.ts
@@ -71,7 +71,7 @@ export class Runner {
       return false;
     }
 
-    const isReady = await getIsDownloaded(version, localPath);
+    const isReady = getIsDownloaded(version, localPath);
 
     if (!isReady) {
       console.warn(`Runner: Binary ${version} not ready`);


Test patch:
diff --git a/tests/renderer/binary-spec.ts b/tests/renderer/binary-spec.ts
index 0528a369..c11e45c3 100644
--- a/tests/renderer/binary-spec.ts
+++ b/tests/renderer/binary-spec.ts
@@ -19,9 +19,10 @@ jest.mock('../../src/renderer/constants', () => ({
 }));
 jest.mock('../../src/utils/import', () => ({
   fancyImport: async (p: string) => {
-    if (p === 'fs-extra') {
-      return require('fs-extra');
+    if (p === 'extract-zip') {
+      return { default: require('extract-zip') };
     }
+    throw new Error(`Unexpected module '${p}'`);
   },
 }));
 jest.mock('../../src/renderer/fetch-types', () => ({
@@ -33,6 +34,7 @@ jest.mock('@electron/get', () => ({
 
 describe('binary', () => {
   let mockState: any = {};
+  const fs = require('fs-extra');
 
   beforeEach(() => {
     mockState = {
@@ -50,8 +52,6 @@ describe('binary', () => {
 
   describe('remove()', () => {
     it('removes a version', async () => {
-      const fs = require('fs-extra');
-
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
 
       await removeBinary('v3.0.0');
@@ -59,8 +59,6 @@ describe('binary', () => {
     });
 
     it('retries on failure', async () => {
-      const fs = require('fs-extra');
-
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
       (fs.remove as jest.Mock<any>).mockImplementation(() => {
         throw new Error('Bwap bwap');
@@ -71,8 +69,6 @@ describe('binary', () => {
     });
 
     it("attempts to clean up the version's associated typedefs", async () => {
-      const fs = require('fs-extra');
-
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
 
       await removeBinary('v3.0.0');
@@ -81,7 +77,6 @@ describe('binary', () => {
     });
 
     it('retries typedef cleanup upon failure', async () => {
-      const fs = require('fs-extra');
       (removeTypeDefsForVersion as jest.Mock).mockImplementation(() => {
         throw new Error('Bwap bwap');
       });
@@ -95,30 +90,22 @@ describe('binary', () => {
 
   describe('getDownloadedVersions()', () => {
     it('finds downloaded versions', async () => {
-      const fs = require('fs-extra');
-
-      (fs.readdir as jest.Mock<any>).mockReturnValue(['v3.0.0']);
+      const versions = ['v3.0.0'];
+      (fs.readdir as jest.Mock<any>).mockResolvedValue(versions);
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
-
       const result = await getDownloadedVersions();
-
-      expect(result).toEqual(['v3.0.0']);
+      expect(result).toEqual(versions);
     });
 
     it('is okay without versions ', async () => {
-      const fs = require('fs-extra');
-
-      (fs.readdir as jest.Mock<any>).mockReturnValue([]);
+      const versions: string[] = [];
+      (fs.readdir as jest.Mock<any>).mockResolvedValue(versions);
       const result = await getDownloadedVersions();
-      expect(result).toEqual([]);
+      expect(result).toEqual(versions);
     });
 
     it('is okay without versions and with errors', async () => {
-      const fs = require('fs-extra');
-
-      (fs.readdir as jest.Mock<any>).mockImplementationOnce(() => {
-        throw new Error('💩');
-      });
+      (fs.readdir as jest.Mock<any>).mockRejectedValueOnce('💩');
       const result = await getDownloadedVersions();
       expect(result).toEqual([]);
     });
@@ -165,40 +152,43 @@ describe('binary', () => {
   });
 
   describe('setupBinary()', () => {
-    it(`downloads a version it hasn't seen before`, async () => {
-      const { download } = require('@electron/get');
-      download.mockReturnValue('/fake/path');
+    const { download } = require('@electron/get');
 
-      mockState.versions['3.0.0'] = { state: 'unknown' };
-      await setupBinary(mockState, 'v3.0.0');
+    it(`downloads a version it has not seen before`, async () => {
+      (fs.existsSync as jest.Mock).mockReturnValueOnce(false);
+
+      const localPath = '/fake/path';
+      download.mockResolvedValue(localPath);
+
+      const version = '3.0.0';
+      mockState.versions[version] = { state: 'unknown' };
+      await setupBinary(mockState, version);
 
       expect(download).toHaveBeenCalled();
-      expect(mockState.versions['3.0.0'].state).toBe('ready');
+      expect(mockState.versions[version].state).toBe('ready');
     });
 
     it(`does not download a version again`, async () => {
-      const fs = require('fs-extra');
       (fs.existsSync as jest.Mock).mockReturnValue(true);
-      const { download } = require('@electron/get');
 
-      mockState.versions['3.0.0'] = { state: 'unknown' };
-      await setupBinary(mockState, 'v3.0.0');
+      const version = '3.0.0';
+      mockState.versions[version] = { state: 'unknown' };
+
+      await setupBinary(mockState, version);
 
-      expect(download).toHaveBeenCalledTimes(0);
-      expect(mockState.versions['3.0.0'].state).toBe('ready');
+      expect(download).not.toHaveBeenCalled();
+      expect(mockState.versions[version].state).toBe('ready');
     });
 
     it(`waits if asked for a version is already downloading`, async () => {
-      const fs = require('fs-extra');
-      const { download } = require('@electron/get');
-
       (fs.existsSync as jest.Mock).mockReturnValue(true);
-      mockState.versions['3.0.0'].state = 'downloading';
 
-      await setupBinary(mockState, 'v3.0.0');
+      const version = '3.0.0';
+      mockState.versions[version] = { state: 'downloading' };
+      await setupBinary(mockState, version);
 
-      expect(download).toHaveBeenCalledTimes(0);
-      expect(mockState.versions['3.0.0'].state).toBe('ready');
+      expect(download).not.toHaveBeenCalled();
+      expect(mockState.versions[version].state).toBe('ready');
     });
 
     it('handles an error in the zip file', async () => {
diff --git a/tests/renderer/components/dialog-add-version-spec.tsx b/tests/renderer/components/dialog-add-version-spec.tsx
index c6a7bca0..0fb38acd 100644
--- a/tests/renderer/components/dialog-add-version-spec.tsx
+++ b/tests/renderer/components/dialog-add-version-spec.tsx
@@ -29,9 +29,6 @@ describe('AddVersionDialog component', () => {
     store = {
       isAddVersionDialogShowing: true,
       addLocalVersion: jest.fn(),
-      binaryManager: {
-        getIsDownloaded: jest.fn(),
-      },
     };
   });
 
@@ -70,7 +67,7 @@ describe('AddVersionDialog component', () => {
 
   describe('setFolderPath()', () => {
     it('does something', async () => {
-      (getIsDownloaded as jest.Mock).mockResolvedValue(true);
+      (getIsDownloaded as jest.Mock).mockReturnValue(true);
       const wrapper = shallow(<AddVersionDialog appState={store} />);
       await (wrapper.instance() as any).setFolderPath('/test/');