migration to pnpm | 迁移到pnpm为了后续拆包做准备 #511
Conversation
refactor: migration to pnpm
fix: fix sharp on mac arm build by x64
|
Warning Rate limit exceeded@zerob13 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
""" WalkthroughThis update transitions the project's package management from npm to pnpm across all workflows, documentation, and scripts. It introduces a new platform-aware installation script for the sharp dependency, enforces pnpm usage via configuration and preinstall checks, and updates documentation to reflect pnpm-based commands. Workflow files and package.json are adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant pnpm
participant install-sharp-for-platform.js
participant pnpm-workspace.yaml
Developer->>GitHub Actions: Push/PR triggers workflow
GitHub Actions->>pnpm: Setup pnpm (v10.12.1)
GitHub Actions->>install-sharp-for-platform.js: Run install:sharp with platform/arch env
install-sharp-for-platform.js->>pnpm-workspace.yaml: Update supportedArchitectures
GitHub Actions->>pnpm: Install dependencies (pnpm install)
GitHub Actions->>pnpm: Run build/lint/scripts
Poem
""" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pnpm-workspace.yaml (1)
1-5: Workspace file is minimal – remember to addpackages:before publishing
pnpm-workspace.yamlusually declares the workspace packages. At the moment it
only containssupportedArchitectures, so pnpm will not link local packages.
Make sure apackages:section (e.g.packages:\n - "packages/*"\n) is added
before the next release.README.md (1)
263-276: MarkdownLint MD014 – drop leading$prompts or show sample outputStatic analysis flagged these lines because
$is used without showing the
command output. Either remove the prompt symbol or add expected output.-$ pnpm install -$ pnpm run installRuntime +$ pnpm install +$ pnpm run installRuntime🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
275-275: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
README.jp.md (1)
261-272: Same MD014 issue in Japanese READMEFor consistency with the main README and to silence markdownlint:
-$ pnpm install -$ pnpm run installRuntime +$ pnpm install +$ pnpm run installRuntime🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
271-271: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
.github/workflows/build.yml (1)
1-181: Consider CI performance and maintainability.
- Introduce caching of the PNPM store (
~/.pnpm-storeor equivalent) withactions/cacheto speed up installs.- Refactor duplicate setup steps (Node.js & PNPM installation, Sharp configuration) into a reusable workflow or composite action to reduce duplication.
package.json (2)
32-42: Add platform-specific build and Sharp scripts.The new
build:*scripts andinstall:sharphelper script (scripts/install-sharp-for-platform.js) cover all OS/architectures. Verify the trailing space in"build:mac:x64"(... --x64) doesn't introduce a runtime error.
22-23: Consider replacingnpxwithpnpm exec.Scripts like
"lint": "npx -y oxlint ."could instead usepnpm exec oxlintfor consistency and reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build.yml(4 hunks).github/workflows/prcheck.yml(1 hunks)CONTRIBUTING.md(3 hunks)CONTRIBUTING.zh.md(3 hunks)README.jp.md(1 hunks)README.md(1 hunks)README.zh.md(1 hunks)package.json(3 hunks)pnpm-workspace.yaml(1 hunks)scripts/install-sharp-for-platform.js(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
275-275: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
README.jp.md
271-271: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
README.zh.md
271-271: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
🪛 YAMLlint (1.37.1)
.github/workflows/prcheck.yml
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (41)
scripts/install-sharp-for-platform.js (1)
45-52: Consider falling back to a generic “current” matrix for unknown platformsWhen
platformKeyis not found the script exits with 0, leaving the workspace
untouched. On exotic architectures CI will later fail duringpnpm install
because Sharp cannot resolve a binary.A safer default is to set
supportedArchitectures: os: [current] cpu: [current]and emit a warning, rather than doing nothing.
README.zh.md (3)
262-263: Confirm documentation aligns withpnpmmigration.The install section correctly replaces
npm installwithpnpm installand adds theinstallRuntimestep. No residualnpmreferences remain here.
271-272: Verify development command switch topnpm.The dev command was updated to
pnpm run dev, which aligns with the migration. Ensure any downstream scripts or CI docs reference this.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
271-271: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
278-292: Check build instructions for all platforms.All build commands (
build:win,build:mac,build:linuxand their arch variants) have been updated topnpm run. Confirm these match the scripts inpackage.jsonand workflows.CONTRIBUTING.md (4)
23-23: Replacenpm run devwithpnpm run dev.The internal team dev start command correctly switched to
pnpm run dev. Confirm consistency in other docs.
87-88: Update dependency installation steps.The install instructions now invoke
pnpm installandpnpm run installRuntime. This aligns with the new setup process.
93-94: Switch external contributor dev startup topnpm.The command to start the dev server is correctly updated to
pnpm run dev.
113-116: Update code style verification commands.Lint, build, and i18n checks correctly use
pnpm run. Make sure that these scripts exist inpackage.json..github/workflows/prcheck.yml (6)
23-24: Pin Node.js version to 22.13.1.Updating the Node.js setup step to
22.13.1ensures alignment withpackage.jsonengines.
25-29: Addpnpmsetup step.Including
pnpm/action-setup@v2guarantees the correct PNPM version (10.12.1) in CI.
30-35: Configure Sharp installation via PNPM.Running
pnpm run install:sharpwith theTARGET_OSandTARGET_ARCHenv vars offloads sharp setup to the dedicated script. This replaces ad-hocnpm installcommands.
36-38: Install dependencies withpnpm install.Switching from
npm installtopnpm installis correct and consistent with the migration.
39-41: Run lint viapnpm run lint.The lint step is properly updated to use PNPM.
45-46: Update build step to use PNPM.Switching
buildtopnpm run buildaligns with the new scripts.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/build.yml (18)
35-36: Align Node.js version in Windows build.The Node.js setup is pinned to
22.13.1, matchingpackage.json's engines.
37-41: Add PNPM in Windows build.Initializing PNPM (
10.12.1) before dependency installation ensures consistent package management.
42-47: Configure Sharp for Windows.Using
pnpm run install:sharpwithTARGET_OS=win32and dynamicTARGET_ARCHreplicates the CI check workflow.
49-54: Install dependencies via PNPM.Switching from
npm installtopnpm installand propagating build env vars is correct.
55-57: Update Node runtime installation.The
installRuntime:winscript is invoked through PNPM, keeping the Windows injector step consistent.
59-64: Use PNPM for Windows build.The build step now calls
pnpm run build:win:${{ matrix.arch }}and correctly passes environment secrets.
86-90: Align Node.js version in Linux build.Node.js version is pinned identically to other jobs, ensuring uniformity.
91-95: Add PNPM in Linux build.Installing PNPM ensures lockfile consistency before dependency install.
96-101: Configure Sharp for Linux.The Sharp installation step is replicated for Linux, which is consistent and DRY.
102-104: Install Linux dependencies via PNPM.Correctly replaces
npm install. No missing env flags.
105-107: Invoke Linux runtime injector.The
installRuntime:linuxscript is called via PNPM as expected.
109-113: Use PNPM for Linux build.Switching to
pnpm run build:linux:${{ matrix.arch }}maintains consistency with Windows/Mac jobs.
140-141: Align Node.js version in Mac build.Node.js version is consistent across all platform jobs.
142-146: Add PNPM in Mac build.PNPM setup again ensures the correct package manager is used.
147-152: Configure Sharp for macOS.Maintains the same pattern of passing
TARGET_OS=darwinand architecture.
153-155: Install Mac dependencies with PNPM.Consistent with other jobs – no issues.
156-158: Invoke macOS runtime injector.The injector call is correct and platform-aware.
159-172: Use PNPM for macOS build.The build command uses
pnpm run build:mac:${{ matrix.arch }}and passes notarization and client secrets correctly.package.json (5)
8-12: Enforce PNPM and Node.js versions.Specifying
"engines"and"packageManager"locks Node (>=20.12.2) and PNPM (>=10.11.0 at 10.12.1) as required. This aligns with CI workflows.
14-14: Restrict package manager usage.The
preinstallscript usingonly-allow pnpmeffectively enforces PNPM.
25-25: Updatetypecheckto use PNPM.Chaining
pnpm run typecheck:node && pnpm run typecheck:webensures both targets are validated under PNPM context.
30-30: Switchbuildscript to PNPM.Using
pnpm run typecheck && electron-vite buildaligns with migrated workflows.
158-161: ExtendonlyBuiltDependencies.Adding
@tailwindcss/oxide,electron-winstaller, andlzoensures PNPM builds these native deps correctly.CONTRIBUTING.zh.md (4)
23-23: Switch to pnpm for internal dev prerequisite
Change correctly updates the prerequisite to usepnpm run devin line with the project’s PNPM migration.
87-89: Update install commands to use pnpm
Commands under "安装项目依赖" now correctly invokepnpm installandpnpm run installRuntime.
93-93: Use pnpm to start dev server
Startup command is correctly updated topnpm run dev.
113-115: Replace npm scripts with pnpm equivalents
Lint, i18n, and build commands have been updated topnpm run lint,pnpm run i18n, andpnpm run buildrespectively.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/install-sharp-for-platform.js (1)
31-34:linux-arm64still missing"current"inoslist
Same gap raised in the previous review – without"current"pnpm refuses to
fetch binaries on native ARM64 hosts.- 'linux-arm64': { - os: ['linux'], + 'linux-arm64': { + os: ['current', 'linux'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/install-sharp-for-platform.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
scripts/install-sharp-for-platform.js
[error] 60-60: Expected an expression but instead found '...'.
Expected an expression here.
(parse)
[error] 84-84: Illegal return statement outside of a function
(parse)
[error] 84-84: Expected a catch clause but instead found '...'.
Expected a catch clause here.
(parse)
[error] 109-109: Expected a statement but instead found '} catch (error)'.
Expected a statement here.
(parse)
[error] 56-56: Shouldn't redeclare 'readFileSync'. Consider to delete it or rename it.
'readFileSync' is defined here:
(lint/suspicious/noRedeclare)
[error] 56-57: Shouldn't redeclare 'writeFileSync'. Consider to delete it or rename it.
'writeFileSync' is defined here:
(lint/suspicious/noRedeclare)
[error] 57-57: Shouldn't redeclare 'existsSync'. Consider to delete it or rename it.
'existsSync' is defined here:
(lint/suspicious/noRedeclare)
[error] 109-113: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🪛 GitHub Actions: PR Check
scripts/install-sharp-for-platform.js
[error] 56-56: SyntaxError: Identifier 'readFileSync' has already been declared. This caused the script to fail with exit code 1 during 'pnpm run install:sharp'.
🔇 Additional comments (1)
scripts/install-sharp-for-platform.js (1)
8-10: Ensure the file is executed under ESM modeThe script uses
importsyntax with a.jsextension.
Confirm that the rootpackage.jsondeclares"type": "module"or rename the
file to.mjs; otherwise Node will refuse to load it.
#444
参考这个pr的后续，目前已经解决了pnpm的开发，打包等一系列的问题
考虑到目前项目越来越庞大，虽然已经拆解了 renderer 部分代码成为外部依赖，但依然是一个非常庞大的程序，外部开发者也很难加入进来进行协作。
准备后续逐步开始利用pnpm 的能力去把项目拆解成更加清晰的结构，方便各个领域的开发者更加容易的加入到开发中来。
目前代码和ci层面的兼容已经完成，剩余还需要把readme和文档更新后，合并该pr
Follow-up on PR #444: Currently, issues related to pnpm development, packaging, and other aspects have been resolved.
Considering the growing size of the project, although the renderer portion of the code has already been decoupled as an external dependency, it remains a very large program, making it difficult for external developers to collaborate.
Moving forward, we plan to gradually leverage pnpm’s capabilities to restructure the project into a clearer architecture, making it easier for developers from different domains to contribute.
Compatibility at the code and CI levels has already been implemented. The remaining tasks include updating the README and documentation before merging this PR.
Summary by CodeRabbit