-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
base: main
Are you sure you want to change the base?
feat(core): pnpm support #3822
Conversation
@@ -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" |
There was a problem hiding this comment.
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: | ||
name: 'Install pnpm' | ||
command: | | ||
npm install -g [email protected] |
There was a problem hiding this comment.
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.
all: '>= 1.0.0', | ||
}, | ||
pnpm: { | ||
all: '>= 8.0.0', |
There was a problem hiding this comment.
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
|
||
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); | ||
} | ||
|
||
async function checkPackageManagerVersion() { | ||
const version = await forgeUtils.yarnOrNpmSpawn(['--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}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions were refactored out into a single checkPackageManager
utility.
hasYarn = hasYarn; | ||
|
||
yarnOrNpmSpawn = yarnOrNpmSpawn; | ||
spawnPackageManager = spawnPackageManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a change in API signatures for @electron-forge/core-utils
a breaking change? If it is, I can add a temporary shim for these utils and deprecate them.
0750cd1
to
c023fcf
Compare
@only-issues it hasn't gotten any PR approvals yet, so not yet. |
@@ -15,12 +15,12 @@ describe('ViteTypeScriptTemplate', () => { | |||
let dir: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can use context to avoid mutable variables.
This suggestion is beyond the scope of the current PR, just a side note :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely worth exploring in a follow-up PR!
vi.spyOn(console, 'warn').mockImplementation(() => undefined); | ||
}); | ||
afterEach(() => { | ||
if (!nodeInstaller) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this. Is it a mistake? Based on the context, I feel it should be written as: if (nodeInstaller)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added clarifying comments in 519aa9f (because I got confused again myself when skimming through my implementation), but the gist of it is:
- Before the test, we store the initial
process.env.NODE_INSTALLER
value. - During the test,
process.env.NODE_INSTALLER
gets added in the test arrangement code. - After the test, we restore the initial value. If
process.env.NODE_INSTALLER
wasn't present before the test ran, it gets deleted again. Otherwise, it gets restored to the initial value we saved before the test.
Co-authored-by: Kevin Cui <[email protected]>
Co-authored-by: Kevin Cui <[email protected]>
Co-authored-by: Kevin Cui <[email protected]>
Co-authored-by: Kevin Cui <[email protected]>
Co-authored-by: Kevin Cui <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your work!
Co-authored-by: Erik Moura <[email protected]>
This reverts commit 8fb93be.
Co-authored-by: Erik Moura <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just some final notes about templates + workspaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I've been testing how templates interact with workspaces using the script below. I don't recall right now why I gave up on using yarn link:prepare
to use the local Forge packages, but something wasn't quite working and I did some controversial overrides
so I could at least do some basic testing. Before running this script, I do a clean Forge install/build with git clean -dfx && yarn && yarn build
:
#!/usr/bin/env bash
PNPM_WORKSPACE_ROOT=forge-pnpm
FORGE_ROOT=~/Documents/electron/forge
# optional cleanup
# rm -rf $PNPM_WORKSPACE_ROOT || true
mkdir $PNPM_WORKSPACE_ROOT
cd $PNPM_WORKSPACE_ROOT
# Create a basic `package.json`
pnpm init
# Use local Forge packages - all of them
npm pkg set pnpm.overrides."@electron-forge/cli"=$FORGE_ROOT/packages/api/cli
npm pkg set pnpm.overrides."@electron-forge/core"=$FORGE_ROOT/packages/api/core
npm pkg set pnpm.overrides."@electron-forge/maker-appx"=$FORGE_ROOT/packages/maker/appx
npm pkg set pnpm.overrides."@electron-forge/maker-base"=$FORGE_ROOT/packages/maker/base
npm pkg set pnpm.overrides."@electron-forge/maker-deb"=$FORGE_ROOT/packages/maker/deb
npm pkg set pnpm.overrides."@electron-forge/maker-dmg"=$FORGE_ROOT/packages/maker/dmg
npm pkg set pnpm.overrides."@electron-forge/maker-flatpak"=$FORGE_ROOT/packages/maker/flatpak
npm pkg set pnpm.overrides."@electron-forge/maker-pkg"=$FORGE_ROOT/packages/maker/pkg
npm pkg set pnpm.overrides."@electron-forge/maker-rpm"=$FORGE_ROOT/packages/maker/rpm
npm pkg set pnpm.overrides."@electron-forge/maker-snap"=$FORGE_ROOT/packages/maker/snap
npm pkg set pnpm.overrides."@electron-forge/maker-squirrel"=$FORGE_ROOT/packages/maker/squirrel
npm pkg set pnpm.overrides."@electron-forge/maker-wix"=$FORGE_ROOT/packages/maker/wix
npm pkg set pnpm.overrides."@electron-forge/maker-zip"=$FORGE_ROOT/packages/maker/zip
npm pkg set pnpm.overrides."@electron-forge/plugin-auto-unpack-natives"=$FORGE_ROOT/packages/plugin/auto-unpack-natives
npm pkg set pnpm.overrides."@electron-forge/plugin-base"=$FORGE_ROOT/packages/plugin/base
npm pkg set pnpm.overrides."@electron-forge/plugin-electronegativity"=$FORGE_ROOT/packages/plugin/electronegativity
npm pkg set pnpm.overrides."@electron-forge/plugin-fuses"=$FORGE_ROOT/packages/plugin/fuses
npm pkg set pnpm.overrides."@electron-forge/plugin-local-electron"=$FORGE_ROOT/packages/plugin/local-electron
npm pkg set pnpm.overrides."@electron-forge/plugin-vite"=$FORGE_ROOT/packages/plugin/vite
npm pkg set pnpm.overrides."@electron-forge/plugin-webpack"=$FORGE_ROOT/packages/plugin/webpack
npm pkg set pnpm.overrides."@electron-forge/publisher-base-static"=$FORGE_ROOT/packages/publisher/base-static
npm pkg set pnpm.overrides."@electron-forge/publisher-base"=$FORGE_ROOT/packages/publisher/base
npm pkg set pnpm.overrides."@electron-forge/publisher-bitbucket"=$FORGE_ROOT/packages/publisher/bitbucket
npm pkg set pnpm.overrides."@electron-forge/publisher-electron-release-server"=$FORGE_ROOT/packages/publisher/electron-release-server
npm pkg set pnpm.overrides."@electron-forge/publisher-gcs"=$FORGE_ROOT/packages/publisher/gcs
npm pkg set pnpm.overrides."@electron-forge/publisher-github"=$FORGE_ROOT/packages/publisher/github
npm pkg set pnpm.overrides."@electron-forge/publisher-nucleus"=$FORGE_ROOT/packages/publisher/nucleus
npm pkg set pnpm.overrides."@electron-forge/publisher-s3"=$FORGE_ROOT/packages/publisher/s3
npm pkg set pnpm.overrides."@electron-forge/publisher-snapcraft"=$FORGE_ROOT/packages/publisher/snapcraft
npm pkg set pnpm.overrides."@electron-forge/template-base"=$FORGE_ROOT/packages/template/base
npm pkg set pnpm.overrides."@electron-forge/template-vite-typescript"=$FORGE_ROOT/packages/template/vite-typescript
npm pkg set pnpm.overrides."@electron-forge/template-vite"=$FORGE_ROOT/packages/template/vite
npm pkg set pnpm.overrides."@electron-forge/template-webpack-typescript"=$FORGE_ROOT/packages/template/webpack-typescript
npm pkg set pnpm.overrides."@electron-forge/template-webpack"=$FORGE_ROOT/packages/template/webpack
npm pkg set pnpm.overrides."@electron-forge/core-utils"=$FORGE_ROOT/packages/utils/core-utils
npm pkg set pnpm.overrides."@electron-forge/test-utils"=$FORGE_ROOT/packages/utils/test-utils
npm pkg set pnpm.overrides."@electron-forge/tracer"=$FORGE_ROOT/packages/utils/tracer
npm pkg set pnpm.overrides."@electron-forge/shared-types"=$FORGE_ROOT/packages/utils/types
npm pkg set pnpm.overrides."@electron-forge/web-multi-logger"=$FORGE_ROOT/packages/utils/web-multi-logger
npm pkg set pnpm.overrides."@electron-forge/create-electron-app"=$FORGE_ROOT/packages/external/create-electron-app
# Set up a pnpm workspace
echo "packages:
- 'packages/*'
" > pnpm-workspace.yaml
# Uncomment to create the `pnpm-lock.yaml` file before initializing the template
# pnpm install
# Initialize a Forge template in `./packages/desktop` using the local `create-electron-app` package
pnpm create electron-app@$FORGE_ROOT/packages/external/create-electron-app packages/desktop
Some test findings:
- running
pnpm install
in the workspace root printsWARN The field "pnpm.onlyBuiltDependencies" was found in /Users/ian/Documents/electron/repros/forge-pnpm/packages/desktop/package.json. This will not take effect. You should configure "pnpm.onlyBuiltDependencies" at the root of the workspace instead.
, so we'd need to set up that field in the workspace root when initializing templates. - similarly, running
pnpm start
inpackages/desktop
prints the✖ When using pnpm, `node-linker` must be set to "hoisted". Run `pnpm config set node-linker hoisted` to set this config value.
error, so I think the.npmrc
file should also live in the workspace root. - running
pnpm package
inpackages/desktop
fails withError: Failed to locate module "electron-squirrel-startup" from "/Users/ian/Documents/electron/repros/forge-pnpm/packages/desktop"
. This might be related to my weird local setup (or the fact that I'm testing on macOS), but could also be a side-effect of not havingnode-linker = hoisted
in the workspace root.
I have a question about use of Presumably another option for this is to try and get PNPM support for hoisting dependencies on a per-workspace / package basis. |
Hey @goosewobbler, thanks for bringing that up.
I think I can amend the PR to change that! |
Nice, there's also |
Added exceptions for |
Closes #2633
Closes #3574
ref #3813
This PR aims to provide a basic level of pnpm support for Electron Forge.
Context
Under the hood, Forge's
init
andimport
commands use a Node.js package manager to install dependencies and otherwise execute commands. The logic is set up to useyarn
if detected, and falls back tonpm
otherwise.With the proliferation of alternative package managers in recent years, one of the most popular requests on our issue tracker was to support pnpm (#2633).
However, this required a decent amount of refactoring since we relied on the unmaintained (
yarn-or-npm
) package to perform this package manager detection. This locked us out of support for other package managers until this utility was completely refactored.Prior art
Big thanks to @makeryi and @goosewobbler for opening their respective PRs attempting support (#3351 and #3574).
Based on their efforts, I first landed #3813, which replacesyarn-or-npm
with the more generalizeddetect-package-manager
package without changing the API signature for our utility APIs.(This utility has also been replaced. See next section.)
This PR follows that up by replacing our package manager utilities (which still only supported yarn or npm) with a generic package manager detection system that returns the current package manager as well as its associated commands and flags.
Implementation
Package manager detection
The core of this PR refactors the
yarn-or-npm
file in@electron-forge/core-utils
to instead be a genericpackage-manager
util.Instead of returning just the name of the package manager, resolving which package manager to use also returns its
install
command as well as the command flags we may want to use.forge/packages/utils/core-utils/src/package-manager.ts
Lines 6 to 28 in 1592d85
Note
In theory, we could do away with these objects for now because the shorthand
<pm> install -E -D
works across npm, yarn, and pnpm. However, this setup future-proofs us against future commands/flags that we'd want to add that aren't necessarily compatible.The behaviour for
resolvePackageManager
now differs.If an unsupported package manager is detected viaNODE_INSTALLER
ordetect-package-manager
, we default tonpm
instead of whatever the detected system package manager is.process.env.NODE_INSTALLER
is detected, use its value.process.env.npm_config_user_agent
is detected, parse the package manager and its version from the user agent.npm
.This solves the case where we'd detect an unsupported package manager (e.g.
bun
).node-linker=hoisted
Out of the box, pnpm provides improved disk space efficiency and install speed because it symlinks all dependencies and stores them in a central location on disk (see Motivation section of the pnpm docs for more details).
However, Forge expects
node_modules
to exist on disk at specific locations because we bundle all production npm dependencies into the Electron app when packaging.To that end, we expect the config to be
node-linker=hoisted
when running Electron Forge. I added a clause tocheck-system.ts
to ensure this by checking the value ofpnpm config get node-linker
.forge/packages/api/cli/src/util/check-system.ts
Lines 30 to 36 in 1592d85
This setting is added out of the box when initializing Forge templates with
pnpm
via.npmrc
:forge/packages/template/base/tmpl/.npmrc
Line 1 in 1592d85
forge/packages/template/base/src/BaseTemplate.ts
Lines 65 to 67 in 1592d85
pnpm workspaces
I think we actually already supported pnpm workspaces via:
forge/packages/utils/core-utils/src/electron-version.ts
Lines 21 to 33 in f084010
Supported pnpm version ranges
I'm not sure if
v8.0.0
is a correct lower bound for pnpm, but it's the lowest version they have for their documentation so I'm assuming something has to do with their support policy.Testing
This PR expands the existing test suite to also support
pnpm
(mostly inapi.slow.spec.ts
).Note
Previously, we actually didn't test packaging/making for
npm
. This PR has the side effect of fully testing the API across all supported package managers, which does bring up the Time to Green for our CI.