Repository: electron/fiddle
Pull request #648 (2021-04-09 15:43:29)

refactor: avoid global calls in renderer/state
No functional changes. This is another PR like https://github.com/electron/fiddle/pull/642 aimed at simplifying our tests by reducing side-effects / shared state / unintended code execution.

This time around, it's about avoiding the global calls that are made as a side-effect of importing `state.ts`:
* getElectronVersions()
* getDefaultVersion()
* the biggie, everything in AppState.constructor() due to the AppState that is unconditionally created
* appState.setVersion()

This PR removes these so that importing state.ts has no side-effects. In production, state is instantiated by the App in the same way it does runner, fileManager, loader, etc.

*edit:* PR contains no new uncovered code. Coveralls is upset about code removal. :1st_place_medal: 

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

Coverage decreased (-0.003%) to 94.069% when pulling **8da31ea7bc3ef4f7b001edc8c24c9232fe850b3d on refactor/remove-globals-from-render-state** into **6f65d71dd0b92ee34782e80408624317b406de8f on master**.


Patch:
diff --git a/src/renderer/app.tsx b/src/renderer/app.tsx
index 6114e1b5..a3af5f72 100644
--- a/src/renderer/app.tsx
+++ b/src/renderer/app.tsx
@@ -21,8 +21,9 @@ import { EMPTY_EDITOR_CONTENT, SORTED_EDITORS } from './constants';
 import { FileManager } from './file-manager';
 import { RemoteLoader } from './remote-loader';
 import { Runner } from './runner';
+import { AppState } from './state';
+import { getElectronVersions } from './versions';
 import { TaskRunner } from './task-runner';
-import { appState } from './state';
 import { getTheme } from './themes';
 import { defaultDark, defaultLight } from './themes-defaults';
 
@@ -35,10 +36,10 @@ import { defaultDark, defaultLight } from './themes-defaults';
 export class App {
   public typeDefDisposable: MonacoType.IDisposable | null = null;
   public monaco: typeof MonacoType | null = null;
-  public state = appState;
-  public fileManager = new FileManager(appState);
-  public remoteLoader = new RemoteLoader(appState);
-  public runner = new Runner(appState);
+  public state = new AppState(getElectronVersions());
+  public fileManager = new FileManager(this.state);
+  public remoteLoader = new RemoteLoader(this.state);
+  public runner = new Runner(this.state);
   public readonly taskRunner: TaskRunner;
 
   constructor() {
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 8f7a8603..95edcec5 100644
--- a/src/renderer/fetch-types.ts
+++ b/src/renderer/fetch-types.ts
@@ -137,8 +137,7 @@ export async function updateEditorTypeDefinitions(
   };
 
   // If this method is called before we're ready, we'll delay this work a bit
-  if (!window.ElectronFiddle.app || !window.ElectronFiddle.app.monaco)
-    return defer();
+  if (!window.ElectronFiddle?.app?.monaco) return defer();
 
   const { app } = window.ElectronFiddle;
   const monaco: typeof MonacoType = app.monaco!;
diff --git a/src/renderer/state.ts b/src/renderer/state.ts
index c0c0dfa1..8be4f9bc 100644
--- a/src/renderer/state.ts
+++ b/src/renderer/state.ts
@@ -53,9 +53,6 @@ import {
   saveLocalVersions,
 } from './versions';
 
-const knownVersions = getElectronVersions();
-const defaultVersion = getDefaultVersion(knownVersions);
-
 /**
  * The application's state. Exported as a singleton below.
  *
@@ -64,7 +61,6 @@ const defaultVersion = getDefaultVersion(knownVersions);
  */
 export class AppState {
   // -- Persisted settings ------------------
-  @observable public version: string = defaultVersion;
   @observable public theme: string | null = localStorage.getItem('theme');
   @observable public gitHubAvatarUrl: string | null = localStorage.getItem(
     'gitHubAvatarUrl',
@@ -116,10 +112,8 @@ export class AppState {
     (this.retrieve('acceleratorsToBlock') as Array<BlockableAccelerator>) || [];
   // -- Various session-only state ------------------
   @observable public gistId: string | undefined;
-  @observable public versions: Record<
-    string,
-    RunnableVersion
-  > = Object.fromEntries(knownVersions.map((ver) => [ver.version, ver]));
+  @observable public versions: Record<string, RunnableVersion>;
+  @observable public version: string;
   @observable public output: Array<OutputEntry> = [];
   @observable public localPath: string | undefined;
   @observable public genericDialogOptions: GenericDialogOptions = {
@@ -165,9 +159,10 @@ export class AppState {
 
   private outputBuffer = '';
   private name: string;
+  private readonly defaultVersion: string;
   public appData: string;
 
-  constructor() {
+  constructor(versions: RunnableVersion[]) {
     // Bind all actions
     this.downloadVersion = this.downloadVersion.bind(this);
     this.pushError = this.pushError.bind(this);
@@ -192,10 +187,16 @@ export class AppState {
     this.hideChannels = this.hideChannels.bind(this);
     this.showChannels = this.showChannels.bind(this);
 
+    // init fields
+    this.versions = Object.fromEntries(versions.map((v) => [v.version, v]));
+    this.defaultVersion = getDefaultVersion(versions);
+    this.version = this.defaultVersion;
+
+    ipcRendererManager.removeAllListeners(IpcEvents.BEFORE_QUIT);
+    ipcRendererManager.removeAllListeners(IpcEvents.BISECT_COMMANDS_TOGGLE);
+    ipcRendererManager.removeAllListeners(IpcEvents.CLEAR_CONSOLE);
     ipcRendererManager.removeAllListeners(IpcEvents.OPEN_SETTINGS);
     ipcRendererManager.removeAllListeners(IpcEvents.SHOW_WELCOME_TOUR);
-    ipcRendererManager.removeAllListeners(IpcEvents.CLEAR_CONSOLE);
-    ipcRendererManager.removeAllListeners(IpcEvents.BISECT_COMMANDS_TOGGLE);
 
     ipcRendererManager.on(IpcEvents.OPEN_SETTINGS, this.toggleSettings);
     ipcRendererManager.on(IpcEvents.SHOW_WELCOME_TOUR, this.showTour);
@@ -291,6 +292,8 @@ export class AppState {
     ipcRendererManager.send(IpcEvents.BLOCK_ACCELERATORS, [
       ...this.acceleratorsToBlock,
     ]);
+
+    this.setVersion(this.version);
   }
 
   /**
@@ -325,11 +328,7 @@ export class AppState {
    * one that can be found.
    */
   @computed get currentElectronVersion(): RunnableVersion {
-    if (this.versions[this.version]) {
-      return this.versions[this.version];
-    } else {
-      return this.versions[defaultVersion];
-    }
+    return this.versions[this.version] || this.versions[this.defaultVersion];
   }
 
   /**
@@ -561,10 +560,8 @@ export class AppState {
     const version = normalizeVersion(input);
 
     if (!this.hasVersion(input)) {
-      console.warn(
-        `State: Called setVersion() with ${version}, which does not exist.`,
-      );
-      await this.setVersion(knownVersions[0].version);
+      console.warn(`State: setVersion() got an unknown version ${version}`);
+      await this.setVersion(this.versionsToShow[0].version);
       return;
     }
 
@@ -874,6 +871,3 @@ export class AppState {
     return JSON.parse(value || 'null') as T;
   }
 }
-
-export const appState = new AppState();
-appState.setVersion(appState.version);


Test patch:
diff --git a/tests/renderer/app-spec.tsx b/tests/renderer/app-spec.tsx
index 376d8ac0..c8d0e4c3 100644
--- a/tests/renderer/app-spec.tsx
+++ b/tests/renderer/app-spec.tsx
@@ -10,17 +10,6 @@ import { defaultDark, defaultLight } from '../../src/renderer/themes-defaults';
 jest.mock('../../src/renderer/file-manager', () =>
   require('../mocks/file-manager'),
 );
-jest.mock('../../src/renderer/state', () => ({
-  appState: {
-    closedPanels: {},
-    getName: () => 'Test',
-    hideChannels: jest.fn(),
-    pushOutput: jest.fn(),
-    setVersion: jest.fn(),
-    showChannels: jest.fn(),
-    theme: 'defaultDark',
-  },
-}));
 jest.mock('../../src/renderer/components/header', () => ({
   Header: () => 'Header;',
 }));
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/state-spec.ts b/tests/renderer/state-spec.ts
index fd98c789..e41fa603 100644
--- a/tests/renderer/state-spec.ts
+++ b/tests/renderer/state-spec.ts
@@ -8,6 +8,7 @@ import {
   GenericDialogType,
   PanelId,
   RunnableVersion,
+  Version,
   VersionSource,
   VersionState,
 } from '../../src/interfaces';
@@ -25,6 +26,7 @@ import {
 import { getTemplate, isContentUnchanged } from '../../src/renderer/content';
 import { ipcRendererManager } from '../../src/renderer/ipc';
 import { AppState } from '../../src/renderer/state';
+import { getElectronVersions } from '../../src/renderer/versions';
 import {
   getUpdatedElectronVersions,
   saveLocalVersions,
@@ -56,7 +58,7 @@ jest.mock('../../src/renderer/versions', () => {
 
   return {
     getUpdatedElectronVersions: jest.fn().mockResolvedValue(mockVersionsArray),
-    getElectronVersions: () => mockVersionsArray,
+    getElectronVersions: jest.fn(),
     getDefaultVersion: () => '2.0.2',
     addLocalVersion: jest.fn(),
     saveLocalVersions: jest.fn(),
@@ -81,8 +83,7 @@ describe('AppState', () => {
     );
     (getVersionState as jest.Mock).mockImplementation((v) => v.state);
 
-    appState = new AppState();
-    appState.versions = mockVersions;
+    appState = new AppState(mockVersionsArray);
     appState.updateVersionStates = jest.fn();
 
     ipcRendererManager.removeAllListeners();
@@ -518,16 +519,19 @@ describe('AppState', () => {
     it('refreshes version state', async () => {
       appState.versions = {};
 
-      await appState.addLocalVersion({
+      const version = '4.0.0';
+      const ver: Version = {
         localPath: '/fake/path',
         name: 'local-foo',
-        version: '4.0.0',
-      });
+        version,
+      };
+
+      (getElectronVersions as jest.Mock).mockReturnValue([ver]);
+
+      await appState.addLocalVersion(ver);
 
-      // 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(Object.keys(mockVersions));
+      expect(getElectronVersions).toHaveBeenCalledTimes(1);
+      expect(appState.getVersion(version)).toStrictEqual(ver);
     });
   });
 
@@ -541,7 +545,7 @@ describe('AppState', () => {
           return VersionState.unknown;
         },
       );
-      appState = new AppState();
+      appState = new AppState(mockVersionsArray);
     });
 
     it('downloads a version if necessary', async () => {