fix: enforce extraModules to be a set of strings (#888)

* fix: usage of this.prodDeps as a Set<string> For some reason TypeScript is AOK with stuffing `Set` objects with random keys. This is problematic as this is not how `Set` are supposed to be used (like a plain JavaScript object map). Fix the logic to respect the `-w/--which-module` CLI argument. * simplify tests * test: add simple, fast test --------- Co-authored-by: Mark Lee <[email protected]> Co-authored-by: Erick Zhao <[email protected]>
electron · Nov 12, 2024 · 6430e74 · 6430e74
1 parent e1eb81a
commit 6430e74
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 13 deletions.
diff --git a/src/module-walker.ts b/src/module-walker.ts
@@ -53,7 +53,7 @@ export class ModuleWalker {
     }
 
     for (const key of depKeys) {
-      this.prodDeps[key] = true;
+      this.prodDeps.add(key);
       const modulePaths: string[] = await searchForModule(
         this.buildPath,
         key,
@@ -97,11 +97,11 @@ export class ModuleWalker {
 
     const callback = this.markChildrenAsProdDeps.bind(this);
     for (const key of Object.keys(childPackageJson.dependencies || {}).concat(Object.keys(childPackageJson.optionalDependencies || {}))) {
-      if (this.prodDeps[key]) {
+      if (this.prodDeps.has(key)) {
         continue;
       }
 
-      this.prodDeps[key] = true;
+      this.prodDeps.add(key);
 
       moduleWait.push(this.findModule(key, modulePath, callback));
     }
@@ -133,7 +133,7 @@ export class ModuleWalker {
       }
       this.realModulePaths.add(realPath);
 
-      if (this.prodDeps[`${prefix}${modulePath}`] && (!this.onlyModules || this.onlyModules.includes(modulePath))) {
+      if (this.prodDeps.has(`${prefix}${modulePath}`) && (!this.onlyModules || this.onlyModules.includes(modulePath))) {
         this.modulesToRebuild.push(realPath);
       }
 

diff --git a/src/rebuild.ts b/src/rebuild.ts
@@ -192,7 +192,7 @@ export class Rebuilder implements IRebuilder {
 
     this.ABIVersion = options.forceABI?.toString();
     const onlyModules = options.onlyModules || null;
-    const extraModules = (options.extraModules || []).reduce((acc: Set<string>, x: string) => acc.add(x), new Set<string>());
+    const extraModules = new Set(options.extraModules);
     const types = options.types || defaultTypes;
     this.moduleWalker = new ModuleWalker(
       this.buildPath,
@@ -237,13 +237,7 @@ export class Rebuilder implements IRebuilder {
 
     this.lifecycle.emit('start');
 
-    await this.moduleWalker.walkModules();
-
-    for (const nodeModulesPath of await this.moduleWalker.nodeModulesPaths) {
-      await this.moduleWalker.findAllModulesIn(nodeModulesPath);
-    }
-
-    for (const modulePath of this.moduleWalker.modulesToRebuild) {
+    for (const modulePath of await this.modulesToRebuild()) {
       this.rebuilds.push(() => this.rebuildModuleAt(modulePath));
     }
 
@@ -258,6 +252,16 @@ export class Rebuilder implements IRebuilder {
     }
   }
 
+  async modulesToRebuild(): Promise<string[]> {
+    await this.moduleWalker.walkModules();
+
+    for (const nodeModulesPath of await this.moduleWalker.nodeModulesPaths) {
+      await this.moduleWalker.findAllModulesIn(nodeModulesPath);
+    }
+
+    return this.moduleWalker.modulesToRebuild;
+  }
+
   async rebuildModuleAt(modulePath: string): Promise<void> {
     if (!(await fs.pathExists(path.resolve(modulePath, 'binding.gyp')))) {
       return;

diff --git a/test/fixture/empty-project/node_modules/extra/package.json b/test/fixture/empty-project/node_modules/extra/package.json
diff --git a/test/fixture/empty-project/package.json b/test/fixture/empty-project/package.json
@@ -0,0 +1 @@
+{}
diff --git a/test/rebuild.ts b/test/rebuild.ts
@@ -1,11 +1,12 @@
+import { EventEmitter } from 'events';
 import { expect } from 'chai';
 import * as fs from 'fs-extra';
 import * as path from 'path';
 
 import { cleanupTestModule, MINUTES_IN_MILLISECONDS, TEST_MODULE_PATH as testModulePath, resetMSVSVersion, resetTestModule, TIMEOUT_IN_MILLISECONDS } from './helpers/module-setup';
 import { expectNativeModuleToBeRebuilt, expectNativeModuleToNotBeRebuilt } from './helpers/rebuild';
 import { getExactElectronVersionSync } from './helpers/electron-version';
-import { rebuild } from '../lib/rebuild';
+import { Rebuilder, rebuild } from '../lib/rebuild';
 
 const testElectronVersion = getExactElectronVersionSync();
 
@@ -179,6 +180,20 @@ describe('rebuilder', () => {
     });
   });
 
+  describe('with extraModules', () => {
+    it('should rebuild existing modules in extraModules despite them not being found during the module walk', async () => {
+      const rebuilder = new Rebuilder({
+        buildPath: path.join(__dirname, 'fixture', 'empty-project'),
+        electronVersion: testElectronVersion,
+        lifecycle: new EventEmitter(),
+        extraModules: ['extra']
+      });
+      const modulesToRebuild = await rebuilder.modulesToRebuild();
+      expect(modulesToRebuild).to.have.length(1);
+      expect(modulesToRebuild[0].endsWith('extra')).to.be.true;
+    });
+  });
+
   describe('debug rebuild', function() {
     this.timeout(10 * MINUTES_IN_MILLISECONDS);