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

feat(core): pnpm support #3822

Merged
merged 37 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7c77355
chore: rename yarn-or-npm -> package-manager
erickzhao Jan 24, 2025
2f6ef3d
refactor: remove hasYarn function and rename spawn
erickzhao Jan 24, 2025
c70224d
pnpm support take 1
erickzhao Jan 28, 2025
0acb1f3
install pnpm in CI
erickzhao Jan 28, 2025
284e219
reverse order of link:prepare
erickzhao Jan 28, 2025
1d85d64
attempt to remove corepack for now
erickzhao Jan 28, 2025
431116d
hoist it!
erickzhao Jan 28, 2025
cafacc3
downgrade to pnpm 9
erickzhao Jan 28, 2025
a19a6ea
adjustments
erickzhao Jan 28, 2025
15db452
copyFile instead of rename
erickzhao Jan 28, 2025
89a0fd1
add additional checks
erickzhao Jan 29, 2025
d1547a8
Merge branch 'main' into pnpmpnpmpnpmpnpm
erickzhao Jan 29, 2025
e23518f
use default reporter in CI
erickzhao Jan 29, 2025
dacb4e5
Merge branch 'pnpmpnpmpnpmpnpm' of github.com:electron/forge into pnp…
erickzhao Jan 29, 2025
93c0ef8
fix [object Object]
erickzhao Jan 29, 2025
3dc34f3
Merge branch 'main' into pnpmpnpmpnpmpnpm
erickzhao Jan 29, 2025
dccf638
Merge branch 'pnpmpnpmpnpmpnpm' of github.com:electron/forge into pnp…
erickzhao Jan 29, 2025
8b0e22f
add back `packageManager` and work around with `.npmrc` setting
erickzhao Jan 29, 2025
1592d85
add missing mock :)
erickzhao Jan 29, 2025
d5a43a2
fix CLI output for initNPM commands
erickzhao Jan 30, 2025
64ac631
Merge branch 'main' into pnpmpnpmpnpmpnpm
erickzhao Feb 3, 2025
d695b26
use npm_config_user_agent
erickzhao Feb 4, 2025
7a5ab70
Update .npmrc
erickzhao Feb 5, 2025
3350dfe
Update packages/template/base/tmpl/.npmrc
erickzhao Feb 5, 2025
65458b3
Update packages/utils/core-utils/src/package-manager.ts
erickzhao Feb 5, 2025
1b084df
clarify todo with my name
erickzhao Feb 5, 2025
336662c
Update packages/api/core/spec/slow/api.slow.spec.ts
erickzhao Feb 5, 2025
33454a5
Update packages/utils/core-utils/spec/package-manager.spec.ts
erickzhao Feb 5, 2025
514f2a1
Merge branch 'pnpmpnpmpnpmpnpm' of github.com:electron/forge into pnp…
erickzhao Feb 5, 2025
f3ff623
afterall
erickzhao Feb 5, 2025
519aa9f
clarify cleanup function
erickzhao Feb 5, 2025
8fb93be
Update packages/utils/core-utils/spec/package-manager.spec.ts
erickzhao Feb 6, 2025
12168ac
Revert "Update packages/utils/core-utils/spec/package-manager.spec.ts"
erickzhao Feb 6, 2025
f19e1cb
Update packages/utils/core-utils/spec/package-manager.spec.ts
erickzhao Feb 6, 2025
67ff824
Merge branch 'pnpmpnpmpnpmpnpm' of github.com:electron/forge into pnp…
erickzhao Feb 6, 2025
2e96421
adjust tests and findUp
erickzhao Feb 6, 2025
7825022
support `hoist-pattern` and `public-hoist-pattern` as well
erickzhao Feb 7, 2025
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
12 changes: 10 additions & 2 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ commands:
- run:
name: 'Run fast tests'
command: |
yarn test:fast --reporter=junit --outputFile="./reports/out/test_output.xml"
yarn test:fast --reporter=default --reporter=junit --outputFile="./reports/out/test_output.xml"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the default reporter both gives us a clearer idea of the failures in CI and prevents us from hitting the 10-minute CircleCI no output timeout.

run-slow-tests:
steps:
Expand All @@ -57,7 +57,7 @@ commands:
- run:
name: 'Run slow tests'
command: |
yarn test:slow --reporter=junit --outputFile="./reports/out/test_output.xml"
yarn test:slow --reporter=default --reporter=junit --outputFile="./reports/out/test_output.xml"
jobs:
lint-and-build:
Expand Down Expand Up @@ -110,6 +110,10 @@ jobs:
libgtk-3-0 \
libgbm1
sudo add-apt-repository -y ppa:alexlarsson/flatpak
- run:
name: 'Install pnpm'
command: |
npm install -g [email protected]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also do this in corepack in theory, but I found this to be easier to grok.

- run-fast-tests
- store_test_results:
path: ./reports/
Expand Down Expand Up @@ -155,6 +159,10 @@ jobs:
libgdk-pixbuf2.0-dev \
libgtk-3-0 \
libgbm1
- run:
name: 'Install pnpm'
command: |
npm install -g [email protected]
- run-slow-tests
- run:
when: always # the report is generated on pass or fail
Expand Down
2 changes: 2 additions & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Required to run pnpm in tests because `packageManager` is set to `yarn` in `package.json`
package-manager-strict=false
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
"cross-spawn": "^7.0.3",
"cross-zip": "^4.0.0",
"debug": "^4.3.1",
"detect-package-manager": "^3.0.2",
"express": "^4.17.1",
"express-ws": "^5.0.2",
"fast-glob": "^3.2.7",
Expand Down
1 change: 1 addition & 0 deletions packages/api/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
},
"dependencies": {
"@electron-forge/core": "7.6.1",
"@electron-forge/core-utils": "7.6.1",
"@electron-forge/shared-types": "7.6.1",
"@electron/get": "^3.0.0",
"chalk": "^4.0.0",
Expand Down
122 changes: 112 additions & 10 deletions packages/api/cli/spec/check-system.spec.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,121 @@
import { describe, expect, it } from 'vitest';
import { resolvePackageManager, spawnPackageManager } from '@electron-forge/core-utils';
import { describe, expect, it, vi } from 'vitest';

import { checkValidPackageManagerVersion } from '../src/util/check-system';
import { checkPackageManager } from '../src/util/check-system';

describe('check-system', () => {
describe('validPackageManagerVersion', () => {
it('should consider whitelisted versions to be valid', () => {
expect(() => checkValidPackageManagerVersion('NPM', '3.10.1', '^3.0.0')).not.toThrow();
vi.mock(import('@electron-forge/core-utils'), async (importOriginal) => {
const mod = await importOriginal();
return {
...mod,
resolvePackageManager: vi.fn(),
spawnPackageManager: vi.fn(),
};
});

describe('checkPackageManager', () => {
it('should consider allowlisted versions to be valid', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'npm',
install: 'install',
dev: '--save-dev',
exact: '--save-exact',
});
vi.mocked(spawnPackageManager).mockResolvedValue('10.9.2');
await expect(checkPackageManager()).resolves.not.toThrow();
});

it('rejects versions that are outside of the supported range', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'yarn',
install: 'add',
dev: '--dev',
exact: '--exact',
});

it('should consider Yarn nightly versions to be invalid', () => {
expect(() => checkValidPackageManagerVersion('Yarn', '0.23.0-20170311.0515', '0.23.0')).toThrow();
// yarn 0.x unsupported
vi.mocked(spawnPackageManager).mockResolvedValue('0.22.0');
await expect(checkPackageManager()).rejects.toThrow();
});

it('should consider Yarn nightly versions to be invalid', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'yarn',
install: 'add',
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockResolvedValue('0.23.0-20170311.0515');
await expect(checkPackageManager()).rejects.toThrow();
});

it('should consider invalid semver versions to be invalid', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'yarn',
install: 'add',
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockResolvedValue('1.22');
await expect(checkPackageManager()).rejects.toThrow();
});

it('should throw if using pnpm without node-linker=hoisted or custom hoist-pattern', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'pnpm',
install: 'add',
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockImplementation((args) => {
if (args?.join(' ') === 'config get node-linker') {
return Promise.resolve('isolated');
} else if (args?.join(' ') === 'config get hoist-pattern') {
return Promise.resolve('undefined');
} else if (args?.join(' ') === 'config get public-hoist-pattern') {
return Promise.resolve('undefined');
} else if (args?.join(' ') === '--version') {
return Promise.resolve('10.0.0');
} else {
throw new Error('Unexpected command');
}
});
await expect(checkPackageManager()).rejects.toThrow(
'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.'
);
});

it.each(['hoist-pattern', 'public-hoist-pattern'])('should pass without validation if user has set %s in their pnpm config', async (cfg) => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'pnpm',
install: 'add',
dev: '--dev',
exact: '--exact',
});
vi.mocked(spawnPackageManager).mockImplementation((args) => {
if (args?.join(' ') === 'config get node-linker') {
return Promise.resolve('isolated');
} else if (args?.join(' ') === `config get ${cfg}`) {
return Promise.resolve('["*eslint*","*babel*"]');
} else if (args?.join(' ') === '--version') {
return Promise.resolve('10.0.0');
} else {
return Promise.resolve('undefined');
}
});
await expect(checkPackageManager()).resolves.not.toThrow();
});

it('should consider invalid semver versions to be invalid', () => {
expect(() => checkValidPackageManagerVersion('Yarn', '0.22', '0.22.0')).toThrow();
// resolvePackageManager optionally returns a `version` if `npm_config_user_agent` was used to
// resolve the package manager being used.
it('should not shell out to child process if version was already parsed via npm_config_user_agent', async () => {
vi.mocked(resolvePackageManager).mockResolvedValue({
executable: 'npm',
install: 'install',
dev: '--save-dev',
exact: '--save-exact',
version: '10.9.2',
});
await expect(checkPackageManager()).resolves.not.toThrow();
expect(spawnPackageManager).not.toHaveBeenCalled();
});
});
91 changes: 59 additions & 32 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 { utils as forgeUtils } from '@electron-forge/core';
import { 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 All @@ -27,43 +27,70 @@ async function checkNodeVersion() {
return process.versions.node;
}

const NPM_ALLOWLISTED_VERSIONS = {
all: '^3.0.0 || ^4.0.0 || ~5.1.0 || ~5.2.0 || >= 5.4.2',
darwin: '>= 5.4.0',
linux: '>= 5.4.0',
};
const YARN_ALLOWLISTED_VERSIONS = {
all: '>= 1.0.0',
};
/**
* Packaging an app with Electron Forge requires `node_modules` to be on disk.
* With `pnpm`, this can be done in a few different ways.
*
* `node-linker=hoisted` replicates the behaviour of npm and Yarn Classic, while
* users may choose to set `public-hoist-pattern` or `hoist-pattern` for advanced
* configuration purposes.
*/
async function checkPnpmConfig() {
const hoistPattern = await spawnPackageManager(['config', 'get', 'hoist-pattern']);
const publicHoistPattern = await spawnPackageManager(['config', 'get', 'public-hoist-pattern']);

export function checkValidPackageManagerVersion(packageManager: string, version: string, allowlistedVersions: string) {
if (!semver.valid(version)) {
d(`Invalid semver-string while checking version: ${version}`);
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
throw new Error(`Could not check ${packageManager} version "${version}", assuming incompatible`);
if (hoistPattern !== 'undefined' || publicHoistPattern !== 'undefined') {
d(
`Custom hoist pattern detected ${JSON.stringify({
hoistPattern,
publicHoistPattern,
})}, assuming that the user has configured pnpm to package dependencies.`
);
return;
}
if (!semver.satisfies(version, allowlistedVersions)) {
throw new Error(`Incompatible version of ${packageManager} detected "${version}", must be in range ${allowlistedVersions}`);

const nodeLinker = await spawnPackageManager(['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.'
);
}
}

function warnIfPackageManagerIsntAKnownGoodVersion(packageManager: string, version: string, allowlistedVersions: { [key: string]: string }) {
const osVersions = allowlistedVersions[process.platform];
const versions = osVersions ? `${allowlistedVersions.all} || ${osVersions}` : allowlistedVersions.all;
const versionString = version.toString();
checkValidPackageManagerVersion(packageManager, versionString, versions);
}
// TODO(erickzhao): Drop antiquated versions of npm for Forge v8
const ALLOWLISTED_VERSIONS: Record<SupportedPackageManager, Record<string, string>> = {
npm: {
all: '^3.0.0 || ^4.0.0 || ~5.1.0 || ~5.2.0 || >= 5.4.2',
darwin: '>= 5.4.0',
linux: '>= 5.4.0',
},
yarn: {
all: '>= 1.0.0',
},
pnpm: {
all: '>= 8.0.0',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is necessary, but the Node compatibility matrix in the pnpm docs only lists up to pnpm 8 so I feel like this is a decent lower bound: https://pnpm.io/installation#compatibility

},
};

async function checkPackageManagerVersion() {
const version = await forgeUtils.yarnOrNpmSpawn(['--version']);
export async function checkPackageManager() {
const pm = await resolvePackageManager();
const version = pm.version ?? (await spawnPackageManager(['--version']));
const versionString = version.toString().trim();
if (await forgeUtils.hasYarn()) {
warnIfPackageManagerIsntAKnownGoodVersion('Yarn', versionString, YARN_ALLOWLISTED_VERSIONS);
return `yarn@${versionString}`;
} else {
warnIfPackageManagerIsntAKnownGoodVersion('NPM', versionString, NPM_ALLOWLISTED_VERSIONS);
return `npm@${versionString}`;

const range = ALLOWLISTED_VERSIONS[pm.executable][process.platform] ?? ALLOWLISTED_VERSIONS[pm.executable].all;
if (!semver.valid(version)) {
d(`Invalid semver-string while checking version: ${version}`);
throw new Error(`Could not check ${pm.executable} version "${version}", assuming incompatible`);
}
if (!semver.satisfies(version, range)) {
throw new Error(`Incompatible version of ${pm.executable} detected: "${version}" must be in range ${range}`);
}

if (pm.executable === 'pnpm') {
await checkPnpmConfig();
}

return `${pm.executable}@${versionString}`;
}

/**
Expand Down Expand Up @@ -106,9 +133,9 @@ export async function checkSystem(task: ForgeListrTask<never>) {
},
},
{
title: 'Checking packageManager version',
title: 'Checking package manager version',
task: async (_, task) => {
const packageManager = await checkPackageManagerVersion();
const packageManager = await checkPackageManager();
task.title = `Found ${packageManager}`;
},
},
Expand Down
1 change: 0 additions & 1 deletion packages/api/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"@malept/cross-spawn-promise": "^2.0.0",
"chalk": "^4.0.0",
"debug": "^4.3.1",
"detect-package-manager": "^3.0.2",
"fast-glob": "^3.2.7",
"filenamify": "^4.1.0",
"find-up": "^5.0.0",
Expand Down
36 changes: 20 additions & 16 deletions packages/api/core/spec/fast/util/install-dependencies.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { hasYarn, yarnOrNpmSpawn } from '@electron-forge/core-utils';
import { resolvePackageManager, spawnPackageManager } from '@electron-forge/core-utils';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import installDependencies, { DepType, DepVersionRestriction } from '../../../src/util/install-dependencies';
Expand All @@ -7,53 +7,57 @@ vi.mock(import('@electron-forge/core-utils'), async (importOriginal) => {
const mod = await importOriginal();
return {
...mod,
hasYarn: vi.fn(),
yarnOrNpmSpawn: vi.fn(),
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', []);
expect(yarnOrNpmSpawn).not.toHaveBeenCalled();
expect(spawnPackageManager).not.toHaveBeenCalled();
});

it('should reject if reject the promise if exit code is not 0', async () => {
vi.mocked(yarnOrNpmSpawn).mockRejectedValueOnce('fail');
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');
});

it('should resolve if reject the promise if exit code is 0', async () => {
vi.mocked(yarnOrNpmSpawn).mockResolvedValueOnce('pass');
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);
});

describe.each([
{ pm: 'npm', install: 'install', flags: { exact: '--save-exact', dev: '--save-dev' } },
{ pm: 'yarn', install: 'add', flags: { exact: '--exact', dev: '--dev' } },
])('$pm', ({ pm, install, flags }) => {
{ 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(hasYarn).mockResolvedValue(pm === 'yarn');
vi.mocked(resolvePackageManager).mockResolvedValue(args);
});

it('should install deps', async () => {
await installDependencies('mydir', ['react']);
expect(yarnOrNpmSpawn).toHaveBeenCalledWith([install, 'react'], expect.anything());
expect(spawnPackageManager).toHaveBeenCalledWith([args.install, 'react'], expect.anything());
});

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

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

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