-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
chore: migrate from npm to pnpm package manager #2000
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,74 +1,73 @@ | ||||||||||
| name: 'Setup Node.js Frontend' | ||||||||||
| description: 'Set up Node.js with npm and cached dependencies for the frontend' | ||||||||||
| description: 'Set up Node.js with pnpm and cached dependencies for the frontend' | ||||||||||
|
|
||||||||||
| inputs: | ||||||||||
| node-version: | ||||||||||
| description: 'Node.js version to use' | ||||||||||
| required: false | ||||||||||
| default: '24' | ||||||||||
| ignore-scripts: | ||||||||||
| description: 'Whether to use --ignore-scripts flag during npm ci' | ||||||||||
| description: 'Whether to use --ignore-scripts flag during pnpm install' | ||||||||||
| required: false | ||||||||||
| default: 'false' | ||||||||||
|
|
||||||||||
| outputs: | ||||||||||
| cache-hit: | ||||||||||
| description: 'Whether npm cache was hit' | ||||||||||
| description: 'Whether pnpm cache was hit' | ||||||||||
| value: ${{ steps.cache.outputs.cache-hit }} | ||||||||||
|
|
||||||||||
| runs: | ||||||||||
| using: 'composite' | ||||||||||
| steps: | ||||||||||
| - name: Setup Node.js ${{ inputs.node-version }} | ||||||||||
| uses: actions/setup-node@v4 | ||||||||||
| uses: actions/setup-node@v6 | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
| with: | ||||||||||
| node-version: ${{ inputs.node-version }} | ||||||||||
|
|
||||||||||
| - name: Get npm cache directory | ||||||||||
| id: npm-cache-dir | ||||||||||
| - name: Install pnpm | ||||||||||
| shell: bash | ||||||||||
| run: echo "dir=$(npm config get cache)" >> "$GITHUB_OUTPUT" | ||||||||||
| run: | | ||||||||||
| npm cache clean --force | ||||||||||
| npm install -g pnpm | ||||||||||
|
Comment on lines
+30
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+29
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Unnecessary The Proposed fix - name: Install pnpm
shell: bash
- run: |
- npm cache clean --force
- npm install -g pnpm
+ run: npm install -g pnpmAlternatively, consider using pnpm/action-setup which is the official GitHub Action for pnpm and handles caching more efficiently. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| - name: Get pnpm store directory | ||||||||||
| id: pnpm-cache-dir | ||||||||||
| shell: bash | ||||||||||
| run: echo "dir=$(pnpm store path)" >> "$GITHUB_OUTPUT" | ||||||||||
|
|
||||||||||
| - name: Cache npm dependencies | ||||||||||
| - name: Cache pnpm dependencies | ||||||||||
| id: cache | ||||||||||
| uses: actions/cache@v4 | ||||||||||
| uses: actions/cache@v5 | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
| with: | ||||||||||
| path: ${{ steps.npm-cache-dir.outputs.dir }} | ||||||||||
| key: ${{ runner.os }}-npm-${{ hashFiles('package-lock.json') }} | ||||||||||
| restore-keys: ${{ runner.os }}-npm- | ||||||||||
| path: ${{ steps.pnpm-cache-dir.outputs.dir }} | ||||||||||
| key: ${{ runner.os }}-pnpm-migrate-${{ hashFiles('pnpm-lock.yaml') }}-${{ hashFiles('apps/frontend/package.json') }} | ||||||||||
| restore-keys: ${{ runner.os }}-pnpm-migrate-${{ hashFiles('pnpm-lock.yaml') }}- | ||||||||||
|
Comment on lines
+43
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Transitional cache key prefix should be removed post-migration. The cache key includes a Post-migration cleanup- key: ${{ runner.os }}-pnpm-migrate-${{ hashFiles('pnpm-lock.yaml') }}-${{ hashFiles('apps/frontend/package.json') }}
- restore-keys: ${{ runner.os }}-pnpm-migrate-${{ hashFiles('pnpm-lock.yaml') }}-
+ key: ${{ runner.os }}-pnpm-${{ hashFiles('pnpm-lock.yaml') }}-${{ hashFiles('apps/frontend/package.json') }}
+ restore-keys: ${{ runner.os }}-pnpm-${{ hashFiles('pnpm-lock.yaml') }}-📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| - name: Install dependencies | ||||||||||
| shell: bash | ||||||||||
| # Run npm ci from root to properly handle workspace dependencies. | ||||||||||
| # With npm workspaces, the lock file is at root and dependencies are hoisted there. | ||||||||||
| # Running npm ci in apps/frontend would fail to populate node_modules correctly. | ||||||||||
| run: | | ||||||||||
| # Remove any existing node_modules to avoid conflicts | ||||||||||
| if [ "${{ inputs.ignore-scripts }}" == "true" ]; then | ||||||||||
| npm ci --ignore-scripts | ||||||||||
| pnpm install --frozen-lockfile --ignore-scripts | ||||||||||
| else | ||||||||||
| npm ci | ||||||||||
| pnpm install --frozen-lockfile | ||||||||||
| fi | ||||||||||
|
|
||||||||||
| - name: Link node_modules for electron-builder | ||||||||||
| shell: bash | ||||||||||
| # electron-builder expects node_modules in apps/frontend for native module rebuilding. | ||||||||||
| # With npm workspaces, packages are hoisted to root. Create a link so electron-builder | ||||||||||
| # can find the modules during packaging and code signing. | ||||||||||
| # With pnpm workspaces, packages are in the pnpm store and linked via node_modules/.pnpm. | ||||||||||
| # Create a link so electron-builder can find the modules during packaging and code signing. | ||||||||||
| # Uses symlink on Unix, directory junction on Windows (works without admin privileges). | ||||||||||
| # | ||||||||||
| # IMPORTANT: npm workspaces may create a partial node_modules in apps/frontend for | ||||||||||
| # packages that couldn't be hoisted. We must remove it and create a proper link to root. | ||||||||||
| run: | | ||||||||||
| # Verify npm ci succeeded | ||||||||||
| # Verify pnpm install succeeded | ||||||||||
| if [ ! -d "node_modules" ]; then | ||||||||||
| echo "::error::Root node_modules does not exist. npm ci may have failed." | ||||||||||
| echo "::error::Root node_modules does not exist. pnpm install may have failed." | ||||||||||
| exit 1 | ||||||||||
| fi | ||||||||||
|
|
||||||||||
| # Remove any existing node_modules in apps/frontend | ||||||||||
| # This handles: partial directories from npm workspaces, AND broken symlinks | ||||||||||
| if [ -e "apps/frontend/node_modules" ] || [ -L "apps/frontend/node_modules" ]; then | ||||||||||
| # Check if it's a valid symlink pointing to root node_modules | ||||||||||
| if [ -L "apps/frontend/node_modules" ]; then | ||||||||||
| target=$(readlink apps/frontend/node_modules 2>/dev/null || echo "") | ||||||||||
| if [ "$target" = "../../node_modules" ] && [ -d "apps/frontend/node_modules" ]; then | ||||||||||
|
|
@@ -78,16 +77,14 @@ runs: | |||||||||
| rm -f "apps/frontend/node_modules" | ||||||||||
| fi | ||||||||||
| else | ||||||||||
| echo "Removing partial node_modules directory created by npm workspaces..." | ||||||||||
| echo "Removing partial node_modules directory created by pnpm workspaces..." | ||||||||||
| rm -rf "apps/frontend/node_modules" | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The GitHub action incorrectly symlinks Suggested FixRemove the steps in the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||||||||||
| fi | ||||||||||
| fi | ||||||||||
|
|
||||||||||
| # Create link if it doesn't exist or was removed | ||||||||||
| if [ ! -L "apps/frontend/node_modules" ]; then | ||||||||||
| if [ "$RUNNER_OS" == "Windows" ]; then | ||||||||||
| # Use directory junction on Windows (works without admin privileges) | ||||||||||
| # Use PowerShell's New-Item -ItemType Junction for reliable path handling | ||||||||||
| abs_target=$(cygpath -w "$(pwd)/node_modules") | ||||||||||
| link_path=$(cygpath -w "$(pwd)/apps/frontend/node_modules") | ||||||||||
| powershell -Command "New-Item -ItemType Junction -Path '$link_path' -Target '$abs_target'" > /dev/null | ||||||||||
|
|
@@ -98,7 +95,6 @@ runs: | |||||||||
| exit 1 | ||||||||||
| fi | ||||||||||
| else | ||||||||||
| # Use symlink on Unix (macOS/Linux) | ||||||||||
| if ln -s ../../node_modules apps/frontend/node_modules; then | ||||||||||
| echo "Created symlink: apps/frontend/node_modules -> ../../node_modules" | ||||||||||
| else | ||||||||||
|
|
@@ -108,14 +104,11 @@ runs: | |||||||||
| fi | ||||||||||
| fi | ||||||||||
|
|
||||||||||
| # Final verification - the link must exist and resolve correctly | ||||||||||
| # Note: On Windows, junctions don't show as symlinks (-L), so we check if the directory exists | ||||||||||
| # and can be listed. On Unix, we also verify it's a symlink. | ||||||||||
| # Final verification | ||||||||||
| if [ "$RUNNER_OS" != "Windows" ] && [ ! -L "apps/frontend/node_modules" ]; then | ||||||||||
| echo "::error::apps/frontend/node_modules symlink was not created" | ||||||||||
| exit 1 | ||||||||||
| fi | ||||||||||
| # Verify the link resolves to a valid directory with content | ||||||||||
| if ! ls apps/frontend/node_modules/electron >/dev/null 2>&1; then | ||||||||||
| echo "::error::apps/frontend/node_modules does not resolve correctly (electron not found)" | ||||||||||
| ls -la apps/frontend/ || true | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,7 +107,7 @@ | |
| "zustand": "^5.0.9" | ||
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "2.3.11", | ||
| "@biomejs/biome": "2.4.10", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "@electron-toolkit/preload": "^3.0.2", | ||
| "@electron-toolkit/utils": "^4.0.0", | ||
| "@electron/rebuild": "^4.0.2", | ||
|
|
@@ -122,19 +122,19 @@ | |
| "@types/react-dom": "^19.2.3", | ||
| "@types/semver": "^7.7.1", | ||
| "@types/uuid": "^11.0.0", | ||
| "@vitejs/plugin-react": "^5.1.2", | ||
| "@vitejs/plugin-react": "^6.0.1", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "autoprefixer": "^10.4.22", | ||
| "cross-env": "^10.1.0", | ||
| "electron": "40.0.0", | ||
| "electron-builder": "^26.4.0", | ||
| "electron-vite": "^5.0.0", | ||
| "husky": "^9.1.7", | ||
| "jsdom": "^27.3.0", | ||
| "jsdom": "^29.0.2", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "lint-staged": "^16.2.7", | ||
| "postcss": "^8.5.6", | ||
| "tailwindcss": "^4.1.17", | ||
| "typescript": "^5.9.3", | ||
| "vite": "^7.2.7", | ||
| "vite": "^8.0.5", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "vitest": "^4.0.16" | ||
| }, | ||
| "overrides": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ | |
| "electron" | ||
| ], | ||
| "devDependencies": { | ||
| "jsdom": "^27.4.0" | ||
| "jsdom": "^29.0.2" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }, | ||
| "dependencies": { | ||
| "lucide-react": "^0.562.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.
🧩 Analysis chain
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 2202
Update workflows to use
pnpm runinstead ofnpm runfor consistency.The setup-node-frontend action configures pnpm, but multiple workflows still invoke
npm runcommands. This creates an inconsistency where the workflows bypass the pnpm setup:.github/workflows/ci.yml(lines 133, 137, 141):npm run typecheck,npm run test,npm run build.github/workflows/release.yml: 8 instances ofnpm runcommands.github/workflows/beta-release.yml: 7 instances ofnpm runcommands.github/workflows/build-prebuilds.yml: 1 instance ofnpm installUpdate these to use
pnpm runandpnpm installrespectively to align with the pnpm migration.🤖 Prompt for AI Agents