feat(core): support ESM Forge module loading (#3582)

* refactor(core): try...catch dynamicImport This ensures calling dynamicImport will rather result in rejected promise than in unhandled exception. * feat(core): support ESM module packages Replace some require() calls with dynamicImport() to support loading makers/publishers/plugins etc. that are ESM makers * fix(core): limit path resolving in helper Avoid resolving path to file url in dynamicImport to support importing modules by identifiers. * fix(core): re-order dynamicImportMaybe methods Try require first, then import, to avoid .default property for CJS. * fix(core) fix tests in core * refactor(core): merge import error messages Merge error messages for imports to easier debug potential failures. * refactor(core): improve tests Rename require-search test to import-search and improve testing the promise rejection. Co-authored-by: Erick Zhao <erick@hotmail.ca> * generic refactors * fix(core): Move plugins init to promise. This should fix regression caused by bd6ea70. --------- Co-authored-by: Erick Zhao <erick@hotmail.ca> Co-authored-by: Samuel Attard <marshallofsound@electronjs.org> Co-authored-by: Erik Moura <erikian@erikian.dev> Co-authored-by: George Xu <33054982+georgexu99@users.noreply.github.com>
electron · Sep 19, 2024 · cc0d7d6 · cc0d7d6
1 parent f4c3834
commit cc0d7d6
Show file tree

Hide file tree

Showing 12 changed files with 101 additions and 77 deletions.
diff --git a/packages/api/core/helper/dynamic-import.d.ts b/packages/api/core/helper/dynamic-import.d.ts
@@ -1 +1,3 @@
 export declare function dynamicImport(path: string): Promise<any>;
+/** Like {@link dynamicImport()}, except it tries out {@link require()} first. */
+export declare function dynamicImportMaybe(path: string): Promise<any>;
diff --git a/packages/api/core/helper/dynamic-import.js b/packages/api/core/helper/dynamic-import.js
@@ -1,5 +1,23 @@
 const url = require('url');
+const fs = require('fs');
 
-exports.dynamicImport = function dynamicImport(path) {
-  return import(url.pathToFileURL(path));
+exports.dynamicImport = async function dynamicImport(path) {
+  try {
+    return await import(fs.existsSync(path) ? url.pathToFileURL(path) : path);
+  } catch (error) {
+    return Promise.reject(error);
+  }
+};
+
+exports.dynamicImportMaybe = async function dynamicImportMaybe(path) {
+  try {
+    return require(path);
+  } catch (e1) {
+    try {
+      return await exports.dynamicImport(path);
+    } catch (e2) {
+      e1.message = '\n1. ' + e1.message + '\n2. ' + e2.message;
+      throw e1;
+    }
+  }
 };
diff --git a/packages/api/core/src/api/init-scripts/find-template.ts b/packages/api/core/src/api/init-scripts/find-template.ts
@@ -2,7 +2,7 @@ import { ForgeTemplate } from '@electron-forge/shared-types';
 import debug from 'debug';
 import resolvePackage from 'resolve-package';
 
-import { PossibleModule } from '../../util/require-search';
+import { PossibleModule } from '../../util/import-search';
 
 const d = debug('electron-forge:init:find-template');
 

diff --git a/packages/api/core/src/api/make.ts b/packages/api/core/src/api/make.ts
@@ -22,10 +22,10 @@ import logSymbols from 'log-symbols';
 
 import getForgeConfig from '../util/forge-config';
 import { getHookListrTasks, runMutatingHook } from '../util/hook';
+import importSearch from '../util/import-search';
 import getCurrentOutDir from '../util/out-dir';
 import parseArchs from '../util/parse-archs';
 import { readMutatedPackageJson } from '../util/read-package-json';
-import requireSearch from '../util/require-search';
 import resolveDir from '../util/resolve-dir';
 
 import { listrPackage } from './package';
@@ -168,7 +168,7 @@ export const listrMake = (
                   throw new Error(`The following maker config has a maker name that is not a string: ${JSON.stringify(resolvableTarget)}`);
                 }
 
-                const MakerClass = requireSearch<typeof MakerImpl>(dir, [resolvableTarget.name]);
+                const MakerClass = await importSearch<typeof MakerImpl>(dir, [resolvableTarget.name]);
                 if (!MakerClass) {
                   throw new Error(
                     `Could not find module with name '${resolvableTarget.name}'. If this is a package from NPM, make sure it's listed in the devDependencies of your package.json. If this is a local module, make sure you have the correct path to its entry point. Try using the DEBUG="electron-forge:require-search" environment variable for more information.`

diff --git a/packages/api/core/src/api/package.ts b/packages/api/core/src/api/package.ts
@@ -14,20 +14,20 @@ import { Listr, PRESET_TIMER } from 'listr2';
 
 import getForgeConfig from '../util/forge-config';
 import { getHookListrTasks, runHook } from '../util/hook';
+import importSearch from '../util/import-search';
 import { warn } from '../util/messages';
 import getCurrentOutDir from '../util/out-dir';
 import { readMutatedPackageJson } from '../util/read-package-json';
-import requireSearch from '../util/require-search';
 import resolveDir from '../util/resolve-dir';
 
 const d = debug('electron-forge:packager');
 
 /**
  * Resolves hooks if they are a path to a file (instead of a `Function`).
  */
-function resolveHooks<F = HookFunction>(hooks: (string | F)[] | undefined, dir: string) {
+async function resolveHooks<F = HookFunction>(hooks: (string | F)[] | undefined, dir: string) {
   if (hooks) {
-    return hooks.map((hook) => (typeof hook === 'string' ? (requireSearch<F>(dir, [hook]) as F) : hook));
+    return await Promise.all(hooks.map(async (hook) => (typeof hook === 'string' ? ((await importSearch<F>(dir, [hook])) as F) : hook)));
   }
 
   return [];
@@ -216,13 +216,12 @@ export const listrPackage = (
 
             const rebuildTasks = new Map<string, Promise<ForgeListrTask<never>>[]>();
             const signalRebuildStart = new Map<string, ((task: ForgeListrTask<never>) => void)[]>();
-
             const afterFinalizePackageTargetsHooks: FinalizePackageTargetsHookFunction[] = [
               (targets, done) => {
                 provideTargets(targets);
                 done();
               },
-              ...resolveHooks(forgeConfig.packagerConfig.afterFinalizePackageTargets, ctx.dir),
+              ...(await resolveHooks(forgeConfig.packagerConfig.afterFinalizePackageTargets, ctx.dir)),
             ];
 
             const pruneEnabled = !('prune' in forgeConfig.packagerConfig) || forgeConfig.packagerConfig.prune;
@@ -265,21 +264,21 @@ export const listrPackage = (
                 await fs.writeJson(path.resolve(buildPath, 'package.json'), copiedPackageJSON, { spaces: 2 });
                 done();
               },
-              ...resolveHooks(forgeConfig.packagerConfig.afterCopy, ctx.dir),
+              ...(await resolveHooks(forgeConfig.packagerConfig.afterCopy, ctx.dir)),
             ];
 
             const afterCompleteHooks: HookFunction[] = [
               async (buildPath, electronVersion, pPlatform, pArch, done) => {
                 signalPackageDone.get(getTargetKey({ platform: pPlatform, arch: pArch }))?.pop()?.();
                 done();
               },
-              ...resolveHooks(forgeConfig.packagerConfig.afterComplete, ctx.dir),
+              ...(await resolveHooks(forgeConfig.packagerConfig.afterComplete, ctx.dir)),
             ];
 
             const afterPruneHooks = [];
 
             if (pruneEnabled) {
-              afterPruneHooks.push(...resolveHooks(forgeConfig.packagerConfig.afterPrune, ctx.dir));
+              afterPruneHooks.push(...(await resolveHooks(forgeConfig.packagerConfig.afterPrune, ctx.dir)));
             }
 
             afterPruneHooks.push((async (buildPath, electronVersion, pPlatform, pArch, done) => {
@@ -293,7 +292,7 @@ export const listrPackage = (
                 done();
               }) as HookFunction,
             ];
-            afterExtractHooks.push(...resolveHooks(forgeConfig.packagerConfig.afterExtract, ctx.dir));
+            afterExtractHooks.push(...(await resolveHooks(forgeConfig.packagerConfig.afterExtract, ctx.dir)));
 
             type PackagerArch = Exclude<ForgeArch, 'arm'>;
 

diff --git a/packages/api/core/src/api/publish.ts b/packages/api/core/src/api/publish.ts
@@ -19,9 +19,9 @@ import fs from 'fs-extra';
 import { Listr } from 'listr2';
 
 import getForgeConfig from '../util/forge-config';
+import importSearch from '../util/import-search';
 import getCurrentOutDir from '../util/out-dir';
 import PublishState from '../util/publish-state';
-import requireSearch from '../util/require-search';
 import resolveDir from '../util/resolve-dir';
 
 import { listrMake, MakeOptions } from './make';
@@ -198,7 +198,7 @@ export default autoTrace(
                 } else {
                   const resolvablePublishTarget = publishTarget as IForgeResolvablePublisher;
                   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-                  const PublisherClass: any = requireSearch(dir, [resolvablePublishTarget.name]);
+                  const PublisherClass: any = await importSearch(dir, [resolvablePublishTarget.name]);
                   if (!PublisherClass) {
                     throw new Error(
                       `Could not find a publish target with the name: ${resolvablePublishTarget.name}. Make sure it's listed in the devDependencies of your package.json`

diff --git a/packages/api/core/src/util/forge-config.ts b/packages/api/core/src/util/forge-config.ts
@@ -6,7 +6,7 @@ import * as interpret from 'interpret';
 import { template } from 'lodash';
 import * as rechoir from 'rechoir';
 
-import { dynamicImport } from '../../helper/dynamic-import.js';
+import { dynamicImportMaybe } from '../../helper/dynamic-import.js';
 
 import { runMutatingHook } from './hook';
 import PluginInterface from './plugin-interface';
@@ -140,13 +140,7 @@ export default async (dir: string): Promise<ResolvedForgeConfig> => {
     const forgeConfigPath = path.resolve(dir, forgeConfig as string);
     try {
       // The loaded "config" could potentially be a static forge config, ESM module or async function
-      let loaded;
-      try {
-        loaded = (await dynamicImport(forgeConfigPath)) as MaybeESM<ForgeConfig | AsyncForgeConfigGenerator>;
-      } catch (err) {
-        // eslint-disable-next-line @typescript-eslint/no-var-requires
-        loaded = require(forgeConfigPath) as MaybeESM<ForgeConfig | AsyncForgeConfigGenerator>;
-      }
+      const loaded = (await dynamicImportMaybe(forgeConfigPath)) as MaybeESM<ForgeConfig | AsyncForgeConfigGenerator>;
       const maybeForgeConfig = 'default' in loaded ? loaded.default : loaded;
       forgeConfig = typeof maybeForgeConfig === 'function' ? await maybeForgeConfig() : maybeForgeConfig;
     } catch (err) {
@@ -173,7 +167,7 @@ export default async (dir: string): Promise<ResolvedForgeConfig> => {
   const templateObj = { ...packageJSON, year: new Date().getFullYear() };
   renderConfigTemplate(dir, templateObj, resolvedForgeConfig);
 
-  resolvedForgeConfig.pluginInterface = new PluginInterface(dir, resolvedForgeConfig);
+  resolvedForgeConfig.pluginInterface = await PluginInterface.create(dir, resolvedForgeConfig);
 
   resolvedForgeConfig = await runMutatingHook(resolvedForgeConfig, 'resolveForgeConfig', resolvedForgeConfig);
 

diff --git a/packages/api/core/src/util/require-search.ts → packages/api/core/src/util/import-search.ts b/packages/api/core/src/util/require-search.ts → packages/api/core/src/util/import-search.ts
@@ -2,7 +2,9 @@ import path from 'path';
 
 import debug from 'debug';
 
-const d = debug('electron-forge:require-search');
+import { dynamicImportMaybe } from '../../helper/dynamic-import.js';
+
+const d = debug('electron-forge:import-search');
 
 // https://github.com/nodejs/node/blob/da0ede1ad55a502a25b4139f58aab3fb1ee3bf3f/lib/internal/modules/cjs/loader.js#L353-L359
 type RequireError = Error & {
@@ -11,14 +13,14 @@ type RequireError = Error & {
   requestPath: string | undefined;
 };
 
-export function requireSearchRaw<T>(relativeTo: string, paths: string[]): T | null {
+export async function importSearchRaw<T>(relativeTo: string, paths: string[]): Promise<T | null> {
   // Attempt to locally short-circuit if we're running from a checkout of forge
   if (__dirname.includes('forge/packages/api/core/') && paths.length === 1 && paths[0].startsWith('@electron-forge/')) {
     const [moduleType, moduleName] = paths[0].split('/')[1].split('-');
     try {
       const localPath = path.resolve(__dirname, '..', '..', '..', '..', moduleType, moduleName);
       d('testing local forge build', { moduleType, moduleName, localPath });
-      return require(localPath);
+      return await dynamicImportMaybe(localPath);
     } catch {
       // Ignore
     }
@@ -32,7 +34,7 @@ export function requireSearchRaw<T>(relativeTo: string, paths: string[]): T | nu
   for (const testPath of testPaths) {
     try {
       d('testing', testPath);
-      return require(testPath);
+      return await dynamicImportMaybe(testPath);
     } catch (err) {
       if (err instanceof Error) {
         const requireErr = err as RequireError;
@@ -51,7 +53,7 @@ export type PossibleModule<T> = {
   default?: T;
 } & T;
 
-export default <T>(relativeTo: string, paths: string[]): T | null => {
-  const result = requireSearchRaw<PossibleModule<T>>(relativeTo, paths);
+export default async <T>(relativeTo: string, paths: string[]): Promise<T | null> => {
+  const result = await importSearchRaw<PossibleModule<T>>(relativeTo, paths);
   return typeof result === 'object' && result && result.default ? result.default : (result as T | null);
 };
diff --git a/packages/api/core/src/util/plugin-interface.ts b/packages/api/core/src/util/plugin-interface.ts
@@ -16,7 +16,7 @@ import debug from 'debug';
 
 import { StartOptions } from '../api';
 
-import requireSearch from './require-search';
+import importSearch from './import-search';
 
 const d = debug('electron-forge:plugins');
 
@@ -25,30 +25,45 @@ function isForgePlugin(plugin: IForgePlugin | unknown): plugin is IForgePlugin {
 }
 
 export default class PluginInterface implements IForgePluginInterface {
-  private plugins: IForgePlugin[];
+  private plugins: IForgePlugin[] = [];
+  private _pluginPromise: Promise<void> = Promise.resolve();
 
   private config: ResolvedForgeConfig;
 
-  constructor(dir: string, forgeConfig: ResolvedForgeConfig) {
-    this.plugins = forgeConfig.plugins.map((plugin) => {
-      if (isForgePlugin(plugin)) {
-        return plugin;
-      }
+  static async create(dir: string, forgeConfig: ResolvedForgeConfig): Promise<PluginInterface> {
+    const int = new PluginInterface(dir, forgeConfig);
+    await int._pluginPromise;
+    return int;
+  }
 
-      if (typeof plugin === 'object' && 'name' in plugin && 'config' in plugin) {
-        const { name: pluginName, config: opts } = plugin;
-        if (typeof pluginName !== 'string') {
-          throw new Error(`Expected plugin[0] to be a string but found ${pluginName}`);
+  private constructor(dir: string, forgeConfig: ResolvedForgeConfig) {
+    this._pluginPromise = Promise.all(
+      forgeConfig.plugins.map(async (plugin): Promise<IForgePlugin> => {
+        if (isForgePlugin(plugin)) {
+          return plugin;
         }
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const Plugin = requireSearch<any>(dir, [pluginName]);
-        if (!Plugin) {
-          throw new Error(`Could not find module with name: ${pluginName}. Make sure it's listed in the devDependencies of your package.json`);
+
+        if (typeof plugin === 'object' && 'name' in plugin && 'config' in plugin) {
+          const { name: pluginName, config: opts } = plugin;
+          if (typeof pluginName !== 'string') {
+            throw new Error(`Expected plugin[0] to be a string but found ${pluginName}`);
+          }
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          const Plugin = await importSearch<any>(dir, [pluginName]);
+          if (!Plugin) {
+            throw new Error(`Could not find module with name: ${pluginName}. Make sure it's listed in the devDependencies of your package.json`);
+          }
+          return new Plugin(opts);
         }
-        return new Plugin(opts);
-      }
 
-      throw new Error(`Expected plugin to either be a plugin instance or a { name, config } object but found ${JSON.stringify(plugin)}`);
+        throw new Error(`Expected plugin to either be a plugin instance or a { name, config } object but found ${JSON.stringify(plugin)}`);
+      })
+    ).then((plugins) => {
+      this.plugins = plugins;
+      for (const plugin of this.plugins) {
+        plugin.init(dir, forgeConfig);
+      }
+      return;
     });
     // TODO: fix hack
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -59,11 +74,6 @@ export default class PluginInterface implements IForgePluginInterface {
       configurable: false,
       writable: false,
     });
-
-    for (const plugin of this.plugins) {
-      plugin.init(dir, forgeConfig);
-    }
-
     this.triggerHook = this.triggerHook.bind(this);
     this.overrideStartLogic = this.overrideStartLogic.bind(this);
   }

diff --git a/packages/api/core/test/fast/import-search_spec.ts b/packages/api/core/test/fast/import-search_spec.ts
@@ -0,0 +1,21 @@
+import { expect } from 'chai';
+
+import findConfig from '../../src/util/forge-config';
+import importSearch from '../../src/util/import-search';
+
+describe('import-search', () => {
+  it('should resolve null if no file exists', async () => {
+    const resolved = await importSearch(__dirname, ['../../src/util/wizard-secrets']);
+    expect(resolved).to.equal(null);
+  });
+
+  it('should resolve a file if it exists', async () => {
+    const resolved = await importSearch(__dirname, ['../../src/util/forge-config']);
+    expect(resolved).to.equal(findConfig);
+  });
+
+  it('should throw if file exists but fails to load', async () => {
+    const promise = importSearch(__dirname, ['../fixture/require-search/throw-error']);
+    await expect(promise).to.be.rejectedWith('test');
+  });
+});
diff --git a/packages/api/core/test/fast/publish_spec.ts b/packages/api/core/test/fast/publish_spec.ts
@@ -60,7 +60,7 @@ describe('publish', () => {
         config.publishers = publishers;
         return config;
       },
-      '../util/require-search': (_: string, [name]: [string]) => {
+      '../util/import-search': async (_: string, [name]: [string]) => {
         if (name === 'void') {
           return fakePublisher(voidStub);
         }

diff --git a/packages/api/core/test/fast/require-search_spec.ts b/packages/api/core/test/fast/require-search_spec.ts