Repository: electron/fiddle
Pull request #655 (2021-04-12 19:47:45)

refactor: replace `iVersion` params with `RunnableVersion` when removing & downloading versions
A small followup to https://github.com/electron/fiddle/pull/654. No functional changes. Removes some unnecessary code + logic branches.

In these three functions:

1. removeBinary()
2. removeVersion()
3. downloadVersion()

the caller has a RunnableVersion obtained from `appState` but, since the functions all take a version string parameter instead, `RunnableVersion.version` is passed in. Then the called functions normalize the string, do a lookup in appState.versions, and more importantly, do some error-handling to gracefully handle situations where the version isn't recognized by appState.

All of that goes away if we replace the iVersion parameter with the RunnableVersion obtained from appState. The string checking + lookup isn't expensive, but the error-handling (e.g. downloadVersion()'s code to "if it's not already in appState.versions, insert it") is only ever reached by the unit tests; never in production.

Side note: there is another change in `handleDeleteAll()`: We no longer need to normalize + compare strings to ensure that we're not removing the currently-selected Electron version. appState.removeVersion() [already safeguards against this](https://github.com/electron/fiddle/blob/fdbd8c0301fac2877c752b9953b25ba011fdb18a/src/renderer/state.ts#L487) so it's not necessary to do it here too.

*edit:* This PR has no new untested lines / branches. Coverage dropped because this PR removes more than it adds.

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

Coverage decreased (-0.02%) to 94.032% when pulling **29dd18417728ada1996020ed289b86588c6b15df on prefer-Version-to-specify-a-version** into **fdbd8c0301fac2877c752b9953b25ba011fdb18a on master**.


Patch:
diff --git a/src/renderer/binary.ts b/src/renderer/binary.ts
index 34808ff5..6cb82f19 100644
--- a/src/renderer/binary.ts
+++ b/src/renderer/binary.ts
@@ -8,7 +8,6 @@ import {
   VersionSource,
   VersionState,
 } from '../interfaces';
-import { normalizeVersion } from '../utils/normalize-version';
 import { USER_DATA_PATH } from './constants';
 import { removeTypeDefsForVersion } from './fetch-types';
 import { download as electronDownload } from '@electron/get';
@@ -45,7 +44,7 @@ export function getVersionState(ver: Version): VersionState {
  * General setup, called with a version. Is called during construction
  * to ensure that we always have or download at least one version.
  *
- * @param {string} iVersion
+ * @param {RunnableVersion} ver
  * @returns {Promise<void>}
  */
 export function setupBinary(ver: RunnableVersion): Promise<void> {
@@ -102,11 +101,11 @@ async function downloadBinary(ver: RunnableVersion): Promise<void> {
  * Remove a version from disk. Does not update state. We'll try up to
  * four times before giving up if an error occurs.
  *
- * @param {string} iVersion
+ * @param {RunnableVersion} ver
  * @returns {Promise<void>}
  */
-export async function removeBinary(iVersion: string) {
-  const version = normalizeVersion(iVersion);
+export async function removeBinary(ver: RunnableVersion) {
+  const { version } = ver;
   let isDeleted = false;
 
   // utility to re-run removal functions upon failure
@@ -146,6 +145,7 @@ export async function removeBinary(iVersion: string) {
   await rerunner(binaryCleaner);
 
   if (isDeleted) {
+    ver.state = VersionState.unknown;
     await rerunner(typeDefsCleaner);
   }
 }
diff --git a/src/renderer/components/settings-electron.tsx b/src/renderer/components/settings-electron.tsx
index 7c2c653a..f33a57ce 100644
--- a/src/renderer/components/settings-electron.tsx
+++ b/src/renderer/components/settings-electron.tsx
@@ -19,7 +19,6 @@ import {
   VersionSource,
   VersionState,
 } from '../../interfaces';
-import { normalizeVersion } from '../../utils/normalize-version';
 import { AppState } from '../state';
 import { getReleaseChannel, getOldestSupportedVersion } from '../versions';
 
@@ -112,8 +111,8 @@ export class ElectronSettings extends React.Component<
 
     const { downloadVersion, versionsToShow } = this.props.appState;
 
-    for (const { version } of versionsToShow) {
-      await downloadVersion(version);
+    for (const ver of versionsToShow) {
+      await downloadVersion(ver);
     }
 
     this.setState({ isDownloadingAll: false });
@@ -127,13 +126,10 @@ export class ElectronSettings extends React.Component<
   public async handleDeleteAll(): Promise<void> {
     this.setState({ isDeletingAll: true });
 
-    const { versions, removeVersion, version } = this.props.appState;
+    const { versions, removeVersion } = this.props.appState;
 
-    for (const key in versions) {
-      // If this isn't the currently selected version, remove it
-      if (normalizeVersion(key) !== normalizeVersion(version)) {
-        await removeVersion(key);
-      }
+    for (const ver of Object.values(versions)) {
+      await removeVersion(ver);
     }
 
     this.setState({ isDeletingAll: false });
@@ -338,11 +334,11 @@ export class ElectronSettings extends React.Component<
    *
    * @private
    * @param {string} key
-   * @param {RunnableVersion} item
+   * @param {RunnableVersion} ver
    * @returns {JSX.Element}
    */
   private renderAction(ver: RunnableVersion): JSX.Element {
-    const { state, source, version } = ver;
+    const { state, source } = ver;
     const { appState } = this.props;
     const buttonProps: IButtonProps = {
       fill: true,
@@ -352,7 +348,7 @@ export class ElectronSettings extends React.Component<
     switch (state) {
       case VersionState.ready:
         buttonProps.icon = 'trash';
-        buttonProps.onClick = () => appState.removeVersion(version);
+        buttonProps.onClick = () => appState.removeVersion(ver);
         buttonProps.text = source === VersionSource.local ? 'Remove' : 'Delete';
         break;
 
@@ -368,7 +364,7 @@ export class ElectronSettings extends React.Component<
         buttonProps.disabled = false;
         buttonProps.icon = 'cloud-download';
         buttonProps.loading = false;
-        buttonProps.onClick = () => appState.downloadVersion(version);
+        buttonProps.onClick = () => appState.downloadVersion(ver);
         buttonProps.text = 'Download';
         break;
     }
diff --git a/src/renderer/state.ts b/src/renderer/state.ts
index 50725e7f..29c41847 100644
--- a/src/renderer/state.ts
+++ b/src/renderer/state.ts
@@ -31,7 +31,7 @@ import {
 import { getName } from '../utils/get-name';
 import { normalizeVersion } from '../utils/normalize-version';
 import { isEditorBackup, isEditorId, isPanelId } from '../utils/type-checks';
-import { getVersionState, removeBinary, setupBinary } from './binary';
+import { removeBinary, setupBinary } from './binary';
 import { Bisector } from './bisect';
 import { DEFAULT_CLOSED_PANELS, DEFAULT_MOSAIC_ARRANGEMENT } from './constants';
 import { getTemplate, isContentUnchanged } from './content';
@@ -491,14 +491,14 @@ export class AppState {
    * @param {string} input
    * @returns {Promise<void>}
    */
-  @action public async removeVersion(input: string) {
-    const ver = this.getVersion(input);
-    if (ver?.state !== VersionState.ready) {
-      console.log('State: Version already removed, doing nothing');
+  @action public async removeVersion(ver: RunnableVersion) {
+    const { version } = ver;
+
+    if (ver.state !== VersionState.ready) {
+      console.log(`State: Version ${version} already removed, doing nothing`);
       return;
     }
 
-    const { version } = ver;
     if (ver === this.currentElectronVersion) {
       console.log(`State: Not removing active version ${version}`);
       return;
@@ -510,8 +510,7 @@ export class AppState {
       delete this.versions[version];
       saveLocalVersions(Object.values(this.versions));
     } else {
-      await removeBinary(version);
-      ver.state = VersionState.unknown;
+      await removeBinary(ver);
     }
   }
 
@@ -521,23 +520,11 @@ export class AppState {
    * @param {string} input
    * @returns {Promise<void>}
    */
-  @action public async downloadVersion(input: string) {
-    let ver = this.getVersion(input);
+  @action public async downloadVersion(ver: RunnableVersion) {
+    const { source, state, version } = ver;
 
-    // ensure the version is tracked in 'this.versions'
-    if (!ver) {
-      ver = {
-        source: VersionSource.remote,
-        state: VersionState.unknown,
-        version: normalizeVersion(input),
-      };
-      ver.state = getVersionState(ver);
-      this.versions[ver.version] = ver;
-    }
-
-    const isRemote = ver.source === VersionSource.remote;
-    const isReady = ver.state === VersionState.ready;
-    const { version } = ver;
+    const isRemote = source === VersionSource.remote;
+    const isReady = state === VersionState.ready;
     if (!isRemote || isReady) {
       console.log(`State: Already have version ${version}; not downloading.`);
       return;
@@ -607,7 +594,7 @@ export class AppState {
     await updateEditorTypeDefinitions(ver);
 
     // Fetch new binaries, maybe?
-    await this.downloadVersion(version);
+    await this.downloadVersion(ver);
   }
 
   /**


Test patch:
diff --git a/tests/renderer/binary-spec.ts b/tests/renderer/binary-spec.ts
index 780a9c7a..a7bdead5 100644
--- a/tests/renderer/binary-spec.ts
+++ b/tests/renderer/binary-spec.ts
@@ -54,7 +54,7 @@ describe('binary', () => {
     it('removes a version', async () => {
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
 
-      await removeBinary(version);
+      await removeBinary(ver);
       expect(fs.remove).toHaveBeenCalled();
     });
 
@@ -64,14 +64,14 @@ describe('binary', () => {
         throw new Error('Bwap bwap');
       });
 
-      await removeBinary(version);
+      await removeBinary(ver);
       expect(fs.remove).toHaveBeenCalledTimes(4);
     });
 
     it("attempts to clean up the version's associated typedefs", async () => {
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
 
-      await removeBinary(version);
+      await removeBinary(ver);
       expect(fs.remove).toHaveBeenCalled();
       expect(removeTypeDefsForVersion).toHaveBeenCalled();
     });
@@ -83,7 +83,7 @@ describe('binary', () => {
 
       (fs.existsSync as jest.Mock<any>).mockReturnValue(true);
 
-      await removeBinary(version);
+      await removeBinary(ver);
       expect(removeTypeDefsForVersion).toHaveBeenCalledTimes(4);
     });
   });
diff --git a/tests/renderer/components/settings-electron-spec.tsx b/tests/renderer/components/settings-electron-spec.tsx
index 2fba5308..a4f2db8e 100644
--- a/tests/renderer/components/settings-electron-spec.tsx
+++ b/tests/renderer/components/settings-electron-spec.tsx
@@ -119,9 +119,7 @@ describe('ElectronSettings component', () => {
     const instance = wrapper.instance() as any;
     await instance.handleDeleteAll();
 
-    expect(store.removeVersion).toHaveBeenCalledTimes(
-      mockVersionsArray.length - 1,
-    );
+    expect(store.removeVersion).toHaveBeenCalledTimes(mockVersionsArray.length);
   });
 
   it('handles the downloadAll()', async () => {
diff --git a/tests/renderer/state-spec.ts b/tests/renderer/state-spec.ts
index f7215fbd..a0adcba9 100644
--- a/tests/renderer/state-spec.ts
+++ b/tests/renderer/state-spec.ts
@@ -397,24 +397,22 @@ describe('AppState', () => {
     });
 
     it('does not remove the active version', async () => {
-      await appState.removeVersion(active);
+      const ver = appState.versions[active];
+      await appState.removeVersion(ver);
       expect(removeBinary).not.toHaveBeenCalled();
     });
 
     it('removes a version', async () => {
-      appState.versions[version].state = VersionState.ready;
-      await appState.removeVersion(version);
-      expect(removeBinary).toHaveBeenCalledWith<any>(version);
+      const ver = appState.versions[version];
+      ver.state = VersionState.ready;
+      await appState.removeVersion(ver);
+      expect(removeBinary).toHaveBeenCalledWith<any>(ver);
     });
 
     it('does not remove it if not necessary', async () => {
-      appState.versions[version].state = VersionState.unknown;
-      await appState.removeVersion(version);
-      expect(removeBinary).toHaveBeenCalledTimes(0);
-    });
-
-    it('does not remove it if not necessary (version not existent)', async () => {
-      await appState.removeVersion('-1.0.0');
+      const ver = appState.versions[version];
+      ver.state = VersionState.unknown;
+      await appState.removeVersion(ver);
       expect(removeBinary).toHaveBeenCalledTimes(0);
     });
 
@@ -426,7 +424,7 @@ describe('AppState', () => {
       ver.source = VersionSource.local;
       ver.state = VersionState.ready;
 
-      await appState.removeVersion(version);
+      await appState.removeVersion(ver);
 
       expect(saveLocalVersions).toHaveBeenCalledTimes(1);
       expect(appState.versions[version]).toBeUndefined();
@@ -436,28 +434,21 @@ describe('AppState', () => {
 
   describe('downloadVersion()', () => {
     it('downloads a version', async () => {
-      const version = '2.0.2';
-      const ver = appState.versions[version];
-
+      const ver = appState.versions['2.0.2'];
       ver.state = VersionState.unknown;
-      await appState.downloadVersion(version);
 
-      expect(setupBinary).toHaveBeenCalledWith<any>(ver);
-    });
+      await appState.downloadVersion(ver);
 
-    it('downloads an unknown version', async () => {
-      const version = '3.5';
-      expect(appState.versions[version]).toBeUndefined();
-      await appState.downloadVersion(version);
-      expect(appState.versions[version]).not.toBeUndefined();
-      expect(setupBinary).toHaveBeenCalledWith<any>(appState.versions[version]);
+      expect(setupBinary).toHaveBeenCalledWith<any>(ver);
     });
 
     it('does not download a version if already ready', async () => {
-      appState.versions['2.0.2'].state = VersionState.ready;
+      const ver = appState.versions['2.0.2'];
+      ver.state = VersionState.ready;
+
+      await appState.downloadVersion(ver);
 
-      await appState.downloadVersion('v2.0.2');
-      expect(setupBinary).toHaveBeenCalledTimes(0);
+      expect(setupBinary).not.toHaveBeenCalled();
     });
   });