Conversation
WalkthroughA new "update" script was added to the npm scripts in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
51-54: Consider aligning “update” vs. “update:*” scriptsYou now have three flavours of the same command:
"update": "npx npm-check-updates -du", "update:major": "npx npm-check-updates -i", "update:minor": "npx npm-check-updates -i -t minor",
updatealready updates all prod + dev deps, so the dedicatedupdate:minor/update:majorvariants may lead to confusion for new contributors.Two common clean-ups:
- Keep only
updateand document the flags (-t minor|major) in README.- Rename
update→update:patch(or similar) and make the triad explicit.No functional defect, just a naming consistency / DX point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (1)
package.json (1)
65-88: Major-version bumps — please run full CI to confirm no breaking changes slipped inSeveral upgrades cross a major boundary (eslint 8→9, mocha 10→11, sinon 17→21, release-it 17→19, etc.). While tests may still pass locally, these tools often change CLIs or default behaviours that surface only in edge cases (e.g., eslint rule renames, Mocha’s watcher flags, Sinon’s fake timers).
Action items:
- Verify
npm run lint,npm test,npm run prerelease,npm run watchon fresh clone.- Check generated artifacts (
dist, typings) still compile undertypescript@5.8.- If you publish an alpha from this branch, install it into a sample project to ensure no peer-dependency warnings.
No immediate code change required, but please confirm before merging.
| "@node-saml/node-saml": "^5.1.0", | ||
| "@types/express": "^4.17.23", | ||
| "@types/passport": "^1.0.17", | ||
| "@types/passport-strategy": "^0.2.38", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move @types/* packages to devDependencies to avoid bloating the runtime bundle
@types/express and @types/passport are compile-time–only artifacts and never required at runtime. Shipping them in dependencies:
- inflates the published tarball / install size,
- pulls in transitive type-only deps for downstream consumers,
- can break consumers that install with
--production.
Proposed minimal fix:
@@ "dependencies": {
- "@node-saml/node-saml": "^5.1.0",
- "@types/express": "^4.17.23",
- "@types/passport": "^1.0.17",
- "@types/passport-strategy": "^0.2.38",
+ "@node-saml/node-saml": "^5.1.0",
+ "@types/passport-strategy": "^0.2.38" // keep ONLY if used at runtime
@@
},
"devDependencies": {
+ "@types/express": "^4.17.23",
+ "@types/passport": "^1.0.17",(If @types/passport-strategy is also type-only, move it likewise.)
📝 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.
| "@node-saml/node-saml": "^5.1.0", | |
| "@types/express": "^4.17.23", | |
| "@types/passport": "^1.0.17", | |
| "@types/passport-strategy": "^0.2.38", | |
| "dependencies": { | |
| "@node-saml/node-saml": "^5.1.0", | |
| "@types/passport-strategy": "^0.2.38" | |
| }, | |
| "devDependencies": { | |
| "@types/express": "^4.17.23", | |
| "@types/passport": "^1.0.17" | |
| // …other existing devDependencies… | |
| } |
🤖 Prompt for AI Agents
In package.json around lines 57 to 60, the @types/express, @types/passport, and
@types/passport-strategy packages are incorrectly listed under dependencies but
should be moved to devDependencies because they are only needed at compile time.
To fix this, remove these @types packages from the dependencies section and add
them to the devDependencies section to prevent bloating the runtime bundle and
avoid issues for downstream consumers.
There was a problem hiding this comment.
For anyone that may look here, this is bad advice. Since we export some types, we need to include them in dependencies so that users of this package will have the dependencies when they are using this library. The only thing we might consider is using peerDependencies, but support isn't universal for that yet.
There was a problem hiding this comment.
@cjbarth Thank you for the important clarification! You're absolutely right, and I appreciate you correcting that suggestion.
When a TypeScript library exports types that reference other @types/* packages, those type definitions become necessary for consumers during their own TypeScript compilation process. Moving them to devDependencies would indeed break the library for downstream users who rely on the exported types.
This is a great example of how the typical rule of "put @types in devDependencies" has important exceptions when building reusable TypeScript libraries that export type definitions.
✏️ Learnings added
Learnt from: cjbarth
PR: node-saml/passport-saml#961
File: package.json:57-60
Timestamp: 2025-07-24T18:05:18.700Z
Learning: In TypeScript libraries that export types referencing @types/* packages (like @types/express, @types/passport), these @types packages must remain in dependencies rather than devDependencies. This is because consumers of the library need these type definitions available during their own TypeScript compilation when using the exported types. Moving them to devDependencies would break the library for downstream users.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This updates minor dependencies only. This is mostly to pull in security fixes made in the
node-samlbase library.Summary by CodeRabbit