Repository: electron/fiddle
Pull request #645 (2021-04-08 15:26:58)

refactor: add getVersionState()
This refactor replaces `getDownloadingVersions()` and `getDownloadedVersions()` with a new function, `getVersionState()` which is intended to be more flexible in that it can also determine any state a Version is in, rather than just downloading/downloaded. For example, is it being unzipped at the moment.

This is an interim step towards dual goals of (a) adding a new state of "downloaded but zipped" for https://github.com/electron/fiddle/issues/644 and (b) cleanup to `state.versions` s.t. we don't need to keep rebuilding all those mobx proxies all the time.

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

Coverage increased (+0.06%) to 93.983% when pulling **99e50b1c33267f5627038c38a1f1404cf1126b1c on refactor/add-getVersionState** into **644e0909e17de9a1c9b213e1e9deb93754ee9365 on master**.


Patch:
diff --git a/src/renderer/binary.ts b/src/renderer/binary.ts
index 9af0bcab..e9970bd0 100644
--- a/src/renderer/binary.ts
+++ b/src/renderer/binary.ts
@@ -2,14 +2,40 @@ import * as fs from 'fs-extra';
 import * as path from 'path';
 import extract from 'extract-zip';
 
-import { VersionState } from '../interfaces';
+import { Version, VersionSource, VersionState } from '../interfaces';
 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>> = {};
+// versions that are currently being downloaded
+const downloading: Map<string, Promise<void>> = new Map();
+
+// versions that are currently being unzipped
+const unzipping: Set<string> = new Set();
+
+/**
+ * Determine by inspection the VersionState of this version
+ *
+ * @param {Version} version
+ * @returns {VersionState} the state of the version
+ */
+export function getVersionState(ver: Version): VersionState {
+  const { localPath, version } = ver;
+
+  if (downloading.has(version)) {
+    return VersionState.downloading;
+  }
+
+  if (unzipping.has(version)) {
+    return VersionState.unzipping;
+  }
+
+  const dir = localPath || getDownloadPath(version);
+  const exec = path.join(dir, execSubpath());
+  return fs.existsSync(exec) ? VersionState.ready : VersionState.unknown;
+}
 
 /**
  * General setup, called with a version. Is called during construction
@@ -24,6 +50,11 @@ export function setupBinary(
 ): Promise<void> {
   const version = normalizeVersion(iVersion);
 
+  // Only remote versions can be downloaded
+  if (appState.versions[version].source !== VersionSource.remote) {
+    return Promise.resolve();
+  }
+
   // If we already have it, then we're done
   if (getIsDownloaded(version)) {
     console.log(`Binary: Electron ${version} already downloaded.`);
@@ -32,12 +63,13 @@ export function setupBinary(
   }
 
   // Return a promise that resolves when the download completes
-  let pending = pendingDownloads[version];
+  let pending = downloading.get(version);
   if (pending) {
     console.log(`Binary: Electron ${version} already downloading.`);
   } else {
     console.log(`Binary: Electron ${version} not present, downloading`);
-    pending = pendingDownloads[version] = downloadBinary(appState, version);
+    pending = downloadBinary(appState, version);
+    downloading.set(version, pending);
   }
   return pending;
 }
@@ -55,6 +87,8 @@ async function downloadBinary(
   );
 
   try {
+    unzipping.add(version);
+    downloading.delete(version);
     appState.versions[version].state = VersionState.unzipping;
 
     // Ensure the target path exists
@@ -71,7 +105,7 @@ async function downloadBinary(
     appState.versions[version].state = VersionState.unknown;
   } finally {
     // This task is done, so remove it from the pending tasks list
-    delete pendingDownloads[version];
+    unzipping.delete(version);
   }
 }
 
@@ -138,6 +172,21 @@ export function getIsDownloaded(version: string, dir?: string): boolean {
   return fs.existsSync(expectedPath);
 }
 
+function execSubpath() {
+  const { platform } = process;
+  switch (platform) {
+    case 'darwin':
+      return 'Electron.app/Contents/MacOS/Electron';
+    case 'linux':
+    case 'freebsd':
+      return 'electron';
+    case 'win32':
+      return 'electron.exe';
+    default:
+      throw new Error(`Electron builds are not available for ${platform}`);
+  }
+}
+
 /**
  * Gets the expected path for the binary of a given Electron version
  *
@@ -149,43 +198,7 @@ export function getElectronBinaryPath(
   version: string,
   dir: string = getDownloadPath(version),
 ): string {
-  switch (process.platform) {
-    case 'darwin':
-      return path.join(dir, 'Electron.app/Contents/MacOS/Electron');
-    case 'freebsd':
-    case 'linux':
-      return path.join(dir, 'electron');
-    case 'win32':
-      return path.join(dir, 'electron.exe');
-    default:
-      throw new Error(
-        `Electron builds are not available for ${process.platform}`,
-      );
-  }
-}
-
-export function getDownloadingVersions(appState: AppState) {
-  return Object.entries(appState.versions)
-    .filter(([_, { state }]) => state === 'downloading')
-    .map(([version, _]) => version);
-}
-
-/**
- * Returns an array of all versions downloaded to disk
- *
- * @returns {Promise<Array<string>>}
- */
-export async function getDownloadedVersions(): Promise<Array<string>> {
-  const downloadPath = path.join(USER_DATA_PATH, 'electron-bin');
-  console.log(`Binary: Checking for downloaded versions`);
-
-  try {
-    const dirs = await fs.readdir(downloadPath);
-    return dirs.filter((dir) => dir && getIsDownloaded(dir));
-  } catch (error) {
-    console.warn(`Could not read known Electron versions`, error);
-    return [];
-  }
+  return path.join(dir, execSubpath());
 }
 
 /**
diff --git a/src/renderer/state.ts b/src/renderer/state.ts
index 7e028c97..c0c0dfa1 100644
--- a/src/renderer/state.ts
+++ b/src/renderer/state.ts
@@ -30,12 +30,7 @@ import {
 import { getName } from '../utils/get-name';
 import { normalizeVersion } from '../utils/normalize-version';
 import { isEditorBackup, isEditorId, isPanelId } from '../utils/type-checks';
-import {
-  getDownloadedVersions,
-  getDownloadingVersions,
-  removeBinary,
-  setupBinary,
-} from './binary';
+import { getVersionState, removeBinary, setupBinary } from './binary';
 import { Bisector } from './bisect';
 import { DEFAULT_CLOSED_PANELS, DEFAULT_MOSAIC_ARRANGEMENT } from './constants';
 import { getTemplate, isContentUnchanged } from './content';
@@ -342,26 +337,17 @@ export class AppState {
    * current settings for states and channels to display
    */
   @computed get versionsToShow(): Array<RunnableVersion> {
-    return sortedElectronMap<RunnableVersion>(
-      this.versions,
-      (_key, item) => item,
-    ).filter((item) => {
-      if (!item) {
-        return false;
-      }
+    const { channelsToShow, statesToShow, versions } = this;
 
-      // Check if we want to show the version
-      if (!this.channelsToShow.includes(getReleaseChannel(item))) {
-        return false;
-      }
+    const filter = (ver: RunnableVersion) =>
+      ver &&
+      statesToShow.includes(ver.state) &&
+      channelsToShow.includes(getReleaseChannel(ver));
 
-      // Check if we want to show the state
-      if (!this.statesToShow.includes(item.state)) {
-        return false;
-      }
-
-      return true;
-    });
+    return sortedElectronMap<RunnableVersion>(
+      versions,
+      (_key, item) => item,
+    ).filter(filter);
   }
 
   /**
@@ -486,7 +472,7 @@ export class AppState {
     this.versions = Object.fromEntries(
       versions.map((ver) => [ver.version, ver]),
     );
-    this.updateDownloadedVersionState();
+    this.updateVersionStates();
   }
 
   /**
@@ -549,7 +535,7 @@ export class AppState {
       this.versions = updatedVersions;
 
       await setupBinary(this, version);
-      this.updateDownloadedVersionState();
+      this.updateVersionStates();
     } else {
       console.log(
         `State: Version ${version} already downloaded, doing nothing.`,
@@ -631,28 +617,12 @@ export class AppState {
    *
    * @returns {Promise<void>}
    */
-  @action public async updateDownloadedVersionState(): Promise<void> {
-    const updatedVersions = toJS(this.versions);
-
-    // Keep state of currently downloading binaries first
-    const downloadingVersions = getDownloadingVersions(this);
-    for (const version of downloadingVersions) {
-      if (updatedVersions[version]) {
-        updatedVersions[version].state = VersionState.downloading;
-      }
-    }
-
-    const downloadedVersions = await getDownloadedVersions();
-
-    for (const version of downloadedVersions) {
-      if (updatedVersions[version]) {
-        updatedVersions[version].state = VersionState.ready;
-      }
+  @action public updateVersionStates() {
+    const versions = toJS(this.versions);
+    for (const ver of Object.values(versions)) {
+      ver.state = getVersionState(ver);
     }
-
-    console.log(`State: Updated version state`, updatedVersions);
-
-    this.versions = updatedVersions;
+    this.versions = versions;
   }
 
   /**
diff --git a/src/renderer/versions.ts b/src/renderer/versions.ts
index 8279c99f..8549c6b4 100644
--- a/src/renderer/versions.ts
+++ b/src/renderer/versions.ts
@@ -6,6 +6,7 @@ import {
   VersionSource,
   VersionState,
 } from '../interfaces';
+import { getVersionState } from './binary';
 import { normalizeVersion } from '../utils/normalize-version';
 
 /**
@@ -135,6 +136,7 @@ function saveVersions(key: VersionKeys, versions: Array<Version>) {
 
 function sanitizeVersion(ver: RunnableVersion): RunnableVersion {
   ver.version = normalizeVersion(ver.version);
+  ver.state = getVersionState(ver);
   return ver;
 }
 


Test patch:
diff --git a/tests/renderer/binary-spec.ts b/tests/renderer/binary-spec.ts
index 8d7c4ad3..6591cf50 100644
--- a/tests/renderer/binary-spec.ts
+++ b/tests/renderer/binary-spec.ts
@@ -1,14 +1,20 @@
 import {
-  getDownloadedVersions,
-  getDownloadingVersions,
   getElectronBinaryPath,
+  getVersionState,
   removeBinary,
   setupBinary,
 } from '../../src/renderer/binary';
+import {
+  RunnableVersion,
+  VersionSource,
+  VersionState,
+} from '../../src/interfaces';
 import { removeTypeDefsForVersion } from '../../src/renderer/fetch-types';
 import { overridePlatform, resetPlatform } from '../utils';
+import { waitFor } from '../../src/utils/wait-for';
 
 import * as path from 'path';
+import * as semver from 'semver';
 import extract from 'extract-zip';
 
 jest.mock('fs-extra');
@@ -80,33 +86,76 @@ describe('binary', () => {
     });
   });
 
-  describe('getDownloadedVersions()', () => {
-    it('finds downloaded versions', async () => {
-      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(versions);
+  describe('getVersionState()', () => {
+    let version = '3.0.0';
+    let ver: RunnableVersion;
+
+    beforeEach(() => {
+      // the download module holds state between runs,
+      // so use a unique version number in each test to
+      // prevent state pollution
+      version = semver.inc(version, 'patch')!;
+      ver = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
+      mockState.versions[version] = ver;
     });
 
-    it('is okay without versions ', async () => {
-      const versions: string[] = [];
-      (fs.readdir as jest.Mock<any>).mockResolvedValue(versions);
-      const result = await getDownloadedVersions();
-      expect(result).toEqual(versions);
+    it(`returns ${VersionState.downloading} when downloading`, () => {
+      // mock a download in progress
+      let downloadResolve: any;
+      const { download } = require('@electron/get');
+      download.mockReturnValue(new Promise((r) => (downloadResolve = r)));
+
+      setupBinary(mockState, ver.version);
+
+      expect(getVersionState(ver)).toBe(VersionState.downloading);
+      downloadResolve();
     });
 
-    it('is okay without versions and with errors', async () => {
-      (fs.readdir as jest.Mock<any>).mockRejectedValueOnce('💩');
-      const result = await getDownloadedVersions();
-      expect(result).toEqual([]);
+    it(`returns ${VersionState.unzipping} when unzipping`, async () => {
+      // mock a completed download + an unzip in progress
+      const { download } = require('@electron/get');
+      download.mockResolvedValueOnce('/fake/path');
+      let unzipResolve: any;
+      const unzipPromise = new Promise((r) => (unzipResolve = r));
+      (extract as jest.Mock).mockReturnValueOnce(unzipPromise);
+
+      setupBinary(mockState, ver.version);
+      await waitFor(() => getVersionState(ver) === VersionState.unzipping);
+
+      expect(getVersionState(ver)).toBe(VersionState.unzipping);
+      unzipResolve();
+    });
+
+    it(`returns ${VersionState.ready} when it is a local version that exists`, async () => {
+      ver.source = VersionSource.local;
+      ver.localPath = '/fake/path';
+      (fs.existsSync as jest.Mock).mockReturnValue(true);
+      expect(getVersionState(ver)).toBe(VersionState.ready);
     });
-  });
 
-  describe('getDownloadingVersions()', () => {
-    it('returns currently downloading versions', () => {
-      const result = getDownloadingVersions(mockState as any);
-      expect(result).toEqual(['3.0.0']);
+    it(`returns ${VersionState.unknown} when it is a local version that is missing`, async () => {
+      ver.source = VersionSource.local;
+      ver.localPath = '/fake/path';
+      (fs.existsSync as jest.Mock).mockReturnValue(false);
+      expect(getVersionState(ver)).toBe(VersionState.unknown);
+    });
+
+    it(`returns ${VersionState.ready} when it is a remote version that is downloaded`, async () => {
+      ver.source = VersionSource.remote;
+      ver.localPath = undefined;
+      (fs.existsSync as jest.Mock).mockReturnValue(true);
+      expect(getVersionState(ver)).toBe(VersionState.ready);
+    });
+
+    it(`returns ${VersionState.unknown} when it is a remote version is not downloaded`, async () => {
+      ver.source = VersionSource.remote;
+      ver.localPath = undefined;
+      (fs.existsSync as jest.Mock).mockReturnValue(false);
+      expect(getVersionState(ver)).toBe(VersionState.unknown);
     });
   });
 
@@ -146,6 +195,23 @@ describe('binary', () => {
   describe('setupBinary()', () => {
     const { download } = require('@electron/get');
 
+    it('does not download local versions', async () => {
+      const localPath = '/fake/path';
+      download.mockResolvedValue(localPath);
+
+      const version = '3.0.0';
+      mockState.versions[version] = {
+        source: VersionSource.local,
+        state: VersionState.unknown,
+        version,
+      };
+
+      await setupBinary(mockState, version);
+
+      expect(download).not.toHaveBeenCalled();
+      expect(mockState.versions[version].state).toBe(VersionState.unknown);
+    });
+
     it(`downloads a version it has not seen before`, async () => {
       (fs.existsSync as jest.Mock).mockReturnValueOnce(false);
 
@@ -153,7 +219,11 @@ describe('binary', () => {
       download.mockResolvedValue(localPath);
 
       const version = '3.0.0';
-      mockState.versions[version] = { state: 'unknown' };
+      mockState.versions[version] = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
       await setupBinary(mockState, version);
 
       expect(download).toHaveBeenCalled();
@@ -164,7 +234,11 @@ describe('binary', () => {
       (fs.existsSync as jest.Mock).mockReturnValue(true);
 
       const version = '3.0.0';
-      mockState.versions[version] = { state: 'unknown' };
+      mockState.versions[version] = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
 
       await setupBinary(mockState, version);
 
@@ -176,7 +250,12 @@ describe('binary', () => {
       (fs.existsSync as jest.Mock).mockReturnValue(true);
 
       const version = '3.0.0';
-      mockState.versions[version] = { state: 'downloading' };
+      mockState.versions[version] = {
+        source: VersionSource.remote,
+        state: VersionState.downloading,
+        version,
+      };
+
       await setupBinary(mockState, version);
 
       expect(download).not.toHaveBeenCalled();
@@ -187,7 +266,12 @@ describe('binary', () => {
       const { download } = require('@electron/get');
 
       download.mockReturnValue('/fake/path');
-      mockState.versions['3.0.0'] = { state: 'unknown' };
+      const version = '3.0.0';
+      mockState.versions[version] = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
 
       (extract as jest.Mock).mockRejectedValue(new Error('bwap-bwap'));
 
diff --git a/tests/renderer/state-spec.ts b/tests/renderer/state-spec.ts
index 14dd83ec..fd98c789 100644
--- a/tests/renderer/state-spec.ts
+++ b/tests/renderer/state-spec.ts
@@ -13,8 +13,7 @@ import {
 } from '../../src/interfaces';
 import { IpcEvents } from '../../src/ipc-events';
 import {
-  getDownloadedVersions,
-  getDownloadingVersions,
+  getVersionState,
   removeBinary,
   setupBinary,
 } from '../../src/renderer/binary';
@@ -43,8 +42,7 @@ jest.mock('../../src/renderer/content', () => ({
 jest.mock('../../src/renderer/binary', () => ({
   removeBinary: jest.fn(),
   setupBinary: jest.fn(),
-  getDownloadedVersions: jest.fn().mockResolvedValue([]),
-  getDownloadingVersions: jest.fn().mockReturnValue([]),
+  getVersionState: jest.fn().mockImplementation((v) => v.state),
 }));
 jest.mock('../../src/renderer/fetch-types', () => ({
   updateEditorTypeDefinitions: jest.fn(),
@@ -78,13 +76,15 @@ describe('AppState', () => {
   beforeEach(() => {
     ({ mockVersions, mockVersionsArray } = new MockVersions());
 
-    (getDownloadedVersions as jest.Mock).mockResolvedValue([]);
-    (getDownloadingVersions as jest.Mock).mockReturnValue([]);
     (getUpdatedElectronVersions as jest.Mock).mockResolvedValue(
       mockVersionsArray,
     );
+    (getVersionState as jest.Mock).mockImplementation((v) => v.state);
 
     appState = new AppState();
+    appState.versions = mockVersions;
+    appState.updateVersionStates = jest.fn();
+
     ipcRendererManager.removeAllListeners();
   });
 
@@ -527,25 +527,31 @@ describe('AppState', () => {
       // We just want to verify that the version state was
       // refreshed - we didn't actually add the local version
       // above, since versions.ts is mocked
-      expect(Object.keys(appState.versions)).toEqual(
-        mockVersionsArray.map((v) => v.version),
-      );
+      expect(Object.keys(appState.versions)).toEqual(Object.keys(mockVersions));
     });
   });
 
-  describe('updateDownloadedVersionState()', () => {
-    it('downloads a version if necessary', async () => {
-      (getDownloadedVersions as jest.Mock).mockResolvedValue(['2.0.2']);
+  describe('updateVersionStates()', () => {
+    beforeEach(() => {
+      ipcRendererManager.removeAllListeners();
+      (getVersionState as jest.Mock).mockImplementation(
+        (v: RunnableVersion) => {
+          if (v.version === '2.0.1') return VersionState.downloading;
+          if (v.version === '2.0.2') return VersionState.ready;
+          return VersionState.unknown;
+        },
+      );
+      appState = new AppState();
+    });
 
-      await appState.updateDownloadedVersionState();
+    it('downloads a version if necessary', async () => {
+      await appState.updateVersionStates();
 
       expect(appState.versions['2.0.2'].state).toBe(VersionState.ready);
     });
 
     it('keeps downloading state intact', async () => {
-      (getDownloadingVersions as jest.Mock).mockReturnValue(['2.0.1']);
-
-      await appState.updateDownloadedVersionState();
+      await appState.updateVersionStates();
 
       expect(appState.versions['2.0.1'].state).toBe(VersionState.downloading);
     });
diff --git a/tests/renderer/versions-spec.ts b/tests/renderer/versions-spec.ts
index ff47cc6a..c4da7377 100644
--- a/tests/renderer/versions-spec.ts
+++ b/tests/renderer/versions-spec.ts
@@ -4,7 +4,9 @@ import {
   ElectronReleaseChannel,
   RunnableVersion,
   VersionSource,
+  VersionState,
 } from '../../src/interfaces';
+import { getVersionState } from '../../src/renderer/binary';
 import {
   addLocalVersion,
   fetchVersions,
@@ -18,6 +20,12 @@ import {
 } from '../../src/renderer/versions';
 import { mockFetchOnce } from '../utils';
 
+jest.mock('../../src/renderer/binary', () => ({
+  getVersionState: jest
+    .fn()
+    .mockImplementation((v: RunnableVersion) => v.state),
+}));
+
 const { expectedVersionCount } = require('../fixtures/releases-metadata.json');
 
 const mockVersions: Array<Partial<RunnableVersion>> = [
@@ -215,20 +223,35 @@ describe('versions', () => {
 
   describe('getUpdatedElectronVersions()', () => {
     it('gets known versions', async () => {
-      (window as any).localStorage.getItem.mockReturnValueOnce(
-        `[{ "version": "3.0.5" }]`,
-      );
-      (window as any).localStorage.getItem.mockReturnValueOnce(
-        `[{ "version": "3.0.5" }]`,
-      );
+      (getVersionState as jest.Mock).mockImplementation((v: any) => {
+        if (v.version === '3.0.5') return VersionState.ready;
+        if (v.version === '3.0.6') return VersionState.unknown;
+        return v.state;
+      });
+
+      (window as any).localStorage.getItem.mockImplementation((key: string) => {
+        if (key === 'known-electron-versions')
+          return '[{ "version": "3.0.5" }]';
+        if (key === 'local-electron-versions')
+          return '[{ "version": "3.0.6" }]';
+        throw new Error(`unexpected key ${key}`);
+      });
+
       mockFetchOnce('');
 
       const result = await getUpdatedElectronVersions();
-      const expectedVersion = { version: '3.0.5', state: 'unknown' };
 
       expect(result).toEqual([
-        { ...expectedVersion, source: 'remote' },
-        { ...expectedVersion, source: 'local', state: 'ready' },
+        {
+          source: VersionSource.remote,
+          state: VersionState.ready,
+          version: '3.0.5',
+        },
+        {
+          source: VersionSource.local,
+          state: VersionState.unknown,
+          version: '3.0.6',
+        },
       ]);
     });
   });