Repository: electron/fiddle
Pull request #647 (2021-04-08 22:16:16)

refactor: resize console with `replaceWith` API
This refactor PR replaces the `this.context.mosaicActions` function that we use to expand and hide the console.

### Background

In `react-mosaic`, information about the current layout is stored in a binary tree of `MosaicNode<T>` objects. An object might looks like this, where `first` and `second` are child nodes:

```js
{
  direction: 'row', // or 'column'
  first: 'panel1',
  second: 'panel2',
  splitPercentage: 50 // between 0 and 100
}
```

The most important value for this PR is `splitPercentage`, which defines the percentage of area that the first item occupies (in the case above, both panels occupy half of the available space).

The module's advanced API allows you to programmatically modify this tree through React Context (available in `this.context.mosaicActions`. We currently use [`this.context.mosaicActions.expand()`](https://github.com/nomcopter/react-mosaic/blob/a80ebd407516e5b9040f8a943b9a41bd9da23835/src/contextTypes.ts#L34)  to set the console's screen real estate to either 25% or 0%. 

### Motivation

The `expand()` function changes the `splitPercentage` property of a Node, but also **bubbles this value up** to the root of the node.

This works for us right now because we only have one level of nodes in our tree. If we ever want to add more complexity to the UI with nested nodes, this function would affect all parent nodes of the console panel.

### Change

Instead of calling `expand()`, we call [`replaceWith()`](https://github.com/nomcopter/react-mosaic/blob/a80ebd407516e5b9040f8a943b9a41bd9da23835/src/contextTypes.ts#L50) to replace the value of the entire tree. This gives us full control of what we want to change.



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

Coverage decreased (-0.01%) to 93.971% when pulling **02f64c9087d0dd88011435b6e47c191a79c88e8b on refactor/mosaic-replace-tree** into **8b16dff5fd73eda95292e2c6e46ca8aa3f664f66 on master**.


Patch:
diff --git a/src/renderer/components/output-editors-wrapper.tsx b/src/renderer/components/output-editors-wrapper.tsx
index 34d4fd5a..8f740fba 100644
--- a/src/renderer/components/output-editors-wrapper.tsx
+++ b/src/renderer/components/output-editors-wrapper.tsx
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { Mosaic, MosaicNode } from 'react-mosaic-component';
+import { Mosaic, MosaicNode, MosaicParent } from 'react-mosaic-component';
 
 import { AppState } from '../state';
 import { Editors } from './editors';
@@ -49,13 +49,12 @@ export class OutputEditorsWrapper extends React.Component<
     );
   }
 
-  private onChange = (currentNode: any) => {
-    const isConsoleShowing = currentNode.splitPercentage !== 0;
+  private onChange = (rootNode: MosaicParent<WrapperMosaicId>) => {
+    const isConsoleShowing = rootNode.splitPercentage !== 0;
 
     if (isConsoleShowing !== this.props.appState.isConsoleShowing) {
       this.props.appState.isConsoleShowing = isConsoleShowing;
     }
-
-    this.setState({ mosaicArrangement: currentNode });
+    this.setState({ mosaicArrangement: rootNode });
   };
 }
diff --git a/src/renderer/components/output.tsx b/src/renderer/components/output.tsx
index 87dbd00a..a4d52a32 100644
--- a/src/renderer/components/output.tsx
+++ b/src/renderer/components/output.tsx
@@ -1,7 +1,11 @@
 import { autorun } from 'mobx';
 import { observer } from 'mobx-react';
 import * as React from 'react';
-import { MosaicContext } from 'react-mosaic-component';
+import {
+  MosaicContext,
+  MosaicNode,
+  MosaicParent,
+} from 'react-mosaic-component';
 import { isWebUri } from 'valid-url';
 
 import { OutputEntry } from '../../interfaces';
@@ -36,15 +40,31 @@ export class Output extends React.Component<CommandsProps> {
 
   public componentDidMount() {
     autorun(() => {
+      /**
+       * Type guard to check whether a react-mosaic node is a parent in the tree
+       * or a leaf. Leaf nodes are represented by the string value of their ID,
+       * whereas parent nodes are objects containing information about the nested
+       * binary tree.
+       * @param node A react-mosaic node
+       * @returns Whether that node is a MosaicParent or not
+       */
+      const isParentNode = (
+        node: MosaicNode<WrapperMosaicId> | null,
+      ): node is MosaicParent<WrapperMosaicId> => {
+        return (node as MosaicParent<WrapperMosaicId>)?.direction !== undefined;
+      };
+
       const { isConsoleShowing } = this.props.appState;
 
       // this context should always exist, but mocking context in enzyme
       // is not fully supported, so this condition makes the tests pass
-      if (this.context.mosaicActions && this.context.mosaicActions.expand) {
-        if (!isConsoleShowing) {
-          this.context.mosaicActions.expand(['first'], 0);
-        } else {
-          this.context.mosaicActions.expand(['first'], 25);
+      if (this.context.mosaicActions) {
+        const mosaicTree = this.context.mosaicActions.getRoot();
+        if (isParentNode(mosaicTree)) {
+          // splitPercentage defines the percentage of space the first panel takes
+          // e.g. 25 would mean the two children panels are split 25%/75%
+          mosaicTree.splitPercentage = isConsoleShowing ? 25 : 0;
+          this.context.mosaicActions.replaceWith([], mosaicTree);
         }
       }
     });
diff --git a/src/renderer/fetch-types.ts b/src/renderer/fetch-types.ts
index 28a9fbb6..8f7a8603 100644
--- a/src/renderer/fetch-types.ts
+++ b/src/renderer/fetch-types.ts
@@ -16,7 +16,10 @@ const definitionPath = path.join(USER_DATA_PATH, 'electron-typedef');
  * @returns {Promise<string>}
  */
 export async function fetchTypeDefinitions(version: string): Promise<string> {
-  const url = `https://unpkg.com/electron@${version}/electron.d.ts`;
+  const packageName = version.includes('nightly')
+    ? 'electron-nightly'
+    : 'electron';
+  const url = `https://unpkg.com/${packageName}@${version}/electron.d.ts`;
 
   let text: string;
   try {
diff --git a/src/renderer/npm.ts b/src/renderer/npm.ts
index 49d9c58b..8d8926d9 100644
--- a/src/renderer/npm.ts
+++ b/src/renderer/npm.ts
@@ -138,7 +138,7 @@ export async function installModules(
   let nameArgs: Array<string> = [];
 
   if (packageManager === 'npm') {
-    nameArgs = names.length > 0 ? ['-S', ...names] : ['--dev --prod'];
+    nameArgs = names.length > 0 ? ['-S', ...names] : ['--also=dev --prod'];
   } else {
     nameArgs = [...names];
   }
diff --git a/src/renderer/transforms/forge.ts b/src/renderer/transforms/forge.ts
index 411f2138..6d168cf9 100644
--- a/src/renderer/transforms/forge.ts
+++ b/src/renderer/transforms/forge.ts
@@ -1,5 +1,7 @@
+import { execSync } from 'child_process';
 import { Files } from '../../interfaces';
 import { PACKAGE_NAME } from '../../shared-constants';
+import { getElectronBinaryPath } from '../binary';
 
 /**
  * This transform turns the files into an electron-forge
@@ -35,6 +37,19 @@ export async function forgeTransform(files: Files): Promise<Files> {
       parsed.config.forge = {};
       parsed.config.forge.packagerConfig = {};
 
+      const nightlyVersion = parsed.devDependencies['electron-nightly'];
+      if (nightlyVersion) {
+        // Fetch forced ABI for nightly.
+        const binaryPath = getElectronBinaryPath(nightlyVersion);
+        const abi = execSync(
+          `ELECTRON_RUN_AS_NODE=1 "${binaryPath}" -p process.versions.modules`,
+        );
+
+        parsed.config.forge.electronRebuildConfig = {
+          forceABI: abi.toString().trim(),
+        };
+      }
+
       // electron-forge makers
       parsed.config.forge.makers = [
         {
diff --git a/src/utils/get-package.ts b/src/utils/get-package.ts
index a233b6c4..c2fe5cbd 100644
--- a/src/utils/get-package.ts
+++ b/src/utils/get-package.ts
@@ -35,7 +35,10 @@ export async function getPackageJson(
   const dependencies: Record<string, string> = {};
 
   if (includeElectron) {
-    devDependencies.electron = appState.version;
+    const packageName = appState.version?.includes('nightly')
+      ? 'electron-nightly'
+      : 'electron';
+    devDependencies[packageName] = appState.version;
   }
 
   if (includeDependencies && values) {


Test patch:
diff --git a/tests/renderer/binary-spec.ts b/tests/renderer/binary-spec.ts
index 6591cf50..29cf4fbe 100644
--- a/tests/renderer/binary-spec.ts
+++ b/tests/renderer/binary-spec.ts
@@ -16,9 +16,11 @@ import { waitFor } from '../../src/utils/wait-for';
 import * as path from 'path';
 import * as semver from 'semver';
 import extract from 'extract-zip';
+import { download as electronDownload } from '@electron/get';
 
 jest.mock('fs-extra');
 jest.mock('extract-zip');
+jest.mock('@electron/get');
 jest.mock('../../src/renderer/ipc', () => ({}));
 jest.mock('../../src/renderer/constants', () => ({
   USER_DATA_PATH: 'user/data/',
@@ -26,9 +28,6 @@ jest.mock('../../src/renderer/constants', () => ({
 jest.mock('../../src/renderer/fetch-types', () => ({
   removeTypeDefsForVersion: jest.fn(),
 }));
-jest.mock('@electron/get', () => ({
-  download: jest.fn(),
-}));
 
 describe('binary', () => {
   let mockState: any = {};
@@ -106,8 +105,9 @@ describe('binary', () => {
     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)));
+      (electronDownload as jest.Mock).mockReturnValueOnce(
+        new Promise((r) => (downloadResolve = r)),
+      );
 
       setupBinary(mockState, ver.version);
 
@@ -117,8 +117,7 @@ describe('binary', () => {
 
     it(`returns ${VersionState.unzipping} when unzipping`, async () => {
       // mock a completed download + an unzip in progress
-      const { download } = require('@electron/get');
-      download.mockResolvedValueOnce('/fake/path');
+      (electronDownload as jest.Mock).mockResolvedValueOnce('/fake/path');
       let unzipResolve: any;
       const unzipPromise = new Promise((r) => (unzipResolve = r));
       (extract as jest.Mock).mockReturnValueOnce(unzipPromise);
@@ -193,11 +192,9 @@ describe('binary', () => {
   });
 
   describe('setupBinary()', () => {
-    const { download } = require('@electron/get');
-
     it('does not download local versions', async () => {
       const localPath = '/fake/path';
-      download.mockResolvedValue(localPath);
+      (electronDownload as jest.Mock).mockResolvedValue(localPath);
 
       const version = '3.0.0';
       mockState.versions[version] = {
@@ -208,7 +205,7 @@ describe('binary', () => {
 
       await setupBinary(mockState, version);
 
-      expect(download).not.toHaveBeenCalled();
+      expect(electronDownload).not.toHaveBeenCalled();
       expect(mockState.versions[version].state).toBe(VersionState.unknown);
     });
 
@@ -216,7 +213,7 @@ describe('binary', () => {
       (fs.existsSync as jest.Mock).mockReturnValueOnce(false);
 
       const localPath = '/fake/path';
-      download.mockResolvedValue(localPath);
+      (electronDownload as jest.Mock).mockResolvedValue(localPath);
 
       const version = '3.0.0';
       mockState.versions[version] = {
@@ -226,10 +223,45 @@ describe('binary', () => {
       };
       await setupBinary(mockState, version);
 
-      expect(download).toHaveBeenCalled();
+      expect(electronDownload).toHaveBeenCalled();
       expect(mockState.versions[version].state).toBe('ready');
     });
 
+    it('handles progress callbacks from electron/get', async () => {
+      const percent = 50;
+      const version = '9.9.9';
+      const ver: RunnableVersion = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
+      mockState.versions[version] = ver;
+
+      // mock a download in progress...
+      let downloadResolve: any;
+      let getProgressCallback: any;
+      (electronDownload as jest.Mock).mockImplementation(
+        (_: any, opts: any) => {
+          ({ getProgressCallback } = opts.downloadOptions);
+          return new Promise((r) => (downloadResolve = r));
+        },
+      );
+
+      setupBinary(mockState, version);
+      await waitFor(() => getVersionState(ver) === VersionState.downloading);
+
+      expect(ver.downloadProgress).not.toBe(percent);
+
+      // send a progress update
+      getProgressCallback({ percent });
+      expect(ver.downloadProgress).toBe(percent);
+      // and do it again to trigger a different if-else branch
+      getProgressCallback({ percent });
+      expect(ver.downloadProgress).toBe(percent);
+
+      downloadResolve();
+    });
+
     it(`does not download a version again`, async () => {
       (fs.existsSync as jest.Mock).mockReturnValue(true);
 
@@ -242,11 +274,27 @@ describe('binary', () => {
 
       await setupBinary(mockState, version);
 
-      expect(download).not.toHaveBeenCalled();
+      expect(electronDownload).not.toHaveBeenCalled();
       expect(mockState.versions[version].state).toBe('ready');
     });
 
-    it(`waits if asked for a version is already downloading`, async () => {
+    it(`returns the same promise if called twice for the same version`, async () => {
+      (fs.existsSync as jest.Mock).mockReturnValue(false);
+
+      const version = '123.456.789';
+      mockState.versions[version] = {
+        source: VersionSource.remote,
+        state: VersionState.unknown,
+        version,
+      };
+
+      const prom1 = setupBinary(mockState, version);
+      const prom2 = setupBinary(mockState, version);
+
+      expect(prom1).toBe(prom2);
+    });
+
+    it(`waits if asked for a version that is already downloading`, async () => {
       (fs.existsSync as jest.Mock).mockReturnValue(true);
 
       const version = '3.0.0';
@@ -258,14 +306,12 @@ describe('binary', () => {
 
       await setupBinary(mockState, version);
 
-      expect(download).not.toHaveBeenCalled();
+      expect(electronDownload).not.toHaveBeenCalled();
       expect(mockState.versions[version].state).toBe('ready');
     });
 
     it('handles an error in the zip file', async () => {
-      const { download } = require('@electron/get');
-
-      download.mockReturnValue('/fake/path');
+      (electronDownload as jest.Mock).mockReturnValue('/fake/path');
       const version = '3.0.0';
       mockState.versions[version] = {
         source: VersionSource.remote,
diff --git a/tests/renderer/components/output-spec.tsx b/tests/renderer/components/output-spec.tsx
index 2229ac1a..41dfc70a 100644
--- a/tests/renderer/components/output-spec.tsx
+++ b/tests/renderer/components/output-spec.tsx
@@ -4,32 +4,17 @@ import * as React from 'react';
 import { Output } from '../../../src/renderer/components/output';
 import { MockState } from '../../mocks/state';
 
-let mockContext: any = {};
-
-jest.mock('react-mosaic-component', () => {
-  const { MosaicContext, MosaicRootActions } = jest.requireActual(
-    'react-mosaic-component',
-  );
-
-  return {
-    MosaicContext,
-    MosaicRootActions,
-  };
-});
-
-beforeAll(() => {
-  mockContext = {
-    mosaicActions: {
-      expand: jest.fn(),
-      remove: jest.fn(),
-      hide: jest.fn(),
-      replaceWith: jest.fn(),
-      updateTree: jest.fn(),
-      getRoot: jest.fn(),
-    },
-    mosaicId: 'output',
-  };
-});
+const mockContext = {
+  mosaicActions: {
+    expand: jest.fn(),
+    remove: jest.fn(),
+    hide: jest.fn(),
+    replaceWith: jest.fn(),
+    updateTree: jest.fn(),
+    getRoot: jest.fn(),
+  },
+  mosaicId: 'output',
+};
 
 describe('Output component', () => {
   let store: any;
@@ -87,13 +72,20 @@ describe('Output component', () => {
       disableLifecycleMethods: true,
     });
 
+    mockContext.mosaicActions.getRoot.mockReturnValue({
+      direction: 'row',
+      first: 'output',
+      second: 'editors',
+    });
+
     wrapper.instance().context = mockContext;
     wrapper.instance().componentDidMount!();
 
-    expect(mockContext.mosaicActions.expand).toHaveBeenCalledWith(
-      ['first'],
-      25,
+    expect(mockContext.mosaicActions.replaceWith).toHaveBeenCalledWith(
+      [],
+      expect.objectContaining({ splitPercentage: 25 }),
     );
+    expect(wrapper.html()).not.toBe(null);
 
     store.isConsoleShowing = false;
 
diff --git a/tests/renderer/npm-spec.ts b/tests/renderer/npm-spec.ts
index 5081f67c..2cf58ae5 100644
--- a/tests/renderer/npm-spec.ts
+++ b/tests/renderer/npm-spec.ts
@@ -220,7 +220,7 @@ describe('npm', () => {
 
         expect(exec).toHaveBeenCalledWith<any>(
           '/my/directory',
-          'npm install --dev --prod',
+          'npm install --also=dev --prod',
         );
       });
     });
diff --git a/tests/utils/get-package-spec.ts b/tests/utils/get-package-spec.ts
index 45d56fa4..880ffcfb 100644
--- a/tests/utils/get-package-spec.ts
+++ b/tests/utils/get-package-spec.ts
@@ -47,6 +47,51 @@ describe('get-package', () => {
     );
   });
 
+  it('getPackageJson() includes electron-nightly if needed', async () => {
+    const result = await getPackageJson(
+      {
+        getName: () => 'test-app',
+        version: '1.0.0-nightly.123456789',
+      } as any,
+      {
+        main: 'app.goDoTheThing()',
+        renderer: `const say = require('say')`,
+        html: '<html />',
+        preload: 'preload',
+        css: 'body { color: black }',
+      },
+      {
+        includeElectron: true,
+        includeDependencies: true,
+      },
+    );
+
+    expect(result).toEqual(
+      JSON.stringify(
+        {
+          name: 'test-app',
+          productName: 'test-app',
+          description: 'My Electron application description',
+          keywords: [],
+          main: './main.js',
+          version: '1.0.0',
+          author: 'test-user',
+          scripts: {
+            start: 'electron .',
+          },
+          dependencies: {
+            say: '*',
+          },
+          devDependencies: {
+            'electron-nightly': '1.0.0-nightly.123456789',
+          },
+        },
+        undefined,
+        2,
+      ),
+    );
+  });
+
   it('getPackageJson() includes electron if needed', async () => {
     const result = await getPackageJson(
       {