feat: migrate yarn v1 to pnpm v9 #557
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve a comprehensive transition from using Yarn to PNPM as the package manager across various GitHub workflows, scripts, and configuration files. Updates include installing PNPM in workflows, modifying commands to use PNPM instead of Yarn, and adjusting the Changes
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
jamiehenson
force-pushed
the
spike-change-yarn-to-pnpm
branch
7 times, most recently
from
a142901 to
e329f3d
Compare
jamiehenson
force-pushed
the
spike-change-yarn-to-pnpm
branch
from
e329f3d to
6c137f1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
scripts/update-pre-release-versions.sh (1)
Line range hint
1-28: Add pnpm installation check.Since the script now depends on pnpm, it would be helpful to add a check for pnpm installation at the beginning of the script.
Add this check after the
set -euo pipefailline:set -euo pipefail + +if ! command -v pnpm >/dev/null 2>&1; then + echo "Error: pnpm is required but not installed. Please install pnpm first." + exit 1 +fi.github/workflows/push.yml (3)
21-22: Consider pinning the pnpm version.While using
pnpm/action-setup@v4is good, specifying the exact pnpm version would ensure consistent behavior across workflow runs.- name: Install pnpm - uses: pnpm/action-setup@v4 + uses: pnpm/action-setup@v4 + with: + version: 9.x.x # Replace with your specific version
29-29: Consider splitting the installation steps.The combined command makes it harder to debug failures. Also, verify if Playwright requires additional dependencies with pnpm.
-run: pnpm i && pnpm exec playwright install +run: | + pnpm i + pnpm exec playwright install --with-deps
Line range hint
21-42: Consider adding a pnpm verification step.To ensure the workflow's reliability, consider adding a step to verify pnpm's installation and version.
- name: Install pnpm uses: pnpm/action-setup@v4 +- name: Verify pnpm installation + run: pnpm --version - name: Set up Node🧰 Tools
🪛 actionlint (1.7.4)
31-31: the runner of "wearerequired/lint-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yml (1)
44-45: LGTM! Consider reusable workflow.The pnpm installation step is consistent with the release job. Consider creating a reusable workflow for these common setup steps to reduce duplication.
# .github/workflows/setup-pnpm.yml name: Setup pnpm runs: using: composite steps: - uses: pnpm/action-setup@v4 - uses: actions/setup-node@v4 with: node-version: 20 cache: "pnpm"scripts/pre-release.sh (1)
44-45: Add pnpm version checkWhile the migration to pnpm looks good, it would be beneficial to verify the pnpm installation and version at the start of the script to prevent potential issues.
Add this check after the initial set of validations:
echo "> pnpm" +if ! command -v pnpm &> /dev/null; then + echo "Error: pnpm is not installed" + exit 1 +fi +PNPM_VERSION=$(pnpm --version) +if [[ ! "$PNPM_VERSION" =~ ^9\. ]]; then + echo "Error: pnpm version 9.x.x is required, found $PNPM_VERSION" + exit 1 +fi pnpm i --frozen-lockfilepackage.json (1)
80-81: Consider improving test script robustness.The test scripts could benefit from some improvements:
- Consider extracting the hardcoded port (6007) to a variable
- Add error handling for the concurrent execution
- Consider adding a timeout to the
wait-oncommandExample improvement:
- "test": "pnpx concurrently -k -s first -n \"SB,TEST\" -c \"magenta,blue\" \"pnpm build-storybook && pnpm http-server preview --port 6007 --silent\" \"pnpx wait-on tcp:6007 && pnpm test-storybook --url http://127.0.0.1:6007\"", + "test": "PORT=6007 pnpx concurrently -k -s first -n \"SB,TEST\" -c \"magenta,blue\" \"pnpm build-storybook && pnpm http-server preview --port $PORT --silent\" \"pnpx wait-on --timeout 30000 tcp:$PORT && pnpm test-storybook --url http://127.0.0.1:$PORT\"",
🛑 Comments failed to post (4)
.github/workflows/review.yml (2)
26-26: 🛠️ Refactor suggestion
Improve build step reliability and performance.
The current build command combines installation and build steps, which could mask installation failures. Additionally, we should leverage pnpm's store for better performance.
Split the commands and add store configuration:
- run: pnpm i && pnpm build-storybook + run: | + pnpm config set store-dir ~/.pnpm-store + pnpm install --frozen-lockfile + pnpm build-storybook📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.run: | pnpm config set store-dir ~/.pnpm-store pnpm install --frozen-lockfile pnpm build-storybook
23-24: 🛠️ Refactor suggestion
Add Node.js setup step before pnpm installation.
While the pnpm installation step is correct, we should add the Node.js setup step for better version control and caching capabilities.
Add this before the pnpm installation:
- name: Checkout uses: actions/checkout@v4 + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '18' + cache: 'pnpm' - name: Install pnpm uses: pnpm/action-setup@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- name: Checkout uses: actions/checkout@v4 - name: Setup Node.js uses: actions/setup-node@v4 with: node-version: '18' cache: 'pnpm' - name: Install pnpm uses: pnpm/action-setup@v4.github/workflows/push.yml (1)
42-42: 💡 Codebase verification
Missing required dependencies for test script execution
The test script requires
concurrentlyandwait-onpackages, but they are not found in either dependencies or devDependencies. This could cause the CI pipeline to fail.
- Add missing dependencies to devDependencies:
concurrentlywait-on🔗 Analysis chain
Verify the test script configuration.
The command looks correct, but let's verify that the test script in package.json has been properly updated for pnpm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the test script in package.json has been updated jq '.scripts.test' package.jsonLength of output: 249
Script:
#!/bin/bash # Let's verify if the dependencies for test script are properly configured jq '.dependencies["@storybook/test-runner"], .dependencies["concurrently"], .dependencies["http-server"], .dependencies["wait-on"]' package.jsonLength of output: 166
Script:
#!/bin/bash # Check devDependencies since test dependencies are often in devDependencies jq '.devDependencies["@storybook/test-runner"], .devDependencies["concurrently"], .devDependencies["http-server"], .devDependencies["wait-on"]' package.jsonLength of output: 187
package.json (1)
37-37: 💡 Codebase verification
Update React-related dependencies to maintain version consistency
The verification confirms version mismatches in React-related packages:
reactis at^18.2.0react-domis at^18.3.1@types/reactis at^18.3.12These inconsistencies between React core, React DOM, and type definitions could lead to runtime issues or incorrect type checking. The React ecosystem typically maintains version parity across these packages.
For Playwright, the version
^1.49.0matches the latest stable release, so there's no version compatibility concern there.🔗 Analysis chain
Review dependency version selections.
The new dependencies have potential version compatibility concerns:
@types/react@^18.3.12seems to be ahead of the React version (^18.2.0) which could lead to type mismatchesplaywright@^1.49.0using a caret range for a testing tool could lead to inconsistent test behaviorAlso applies to: 52-52
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version compatibility issues # Check React and @types/react version compatibility rg -A 1 '"react": ".*"' package.json rg '"@types/react": ".*"' package.json # Check latest stable Playwright version curl -s https://registry.npmjs.org/playwright/latest | jq '.version'Length of output: 241
jamiehenson
force-pushed
the
spike-change-yarn-to-pnpm
branch
6 times, most recently
from
4d78d49 to
fb2a006
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
.github/workflows/review.yml (1)
23-26: Consider pinning the pnpm version explicitlyWhile using the official pnpm action is good, consider pinning the specific pnpm version to match your project's requirements (v9.x as mentioned in PR objectives) for consistency across environments.
- name: Install pnpm uses: pnpm/action-setup@v4 + with: + version: 9.x.x
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/push.yml(2 hunks).github/workflows/release.yml(2 hunks).github/workflows/review.yml(1 hunks).gitignore(0 hunks).npmrc(1 hunks).tool-versions(1 hunks)package.json(5 hunks)scripts/pre-release.sh(1 hunks)scripts/release.sh(1 hunks)scripts/update-pre-release-versions.sh(2 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/push.yml
- .github/workflows/release.yml
- .npmrc
- .tool-versions
- scripts/pre-release.sh
- scripts/release.sh
- scripts/update-pre-release-versions.sh
🔇 Additional comments (3)
.github/workflows/review.yml (2)
27-31: LGTM! Proper Node.js setup with caching
The Node.js setup is well configured with:
- Latest LTS Node.js version
- Proper pnpm caching configuration
- Latest setup-node action version
21-35: Verify consistent package manager migration across workflows
Let's ensure all workflow files are consistently updated to use pnpm.
✅ Verification successful
Package manager migration is consistent across workflows
The verification shows:
- No remaining yarn references found in any workflow files
- All workflow files (.github/workflows/review.yml, push.yml, and release.yml) that use Node.js setup have the correct pnpm cache configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining yarn references in workflow files and verify pnpm setup in all workflows
echo "Checking for remaining yarn references in workflows..."
rg -i "yarn" .github/workflows/
echo "Verifying pnpm setup in all workflows..."
rg -l "actions/setup-node" .github/workflows/ | while read -r file; do
echo "Checking $file for pnpm cache configuration..."
rg "cache: \"pnpm\"" "$file" || echo "Missing pnpm cache in $file"
done
Length of output: 1037
package.json (1)
5-5: LGTM: Package manager specification is correctly added
The addition of the packageManager field with a pinned version is a good practice for ensuring consistent builds across the team.
|
Thanks @matt423 @aralovelace. I'll get the README side tidied up and give the installation procedure another lookover in a bit. Exciting to move the tools forward here |
jamiehenson
force-pushed
the
spike-change-yarn-to-pnpm
branch
2 times, most recently
from
47e5e37 to
f13dd0d
Compare
jamiehenson
force-pushed
the
spike-change-yarn-to-pnpm
branch
from
f13dd0d to
d0b9c9f
Compare
Jira Ticket Link / Motivation
Across our various JS-oriented web apps, we use Yarn v1. It still trucks on, but is updated infrequently and is considered to be part of the past - Yarn has since taken a different direction and is now on version 4. However, in my view the upgrade path lies not with later version of Yarn but with
pnpm- primarily due to its design feature of symlinking from a centralised repository of modules on your system instead of having local caches for each repo.Some reading on the subject: https://www.dhiwise.com/post/pnpm-vs-npm-vs-yarn-which-javascript-package-managerhttps://www.dhiwise.com/post/pnpm-vs-npm-vs-yarn-which-javascript-package-manager
This PR performs the conversion from Yarn 1 to the latest version of
pnpm, with all working. I've found it to be substantially faster, both from a cold start, and particularly when making incremental changes to packages.The only breaking change is a different four-letter word to type when you wanna do stuff.
Summary of changes
Removes yarn 1 from the
asdftools, addspnpmv9, migrates theyarn.locktopnpm's format, updates internal scripts.How do you manually test this?
Pull it down, run
asdf install(if you don't have pnpm locally already),pnpm ito install packages, try a few other things likepnpm storybookandpnpm test.Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.gitignoreand.tool-versions.