migrate docs package manager from npm to pnpm#1301
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes update the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 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: 0
🧹 Nitpick comments (2)
docs/package.json (2)
10-11: Prefer the built-inpackageManagerfield over thepreinstallguard
"preinstall": "npx only-allow pnpm"works, but pnpm can be enforced more robustly (and without an extra network call) by adding thepackageManagermetadata that Node ≥16.9 and Corepack understand:"private": true, "scripts": { @@ - "update-rad": "pnpm i @radui/ui@latest", - "preinstall": "npx only-allow pnpm" + "update-rad": "pnpm i @radui/ui@latest" }, + "packageManager": "pnpm@8.15.0",This:
• stops installs early when another package manager is used,
• avoids the extranpxinvocation, and
• makes the expected pnpm version explicit and reproducible.If you keep the
preinstallhook, consider switching topnpm dlx only-allow pnpmso it also works in environments wherenpm/npxis not present.
10-10: Usepnpm up --latestinstead ofpnpm ifor upgrading
pnpm i @radui/ui@latestrewritespackage.jsonand may reinstall node-modules unnecessarily.
A cleaner upgrade path is:- "update-rad": "pnpm i @radui/ui@latest", + "update-rad": "pnpm up --latest @radui/ui",
pnpm upupdates the declared version and respects workspace ranges without doing a full reinstall, which is quicker and keeps lockfiles tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/package-lock.jsonis excluded by!**/package-lock.jsondocs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
docs/package.json(2 hunks)docs/pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Learnt from: GoldGroove06
PR: rad-ui/ui#1214
File: package.json:64-64
Timestamp: 2025-07-14T18:13:36.606Z
Learning: In npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their corresponding main scripts. For example, a `prebuild` script will automatically run before the `build` script when executing `npm run build`, so there's no need to explicitly call `npm run prebuild` in the build script.
📚 Learning: in npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their co...
Learnt from: GoldGroove06
PR: rad-ui/ui#1214
File: package.json:64-64
Timestamp: 2025-07-14T18:13:36.606Z
Learning: In npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their corresponding main scripts. For example, a `prebuild` script will automatically run before the `build` script when executing `npm run build`, so there's no need to explicitly call `npm run prebuild` in the build script.
Applied to files:
docs/package.json
📚 Learning: in the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and...
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Applied to files:
docs/package.json
🔇 Additional comments (1)
docs/pnpm-workspace.yaml (1)
1-3: Confirm workspace file placement
pnpm-workspace.yamlmust live at the workspace root thatpnpmis executed from.
Placing it insidedocs/only works if all pnpm commands are run from that folder; otherwise the file will be ignored andonlyBuiltDependencieswon’t apply.Please verify CI/dev workflows and move the file to the repository root (or adjust working directories) if needed.
| @@ -0,0 +1,3 @@ | |||
| onlyBuiltDependencies: | |||
There was a problem hiding this comment.
sorry not sure what this file does, i'm also new to pnpm way of doing things - what does this file mean?
There was a problem hiding this comment.
This was auto-generated by pnpm, It ignores install/postinstall scripts by default as a security measure. I believe these packages have scripts to run during installation. Pnpm ignores these unless explicitly approving of them which i approved. That was why the file was generated.
| "lint": "next lint --fix", | ||
| "update-rad": "npm i @radui/ui@latest" | ||
| "update-rad": "pnpm i @radui/ui@latest", | ||
| "preinstall": "npx only-allow pnpm" |
There was a problem hiding this comment.
where does this script run?
There was a problem hiding this comment.
and where do you think this might help us?
There was a problem hiding this comment.
The preinstall script is to ensure that only pnpm is used to install dependencies and i had to update the "update-rad" npm prefix to pnpm to install the radui package when that "update-rad" script is run
|
Looks mostly good! Not seeing any issues - left some comments |
|
You can resolve the merge conflicts and its good to go @segundavid-dev |
segundavid-dev
force-pushed
the
migrate-pnpm
branch
2 times, most recently
from
b22bcc1 to
37ce92b
Compare
|
why close? |
I am trying to fix the merge conflict, i will reopen after fixing |
segundavid-dev
force-pushed
the
migrate-pnpm
branch
from
c8c93d3 to
47a6332
Compare
|
@kotAPI Merge conflict fixed |
|
Merging, will test this out soon and let you know how it goes! |
2 participants
Please go through this and help me confirm there's no issue on your end. I ran it locally and it worked though
Summary by CodeRabbit