Skip to content

Commit e5a78d1

Browse files
erickzhaoerickzhao
authored
perf(cli): minimize resolvePackageManager calls (#3853)
* fix(cli): emit NODE_INSTALLER deprecation warning once * Revert "fix(cli): emit NODE_INSTALLER deprecation warning once" This reverts commit b426540. * perf(cli): minimize `resolvePackageManager` calls * back to square one I guess * fix tests * hmmm???
1 parent 18b3db1 commit e5a78d1

File tree

15 files changed

+105
-100
lines changed

15 files changed

+105
-100
lines changed

‎packages/api/cli/spec/util/check-system.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ describe('checkPackageManager', () => {
6666
dev: '--dev',
6767
exact: '--exact',
6868
});
69-
vi.mocked(spawnPackageManager).mockImplementation((args) => {
69+
vi.mocked(spawnPackageManager).mockImplementation((_pm, args) => {
7070
if (args?.join(' ') === 'config get node-linker') {
7171
return Promise.resolve('isolated');
7272
} else if (args?.join(' ') === 'config get hoist-pattern') {
@@ -91,7 +91,7 @@ describe('checkPackageManager', () => {
9191
dev: '--dev',
9292
exact: '--exact',
9393
});
94-
vi.mocked(spawnPackageManager).mockImplementation((args) => {
94+
vi.mocked(spawnPackageManager).mockImplementation((_pm, args) => {
9595
if (args?.join(' ') === 'config get node-linker') {
9696
return Promise.resolve('isolated');
9797
} else if (args?.join(' ') === `config get ${cfg}`) {

‎packages/api/cli/src/util/check-system.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { exec } from 'node:child_process';
22
import os from 'node:os';
33
import path from 'node:path';
44

5-
import { resolvePackageManager, spawnPackageManager, SupportedPackageManager } from '@electron-forge/core-utils';
5+
import { PACKAGE_MANAGERS, resolvePackageManager, spawnPackageManager, SupportedPackageManager } from '@electron-forge/core-utils';
66
import { ForgeListrTask } from '@electron-forge/shared-types';
77
import debug from 'debug';
88
import fs from 'fs-extra';
@@ -36,8 +36,9 @@ async function checkNodeVersion() {
3636
* configuration purposes.
3737
*/
3838
async function checkPnpmConfig() {
39-
const hoistPattern = await spawnPackageManager(['config', 'get', 'hoist-pattern']);
40-
const publicHoistPattern = await spawnPackageManager(['config', 'get', 'public-hoist-pattern']);
39+
const { pnpm } = PACKAGE_MANAGERS;
40+
const hoistPattern = await spawnPackageManager(pnpm, ['config', 'get', 'hoist-pattern']);
41+
const publicHoistPattern = await spawnPackageManager(pnpm, ['config', 'get', 'public-hoist-pattern']);
4142

4243
if (hoistPattern !== 'undefined' || publicHoistPattern !== 'undefined') {
4344
d(
@@ -49,7 +50,7 @@ async function checkPnpmConfig() {
4950
return;
5051
}
5152

52-
const nodeLinker = await spawnPackageManager(['config', 'get', 'node-linker']);
53+
const nodeLinker = await spawnPackageManager(pnpm, ['config', 'get', 'node-linker']);
5354
if (nodeLinker !== 'hoisted') {
5455
throw new Error(
5556
'When using pnpm, `node-linker` must be set to "hoisted" (or a custom `hoist-pattern` or `public-hoist-pattern` must be defined). Run `pnpm config set node-linker hoisted` to set this config value, or add it to your project\'s `.npmrc` file.'
@@ -74,7 +75,7 @@ const ALLOWLISTED_VERSIONS: Record<SupportedPackageManager, Record<string, strin
7475

7576
export async function checkPackageManager() {
7677
const pm = await resolvePackageManager();
77-
const version = pm.version ?? (await spawnPackageManager(['--version']));
78+
const version = pm.version ?? (await spawnPackageManager(pm, ['--version']));
7879
const versionString = version.toString().trim();
7980

8081
const range = ALLOWLISTED_VERSIONS[pm.executable][process.platform] ?? ALLOWLISTED_VERSIONS[pm.executable].all;

‎packages/api/core/spec/fast/util/install-dependencies.spec.ts‎

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,51 @@
1-
import { resolvePackageManager, spawnPackageManager } from '@electron-forge/core-utils';
2-
import { beforeEach, describe, expect, it, vi } from 'vitest';
1+
import { PACKAGE_MANAGERS, spawnPackageManager } from '@electron-forge/core-utils';
2+
import { describe, expect, it, vi } from 'vitest';
33

44
import installDependencies, { DepType, DepVersionRestriction } from '../../../src/util/install-dependencies';
55

66
vi.mock(import('@electron-forge/core-utils'), async (importOriginal) => {
77
const mod = await importOriginal();
88
return {
99
...mod,
10-
resolvePackageManager: vi.fn(),
1110
spawnPackageManager: vi.fn(),
1211
};
1312
});
1413

1514
describe('installDependencies', () => {
1615
it('should immediately resolve if no deps are provided', async () => {
17-
vi.mocked(resolvePackageManager).mockResolvedValue({ executable: 'npm', install: 'install', dev: '--save-dev', exact: '--save-exact' });
18-
await installDependencies('mydir', []);
16+
await installDependencies(PACKAGE_MANAGERS['npm'], 'mydir', []);
1917
expect(spawnPackageManager).not.toHaveBeenCalled();
2018
});
2119

2220
it('should reject if the package manager fails to spawn', async () => {
23-
vi.mocked(resolvePackageManager).mockResolvedValue({ executable: 'npm', install: 'install', dev: '--save-dev', exact: '--save-exact' });
2421
vi.mocked(spawnPackageManager).mockRejectedValueOnce('fail');
25-
await expect(installDependencies('void', ['electron'])).rejects.toThrow('fail');
22+
await expect(installDependencies(PACKAGE_MANAGERS['npm'], 'void', ['electron'])).rejects.toThrow('fail');
2623
});
2724

2825
it('should resolve if the package manager command succeeds', async () => {
29-
vi.mocked(resolvePackageManager).mockResolvedValue({ executable: 'npm', install: 'install', dev: '--save-dev', exact: '--save-exact' });
3026
vi.mocked(spawnPackageManager).mockResolvedValueOnce('pass');
31-
await expect(installDependencies('void', ['electron'])).resolves.toBe(undefined);
27+
await expect(installDependencies(PACKAGE_MANAGERS['npm'], 'void', ['electron'])).resolves.toBe(undefined);
3228
});
3329

34-
describe.each([
35-
{ executable: 'npm' as const, install: 'install', exact: '--save-exact', dev: '--save-dev' },
36-
{ executable: 'yarn' as const, install: 'add', exact: '--exact', dev: '--dev' },
37-
{ executable: 'pnpm' as const, install: 'install', exact: '--save-exact', dev: '--save-dev' },
38-
])('$executable', (args) => {
39-
beforeEach(() => {
40-
vi.mocked(resolvePackageManager).mockResolvedValue(args);
41-
});
42-
30+
describe.each([PACKAGE_MANAGERS['npm'], PACKAGE_MANAGERS['yarn'], PACKAGE_MANAGERS['pnpm']])('$executable', (pm) => {
4331
it('should install deps', async () => {
44-
await installDependencies('mydir', ['react']);
45-
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'react'], expect.anything());
32+
await installDependencies(pm, 'mydir', ['react']);
33+
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'react'], expect.anything());
4634
});
4735

4836
it('should install dev deps', async () => {
49-
await installDependencies('mydir', ['eslint'], DepType.DEV);
50-
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'eslint', args.dev], expect.anything());
37+
await installDependencies(pm, 'mydir', ['eslint'], DepType.DEV);
38+
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'eslint', pm.dev], expect.anything());
5139
});
5240

5341
it('should install exact deps', async () => {
54-
await installDependencies('mydir', ['react'], DepType.PROD, DepVersionRestriction.EXACT);
55-
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'react', args.exact], expect.anything());
42+
await installDependencies(pm, 'mydir', ['react'], DepType.PROD, DepVersionRestriction.EXACT);
43+
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'react', pm.exact], expect.anything());
5644
});
5745

5846
it('should install exact dev deps', async () => {
59-
await installDependencies('mydir', ['eslint'], DepType.DEV, DepVersionRestriction.EXACT);
60-
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'eslint', args.dev, args.exact], expect.anything());
47+
await installDependencies(pm, 'mydir', ['eslint'], DepType.DEV, DepVersionRestriction.EXACT);
48+
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'eslint', pm.dev, pm.exact], expect.anything());
6149
});
6250
});
6351
});

‎packages/api/core/spec/slow/api.slow.spec.ts‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,12 @@ import { execSync } from 'node:child_process';
33
import fs from 'node:fs';
44
import path from 'node:path';
55

6-
import { spawnPackageManager } from '@electron-forge/core-utils';
6+
import { PACKAGE_MANAGERS, spawnPackageManager } from '@electron-forge/core-utils';
77
import { createDefaultCertificate } from '@electron-forge/maker-appx';
88
import { ForgeConfig, IForgeResolvableMaker } from '@electron-forge/shared-types';
99
import { ensureTestDirIsNonexistent, expectLintToPass } from '@electron-forge/test-utils';
1010
import { readMetadata } from 'electron-installer-common';
11-
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
11+
import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest';
1212

1313
// eslint-disable-next-line n/no-missing-import
1414
import { api, InitOptions } from '../../src/api';
@@ -29,19 +29,18 @@ async function updatePackageJSON(dir: string, packageJSONUpdater: (packageJSON:
2929
await fs.promises.writeFile(path.resolve(dir, 'package.json'), JSON.stringify(packageJSON), 'utf-8');
3030
}
3131

32-
describe.each([{ pm: 'npm' }, { pm: 'yarn' }, { pm: 'pnpm' }])(`init (with $pm)`, ({ pm }) => {
32+
describe.each([PACKAGE_MANAGERS['npm'], PACKAGE_MANAGERS['yarn'], PACKAGE_MANAGERS['pnpm']])(`init (with $executable)`, (pm) => {
3333
let dir: string;
3434

3535
beforeAll(async () => {
36-
await spawnPackageManager(['run', 'link:prepare']);
37-
process.env.NODE_INSTALLER = pm;
36+
await spawnPackageManager(pm, ['run', 'link:prepare']);
3837

39-
if (pm === 'pnpm') {
40-
await spawnPackageManager('config set node-linker hoisted'.split(' '));
38+
if (pm.executable === 'pnpm') {
39+
await spawnPackageManager(pm, 'config set node-linker hoisted'.split(' '));
4140
}
4241

4342
return async () => {
44-
await spawnPackageManager(['run', 'link:remove']);
43+
await spawnPackageManager(pm, ['run', 'link:remove']);
4544
delete process.env.NODE_INSTALLER;
4645
};
4746
});
@@ -187,7 +186,7 @@ describe.each([{ pm: 'npm' }, { pm: 'yarn' }, { pm: 'pnpm' }])(`init (with $pm)`
187186
});
188187

189188
describe('import', () => {
190-
beforeAll(async () => {
189+
beforeEach(async () => {
191190
dir = await ensureTestDirIsNonexistent();
192191
await fs.promises.mkdir(dir);
193192
execSync(`git clone https://github.com/electron/electron-quick-start.git . --quiet`, {
@@ -209,9 +208,7 @@ describe.each([{ pm: 'npm' }, { pm: 'yarn' }, { pm: 'pnpm' }])(`init (with $pm)`
209208

210209
expect(fs.existsSync(path.join(dir, 'forge.config.js'))).toEqual(true);
211210

212-
execSync(`${pm} install`, {
213-
cwd: dir,
214-
});
211+
await spawnPackageManager(pm, ['install'], { cwd: dir });
215212

216213
await api.package({ dir });
217214

@@ -297,7 +294,7 @@ describe.each([{ pm: 'npm' }, { pm: 'yarn' }, { pm: 'pnpm' }])(`init (with $pm)`
297294

298295
describe('with prebuilt native module deps installed', () => {
299296
beforeAll(async () => {
300-
await installDeps(dir, ['ref-napi']);
297+
await installDeps(pm, dir, ['ref-napi']);
301298

302299
return async () => {
303300
await fs.promises.rm(path.resolve(dir, 'node_modules/ref-napi'), { recursive: true, force: true });

‎packages/api/core/spec/slow/install-dependencies.slow.spec.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import fs from 'node:fs/promises';
22
import os from 'node:os';
33
import path from 'node:path';
44

5+
import { PACKAGE_MANAGERS } from '@electron-forge/core-utils';
56
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
67

78
import installDeps from '../../src/util/install-dependencies';
@@ -16,7 +17,7 @@ describe.runIf(!(process.platform === 'linux' && process.env.CI))('install-depen
1617
});
1718

1819
it('should install the latest minor version when the dependency has a caret', async () => {
19-
await installDeps(installDir, ['debug@^2.0.0']);
20+
await installDeps(PACKAGE_MANAGERS['npm'], installDir, ['debug@^2.0.0']);
2021

2122
const packageJSON = await import(path.resolve(installDir, 'node_modules', 'debug', 'package.json'));
2223
expect(packageJSON.version).not.toEqual('2.0.0');

‎packages/api/core/src/api/import.ts‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import path from 'node:path';
22

3-
import { resolvePackageManager, updateElectronDependency } from '@electron-forge/core-utils';
3+
import { PMDetails, resolvePackageManager, updateElectronDependency } from '@electron-forge/core-utils';
44
import { ForgeListrOptions, ForgeListrTaskFn } from '@electron-forge/shared-types';
55
import baseTemplate from '@electron-forge/template-base';
66
import { autoTrace } from '@electron-forge/tracer';
@@ -58,7 +58,7 @@ export default autoTrace(
5858
childTrace,
5959
{ dir = process.cwd(), interactive = false, confirmImport, shouldContinueOnExisting, shouldRemoveDependency, shouldUpdateScript, outDir }: ImportOptions
6060
): Promise<void> => {
61-
const listrOptions: ForgeListrOptions<unknown> = {
61+
const listrOptions: ForgeListrOptions<{ pm: PMDetails }> = {
6262
concurrent: false,
6363
rendererOptions: {
6464
collapseSubtasks: false,
@@ -188,12 +188,18 @@ export default autoTrace(
188188
await fs.writeJson(path.resolve(dir, 'package.json'), packageJSON, { spaces: 2 });
189189
};
190190

191-
return task.newListr(
191+
return task.newListr<{ pm: PMDetails }>(
192192
[
193+
{
194+
title: `Resolving package manager`,
195+
task: async (ctx, task) => {
196+
ctx.pm = await resolvePackageManager();
197+
task.title = `Resolving package manager: ${chalk.cyan(ctx.pm.executable)}`;
198+
},
199+
},
193200
{
194201
title: 'Installing dependencies',
195-
task: async (_, task) => {
196-
const pm = await resolvePackageManager();
202+
task: async ({ pm }, task) => {
197203
await writeChanges();
198204

199205
d('deleting old dependencies forcefully');
@@ -202,15 +208,15 @@ export default autoTrace(
202208

203209
d('installing dependencies');
204210
task.output = `${pm.executable} ${pm.install} ${importDeps.join(' ')}`;
205-
await installDepList(dir, importDeps);
211+
await installDepList(pm, dir, importDeps);
206212

207213
d('installing devDependencies');
208214
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${importDevDeps.join(' ')}`;
209-
await installDepList(dir, importDevDeps, DepType.DEV);
215+
await installDepList(pm, dir, importDevDeps, DepType.DEV);
210216

211217
d('installing devDependencies with exact versions');
212218
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${pm.exact} ${importExactDevDeps.join(' ')}`;
213-
await installDepList(dir, importExactDevDeps, DepType.DEV, DepVersionRestriction.EXACT);
219+
await installDepList(pm, dir, importExactDevDeps, DepType.DEV, DepVersionRestriction.EXACT);
214220
},
215221
},
216222
{

‎packages/api/core/src/api/init-scripts/init-link.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import path from 'node:path';
22

3-
import { resolvePackageManager, spawnPackageManager } from '@electron-forge/core-utils';
3+
import { PMDetails, spawnPackageManager } from '@electron-forge/core-utils';
44
import { ForgeListrTask } from '@electron-forge/shared-types';
55
import debug from 'debug';
66

@@ -17,20 +17,19 @@ const d = debug('electron-forge:init:link');
1717
* Note: `yarn link:prepare` needs to run first before dependencies can be
1818
* linked.
1919
*/
20-
export async function initLink<T>(dir: string, task?: ForgeListrTask<T>) {
20+
export async function initLink<T>(pm: PMDetails, dir: string, task?: ForgeListrTask<T>) {
2121
const shouldLink = process.env.LINK_FORGE_DEPENDENCIES_ON_INIT;
2222
if (shouldLink) {
2323
d('Linking forge dependencies');
2424
const packageJson = await readRawPackageJson(dir);
25-
const pm = await resolvePackageManager();
2625
// TODO(erickzhao): the `--link-folder` argument only works for `yarn`. Since this command is
2726
// only made for Forge contributors, it isn't a big deal if it doesn't work for other package managers,
2827
// but we should make it cleaner.
2928
const linkFolder = path.resolve(__dirname, '..', '..', '..', '..', '..', '..', '.links');
3029
for (const packageName of Object.keys(packageJson.devDependencies)) {
3130
if (packageName.startsWith('@electron-forge/')) {
3231
if (task) task.output = `${pm.executable} link --link-folder ${linkFolder} ${packageName}`;
33-
await spawnPackageManager(['link', '--link-folder', linkFolder, packageName], {
32+
await spawnPackageManager(pm, ['link', '--link-folder', linkFolder, packageName], {
3433
cwd: dir,
3534
});
3635
}

‎packages/api/core/src/api/init-scripts/init-npm.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import path from 'node:path';
22

3-
import { resolvePackageManager } from '@electron-forge/core-utils';
3+
import { PMDetails } from '@electron-forge/core-utils';
44
import { ForgeListrTask } from '@electron-forge/shared-types';
55
import debug from 'debug';
66
import fs from 'fs-extra';
@@ -27,19 +27,18 @@ export const devDeps = [
2727
];
2828
export const exactDevDeps = ['electron'];
2929

30-
export const initNPM = async <T>(dir: string, task: ForgeListrTask<T>): Promise<void> => {
30+
export const initNPM = async <T>(pm: PMDetails, dir: string, task: ForgeListrTask<T>): Promise<void> => {
3131
d('installing dependencies');
32-
const pm = await resolvePackageManager();
3332
task.output = `${pm.executable} ${pm.install} ${deps.join(' ')}`;
34-
await installDepList(dir, deps);
33+
await installDepList(pm, dir, deps);
3534

3635
d('installing devDependencies');
3736
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${deps.join(' ')}`;
38-
await installDepList(dir, devDeps, DepType.DEV);
37+
await installDepList(pm, dir, devDeps, DepType.DEV);
3938

4039
d('installing exact devDependencies');
4140
for (const packageName of exactDevDeps) {
4241
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${pm.exact} ${packageName}`;
43-
await installDepList(dir, [packageName], DepType.DEV, DepVersionRestriction.EXACT);
42+
await installDepList(pm, dir, [packageName], DepType.DEV, DepVersionRestriction.EXACT);
4443
}
4544
};

0 commit comments

Comments
 (0)