-
Notifications
You must be signed in to change notification settings - Fork 280
chore: update package manager from yarn to pnpm #2465
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
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 |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ This Project serves the backend APIs required for [Real Dev Squad](https://reald | |
|
|
||
| ## Starting Local Development | ||
|
|
||
| Please install `yarn` and `volta` | ||
| Please install `pnpm` and `volta` | ||
|
|
||
| [Why Volta?](https://docs.volta.sh/guide/#why-volta) | ||
|
|
||
|
|
@@ -39,12 +39,12 @@ To install Volta, please follow the [process](https://docs.volta.sh/guide/gettin | |
| Install all the packages using the following command: | ||
|
|
||
| ```shell | ||
| yarn | ||
| pnpm | ||
| ``` | ||
| Now if one runs yarn install. The yarn.lock will be unexpectedly updated with an unknown future version of a dependency, potentially breaking the build in the future. To ensure that the yarn.lock file is not update, you will need to use the --frozen-lockfile flag. | ||
|
|
||
| ```shell | ||
| yarn install --frozen-lockfile | ||
| pnpm install --frozen-lockfile | ||
| ``` | ||
|
|
||
| #### Confirm correct configuration setup | ||
|
|
@@ -53,7 +53,7 @@ This command should be successful, before moving to development. | |
|
|
||
|
|
||
| ```shell | ||
| yarn validate-setup | ||
| pnpm validate-setup | ||
| ``` | ||
|
|
||
| #### TDD Local Development | ||
|
|
@@ -65,39 +65,33 @@ Head over to [TDD Tests Files List](scripts/tests/tdd-files-list.txt), and add t | |
| Run TDD in watch mode. Exiting this command will print the coverage report. Try to achieve 100% coverage. | ||
|
|
||
| ```shell | ||
| yarn tdd:watch | ||
| pnpm tdd:watch | ||
| ``` | ||
|
|
||
| #### Running a server in Dev mode | ||
|
|
||
| ```shell | ||
| yarn dev | ||
| pnpm dev | ||
| ``` | ||
|
|
||
| ## What happens in production: | ||
|
|
||
| - Install packages | ||
|
|
||
| ``` | ||
| yarn | ||
| pnpm install --prod | ||
| ``` | ||
|
|
||
| - Run tests | ||
|
|
||
| ``` | ||
| yarn run test | ||
| ``` | ||
|
|
||
| - Prune dev dependencies | ||
|
|
||
| ``` | ||
| npm prune --production | ||
| pnpm run test | ||
| ``` | ||
|
|
||
| - Run start command (with port information) | ||
|
|
||
| ``` | ||
| yarn start | ||
| pnpm start | ||
| ``` | ||
|
|
||
|
Comment on lines
77
to
96
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. 💡 Verification agent 🧩 Analysis chainProduction section: heading, fenced languages, and install/test flow
-## What happens in production:
+## What happens in production
- Install packages
-
-```
-pnpm install --prod
-```
+```shell
+# Install all deps to run tests (CI step)
+pnpm install --frozen-lockfile
+```
- Run tests
-
-```
-pnpm run test
-```
+```shell
+pnpm run test
+```
- Run start command (with port information)
-
-```
-pnpm start
-```
+```shell
+pnpm start
+```
Note: These are handled automatically behind the scene when pushing to [Heroku](https://devcenter.heroku.com/)If you prefer to keep the “prod-only install” guidance, drop the test step or add a prune step after tests: 🏁 Script executed: #!/bin/bash
echo "If keeping tests, prefer:"
echo "pnpm install --frozen-lockfile && pnpm test && pnpm prune --prod"Length of output: 199 Update production instructions Please remove the trailing colon, add blank lines around your fenced blocks, specify -## What happens in production:
+## What happens in production
- Install packages
-
-```
-pnpm install --prod
-```
+```shell
+# Install all deps, run tests, then prune devDependencies for production
+pnpm install --frozen-lockfile
+pnpm run test
+pnpm prune --prod
+```
- Run start command (with port information)
-
-```
-pnpm start
-```
+```shell
+pnpm start
+```
Note: When deploying to Heroku this entire workflow is run automatically.🧰 Tools🪛 LanguageTool[grammar] ~77-~77: Use correct spacing (QB_NEW_EN_OTHER_ERROR_IDS_5) [grammar] ~79-~79: There might be a mistake here. (QB_NEW_EN_OTHER) [grammar] ~85-~85: There might be a problem here. (QB_NEW_EN_MERGED_MATCH) [grammar] ~91-~91: There might be a mistake here. (QB_NEW_EN_OTHER) 🪛 markdownlint-cli2 (0.17.2)77-77: Trailing punctuation in heading (MD026, no-trailing-punctuation) 81-81: Fenced code blocks should have a language specified (MD040, fenced-code-language) 87-87: Fenced code blocks should have a language specified (MD040, fenced-code-language) 93-93: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI Agents |
||
| Note: These are handled automatically behind the scene when pushing to [Heroku](https://devcenter.heroku.com/) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,14 @@ | |
| "scripts": { | ||
| "start": "node ./dist/server.js", | ||
| "tsc": "tsc", | ||
| "postinstall": "yarn run tsc", | ||
| "postinstall": "pnpm tsc", | ||
|
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. 💡 Verification agent 🧩 Analysis chainReconsider postinstall build: will break with production-only installs
Apply one of these diffs: Option A (preferred: explicit build) - "postinstall": "pnpm tsc",
+ "build": "pnpm tsc",Then ensure CI/deploy runs Option B (guarded postinstall; POSIX shells) - "postinstall": "pnpm tsc",
+ "postinstall": "command -v tsc >/dev/null 2>&1 && pnpm tsc || echo \"Skipping TypeScript build (tsc not available)\"",If you’re deploying to Windows environments as well, prefer Option A. To confirm whether Heroku/CI expects postinstall, and whether devDeps are available at deploy time, search your workflows/buildpack config: 🏁 Script executed: #!/bin/bash
rg -n -C2 -g ".github/workflows/**" -P '\bpostinstall\b|\bbuild\b|\bpnpm\b|\byarn\b'Length of output: 235 I’d like to inspect your 🏁 Script executed: #!/bin/bash
set -e
echo ">>> package.json ‘scripts’ section:"
if command -v jq >/dev/null 2>&1; then
jq .scripts package.json
else
grep -R '"scripts"' -n package.json -A20
fi
echo -e "\n>>> CI config files (*.yml, *.yaml):"
find . -maxdepth 3 -type f \( -name "*.yml" -o -name "*.yaml" \) -print
echo -e "\n>>> Searching for ‘heroku’ references:"
rg -n "heroku" --glob '!*node_modules*' || echo "No matches"Length of output: 1103 🏁 Script executed: #!/bin/bash
set -e
echo ">>> devDependencies in package.json:"
jq .devDependencies package.json
echo -e "\n>>> README: recommended install commands (lines around --prod):"
rg -n -C2 '\-\-prod' README.md || echo "No matches for “--prod” in README.md"
echo -e "\n>>> CI workflow install steps:"
rg -nP 'run:\s*pnpm install\b|run:\s*npm install\b' -C2 .github/workflowsLength of output: 1387 Move TypeScript compile out of postinstall The • package.json, scripts section (around line 8): "scripts": {
- "postinstall": "pnpm tsc",
+ "build": "pnpm tsc",
"start": "node ./dist/server.js",
…
}• Update your CI/deployment workflows to run: pnpm install
pnpm build
pnpm startOptionally, if you really must keep compilation on install, guard against missing - "postinstall": "pnpm tsc",
+ "postinstall": "command -v tsc >/dev/null 2>&1 && pnpm tsc || echo \"Skipping TypeScript build (tsc not available)\"",🤖 Prompt for AI Agents |
||
| "dev": "cross-env NODE_ENV=development ts-node-dev server.js", | ||
| "lint": "eslint .", | ||
| "lint-fix": "eslint . --fix", | ||
| "validate-setup": "node scripts/validateSetup.js", | ||
| "test-integration": "sh scripts/tests/testIntegration.sh", | ||
| "test-unit": "sh scripts/tests/testUnit.sh", | ||
| "test": "yarn lint && yarn test-unit && yarn test-integration", | ||
| "test": "pnpm lint && pnpm test-unit && pnpm test-integration", | ||
| "tdd:watch": "sh scripts/tests/tdd.sh" | ||
| }, | ||
| "dependencies": { | ||
|
|
@@ -33,6 +33,7 @@ | |
| "joi": "17.12.2", | ||
| "jsdoc": "4.0.2", | ||
| "jsonwebtoken": "^8.5.1", | ||
| "lodash": "^4.17.21", | ||
|
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. 💡 Verification agent 🧩 Analysis chainQuestion: Why introduce lodash in a package-manager-only PR? Adding a new runtime dependency changes attack surface and deployment footprint and is out of scope for a tooling migration. If not strictly required, please drop it; otherwise, justify usage and add tests. Run this to verify actual usage before keeping it: If only a couple of utilities are needed, prefer ES built-ins or targeted imports like 🏁 Script executed: #!/bin/bash
# Look for explicit lodash imports/requires
rg -n -C2 -t js -t ts -P "(from\\s+['\"]lodash['\"]|require\\(['\"]lodash['\"]\\))"Length of output: 1577 🏁 Script executed: #!/bin/bash
# Display the contents of utils/logs.js to confirm lodash usage
echo "=== utils/logs.js (first 200 lines) ==="
sed -n '1,200p' utils/logs.js
echo
echo "=== Instances of '_' usage in utils/logs.js ==="
rg -n -C1 '\b_\.' utils/logs.jsLength of output: 5223 🏁 Script executed: #!/bin/bash
# Inspect lodash usage in test files to see which utilities are used
for f in test/unit/middlewares/oooRequests.test.ts test/integration/impersonationRequests.test.ts test/integration/requests.test.ts; do
echo "=== $f (first 200 lines) ==="
sed -n '1,200p' "$f"
echo
echo "=== lodash method calls in $f ==="
rg -n '\b_\.[a-zA-Z]\w*\b' -C1 "$f" || echo "No lodash calls found"
echo
doneLength of output: 20225 Consider removing lodash or using targeted imports I see that lodash was added as a runtime dependency but is only used for
If lodash isn’t strictly required, please remove it and replace all uses of // Before
..._.omit(formattedData, "users")
// After
const { users, ...others } = formattedData;
...othersAnd in tests: // Before
.body(_.omit(obj, "comment"))
// After
const { comment, ...body } = obj;
.body(body)Otherwise, minimize our footprint by installing only the needed function: npm install lodash.omitimport omit from "lodash.omit";This avoids expanding our runtime attack surface and bundle size. 🤖 Prompt for AI Agents |
||
| "morgan": "1.10.0", | ||
| "multer": "1.4.5-lts.1", | ||
| "newrelic": "11.19.0", | ||
|
|
@@ -85,6 +86,7 @@ | |
| ], | ||
| "volta": { | ||
| "node": "22.10.0", | ||
| "yarn": "1.22.21" | ||
| } | ||
| "pnpm": "10.14.0" | ||
| }, | ||
| "packageManager": "pnpm@10.14.0" | ||
|
Comment on lines
+89
to
+91
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. 💡 Verification agent 🧩 Analysis chainFix CI: enable Corepack and migrate workflows from Yarn to pnpm The pipeline failed with “Run yarn” and a Corepack error. With
Use this script to locate remaining Yarn usage and lockfiles: Suggested CI snippet (for reference): - uses: actions/setup-node@v4
with:
node-version-file: package.json
cache: 'pnpm'
cache-dependency-path: pnpm-lock.yaml
- run: corepack enable
- run: corepack prepare pnpm@10.14.0 --activate
- run: pnpm install --frozen-lockfile
- run: pnpm testI can open a follow-up PR updating your workflows to pnpm with Corepack if you’d like. 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Workflows using yarn:"
rg -n -C2 -g ".github/workflows/**" -P '\byarn\b'
echo -e "\nLockfiles present:"
fd -H -a 'yarn.lock' || true
fd -H -a 'pnpm-lock.yaml' || trueLength of output: 273 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Searching for 'yarn' in GitHub Actions workflows..."
rg -n -C2 '\byarn\b' .github/workflows || echo "No 'yarn' usage found in .github/workflows"
echo -e "\nListing lockfiles in the repo:"
find . -type f \( -name 'yarn.lock' -o -name 'pnpm-lock.yaml' \) || trueLength of output: 564 Fix CI workflows: replace Yarn invocations and enable Corepack for pnpm The repo still has a Yarn command in your GitHub Actions (no • .github/workflows/test.yml (lines 26–27)
Please update your workflow to use pnpm and Corepack, for example: - uses: actions/setup-node@v4
with:
node-version-file: package.json
cache: 'pnpm'
cache-dependency-path: pnpm-lock.yaml
- run: corepack enable
- run: corepack prepare pnpm@10.14.0 --activate
- run: pnpm install --frozen-lockfile
- run: pnpm testLet me know if you’d like me to open a follow-up PR converting all workflows. 🤖 Prompt for AI Agents |
||
| } | ||
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.
🛠️ Refactor suggestion
Fix install command and remove Yarn-specific paragraph
The code block
pnpmis not a valid install command. The paragraph below still references Yarn and yarn.lock, which is inconsistent with the pnpm migration. Also add the fenced code language and surround fences with blank lines.🧰 Tools
🪛 LanguageTool
[grammar] ~44-~44: Make sure you are using the right part of speech
Context: ...o ensure that the yarn.lock file is not update, you will need to use the --frozen-lock...
(QB_NEW_EN_OTHER_ERROR_IDS_21)
[grammar] ~44-~44: Use correct spacing
Context: ... need to use the --frozen-lockfile flag.
shell pnpm install --frozen-lockfile#### Confirm correct configuration setup Thi...(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 markdownlint-cli2 (0.17.2)
43-43: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents