Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(cli): minimize resolvePackageManager calls #3853

Merged
merged 8 commits into from
Feb 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/api/cli/spec/util/check-system.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('checkPackageManager', () => {
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockImplementation((args) => {
vi.mocked(spawnPackageManager).mockImplementation((_pm, args) => {
if (args?.join(' ') === 'config get node-linker') {
return Promise.resolve('isolated');
} else if (args?.join(' ') === 'config get hoist-pattern') {
Expand All @@ -91,7 +91,7 @@ describe('checkPackageManager', () => {
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockImplementation((args) => {
vi.mocked(spawnPackageManager).mockImplementation((_pm, args) => {
if (args?.join(' ') === 'config get node-linker') {
return Promise.resolve('isolated');
} else if (args?.join(' ') === `config get ${cfg}`) {
Expand Down
11 changes: 6 additions & 5 deletions packages/api/cli/src/util/check-system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { exec } from 'node:child_process';
import os from 'node:os';
import path from 'node:path';

import { resolvePackageManager, spawnPackageManager, SupportedPackageManager } from '@electron-forge/core-utils';
import { PACKAGE_MANAGERS, resolvePackageManager, spawnPackageManager, SupportedPackageManager } from '@electron-forge/core-utils';
import { ForgeListrTask } from '@electron-forge/shared-types';
import debug from 'debug';
import fs from 'fs-extra';
Expand Down Expand Up @@ -36,8 +36,9 @@ async function checkNodeVersion() {
* configuration purposes.
*/
async function checkPnpmConfig() {
const hoistPattern = await spawnPackageManager(['config', 'get', 'hoist-pattern']);
const publicHoistPattern = await spawnPackageManager(['config', 'get', 'public-hoist-pattern']);
const { pnpm } = PACKAGE_MANAGERS;
const hoistPattern = await spawnPackageManager(pnpm, ['config', 'get', 'hoist-pattern']);
const publicHoistPattern = await spawnPackageManager(pnpm, ['config', 'get', 'public-hoist-pattern']);

if (hoistPattern !== 'undefined' || publicHoistPattern !== 'undefined') {
d(
Expand All @@ -49,7 +50,7 @@ async function checkPnpmConfig() {
return;
}

const nodeLinker = await spawnPackageManager(['config', 'get', 'node-linker']);
const nodeLinker = await spawnPackageManager(pnpm, ['config', 'get', 'node-linker']);
if (nodeLinker !== 'hoisted') {
throw new Error(
'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.'
Expand All @@ -74,7 +75,7 @@ const ALLOWLISTED_VERSIONS: Record<SupportedPackageManager, Record<string, strin

export async function checkPackageManager() {
const pm = await resolvePackageManager();
const version = pm.version ?? (await spawnPackageManager(['--version']));
const version = pm.version ?? (await spawnPackageManager(pm, ['--version']));
const versionString = version.toString().trim();

const range = ALLOWLISTED_VERSIONS[pm.executable][process.platform] ?? ALLOWLISTED_VERSIONS[pm.executable].all;
Expand Down
40 changes: 14 additions & 26 deletions packages/api/core/spec/fast/util/install-dependencies.spec.ts
Original file line number Diff line number Diff line change
@@ -1,63 +1,51 @@
import { resolvePackageManager, spawnPackageManager } from '@electron-forge/core-utils';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { PACKAGE_MANAGERS, spawnPackageManager } from '@electron-forge/core-utils';
import { describe, expect, it, vi } from 'vitest';

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

vi.mock(import('@electron-forge/core-utils'), async (importOriginal) => {
const mod = await importOriginal();
return {
...mod,
resolvePackageManager: vi.fn(),
spawnPackageManager: vi.fn(),
};
});

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

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

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

describe.each([
{ executable: 'npm' as const, install: 'install', exact: '--save-exact', dev: '--save-dev' },
{ executable: 'yarn' as const, install: 'add', exact: '--exact', dev: '--dev' },
{ executable: 'pnpm' as const, install: 'install', exact: '--save-exact', dev: '--save-dev' },
])('$executable', (args) => {
beforeEach(() => {
vi.mocked(resolvePackageManager).mockResolvedValue(args);
});

describe.each([PACKAGE_MANAGERS['npm'], PACKAGE_MANAGERS['yarn'], PACKAGE_MANAGERS['pnpm']])('$executable', (pm) => {
it('should install deps', async () => {
await installDependencies('mydir', ['react']);
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'react'], expect.anything());
await installDependencies(pm, 'mydir', ['react']);
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'react'], expect.anything());
});

it('should install dev deps', async () => {
await installDependencies('mydir', ['eslint'], DepType.DEV);
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'eslint', args.dev], expect.anything());
await installDependencies(pm, 'mydir', ['eslint'], DepType.DEV);
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'eslint', pm.dev], expect.anything());
});

it('should install exact deps', async () => {
await installDependencies('mydir', ['react'], DepType.PROD, DepVersionRestriction.EXACT);
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'react', args.exact], expect.anything());
await installDependencies(pm, 'mydir', ['react'], DepType.PROD, DepVersionRestriction.EXACT);
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'react', pm.exact], expect.anything());
});

it('should install exact dev deps', async () => {
await installDependencies('mydir', ['eslint'], DepType.DEV, DepVersionRestriction.EXACT);
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'eslint', args.dev, args.exact], expect.anything());
await installDependencies(pm, 'mydir', ['eslint'], DepType.DEV, DepVersionRestriction.EXACT);
expect(spawnPackageManager).toHaveBeenCalledWith(pm, [pm.install, 'eslint', pm.dev, pm.exact], expect.anything());
});
});
});
23 changes: 10 additions & 13 deletions packages/api/core/spec/slow/api.slow.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import { execSync } from 'node:child_process';
import fs from 'node:fs';
import path from 'node:path';

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

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

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

beforeAll(async () => {
await spawnPackageManager(['run', 'link:prepare']);
process.env.NODE_INSTALLER = pm;
await spawnPackageManager(pm, ['run', 'link:prepare']);

if (pm === 'pnpm') {
await spawnPackageManager('config set node-linker hoisted'.split(' '));
if (pm.executable === 'pnpm') {
await spawnPackageManager(pm, 'config set node-linker hoisted'.split(' '));
}

return async () => {
await spawnPackageManager(['run', 'link:remove']);
await spawnPackageManager(pm, ['run', 'link:remove']);
delete process.env.NODE_INSTALLER;
};
});
Expand Down Expand Up @@ -187,7 +186,7 @@ describe.each([{ pm: 'npm' }, { pm: 'yarn' }, { pm: 'pnpm' }])(`init (with $pm)`
});

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

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

execSync(`${pm} install`, {
cwd: dir,
});
await spawnPackageManager(pm, ['install'], { cwd: dir });

await api.package({ dir });

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

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

return async () => {
await fs.promises.rm(path.resolve(dir, 'node_modules/ref-napi'), { recursive: true, force: true });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import fs from 'node:fs/promises';
import os from 'node:os';
import path from 'node:path';

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

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

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

const packageJSON = await import(path.resolve(installDir, 'node_modules', 'debug', 'package.json'));
expect(packageJSON.version).not.toEqual('2.0.0');
Expand Down
22 changes: 14 additions & 8 deletions packages/api/core/src/api/import.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'node:path';

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

return task.newListr(
return task.newListr<{ pm: PMDetails }>(
[
{
title: `Resolving package manager`,
task: async (ctx, task) => {
ctx.pm = await resolvePackageManager();
task.title = `Resolving package manager: ${chalk.cyan(ctx.pm.executable)}`;
},
},
{
title: 'Installing dependencies',
task: async (_, task) => {
const pm = await resolvePackageManager();
task: async ({ pm }, task) => {
await writeChanges();

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

d('installing dependencies');
task.output = `${pm.executable} ${pm.install} ${importDeps.join(' ')}`;
await installDepList(dir, importDeps);
await installDepList(pm, dir, importDeps);

d('installing devDependencies');
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${importDevDeps.join(' ')}`;
await installDepList(dir, importDevDeps, DepType.DEV);
await installDepList(pm, dir, importDevDeps, DepType.DEV);

d('installing devDependencies with exact versions');
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${pm.exact} ${importExactDevDeps.join(' ')}`;
await installDepList(dir, importExactDevDeps, DepType.DEV, DepVersionRestriction.EXACT);
await installDepList(pm, dir, importExactDevDeps, DepType.DEV, DepVersionRestriction.EXACT);
},
},
{
Expand Down
7 changes: 3 additions & 4 deletions packages/api/core/src/api/init-scripts/init-link.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'node:path';

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

Expand All @@ -17,20 +17,19 @@ const d = debug('electron-forge:init:link');
* Note: `yarn link:prepare` needs to run first before dependencies can be
* linked.
*/
export async function initLink<T>(dir: string, task?: ForgeListrTask<T>) {
export async function initLink<T>(pm: PMDetails, dir: string, task?: ForgeListrTask<T>) {
const shouldLink = process.env.LINK_FORGE_DEPENDENCIES_ON_INIT;
if (shouldLink) {
d('Linking forge dependencies');
const packageJson = await readRawPackageJson(dir);
const pm = await resolvePackageManager();
// TODO(erickzhao): the `--link-folder` argument only works for `yarn`. Since this command is
// only made for Forge contributors, it isn't a big deal if it doesn't work for other package managers,
// but we should make it cleaner.
const linkFolder = path.resolve(__dirname, '..', '..', '..', '..', '..', '..', '.links');
for (const packageName of Object.keys(packageJson.devDependencies)) {
if (packageName.startsWith('@electron-forge/')) {
if (task) task.output = `${pm.executable} link --link-folder ${linkFolder} ${packageName}`;
await spawnPackageManager(['link', '--link-folder', linkFolder, packageName], {
await spawnPackageManager(pm, ['link', '--link-folder', linkFolder, packageName], {
cwd: dir,
});
}
Expand Down
11 changes: 5 additions & 6 deletions packages/api/core/src/api/init-scripts/init-npm.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'node:path';

import { resolvePackageManager } from '@electron-forge/core-utils';
import { PMDetails } from '@electron-forge/core-utils';
import { ForgeListrTask } from '@electron-forge/shared-types';
import debug from 'debug';
import fs from 'fs-extra';
Expand All @@ -27,19 +27,18 @@ export const devDeps = [
];
export const exactDevDeps = ['electron'];

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

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

d('installing exact devDependencies');
for (const packageName of exactDevDeps) {
task.output = `${pm.executable} ${pm.install} ${pm.dev} ${pm.exact} ${packageName}`;
await installDepList(dir, [packageName], DepType.DEV, DepVersionRestriction.EXACT);
await installDepList(pm, dir, [packageName], DepType.DEV, DepVersionRestriction.EXACT);
}
};
Loading