Dependabot/npm and yarn/deps - #162
Conversation
WalkthroughDevDependencies and build scripts updated across multiple packages. Each package.json replaces the rollup invocation path with a direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ice/package.json (1)
23-23:⚠️ Potential issue | 🔴 CriticalFix Rollup CLI invocation path—current hardcoded path is incompatible with Rollup v4.
Rollup v4 moved the CLI binary from
bin/rolluptodist/bin/rollup. The hardcoded path../../node_modules/rollup/bin/rollupon line 23 will fail. Userollup -c rollup.config.jsinstead so npm scripts automatically resolve the correct binary location vianode_modules/.bin.This issue affects all 7 packages in the workspace (ice, utils, search, models, redux, content, classes), not just ice.
Suggested manifest change
- "rollup": "../../node_modules/rollup/bin/rollup -c rollup.config.js", + "rollup": "rollup -c rollup.config.js",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ice/package.json` at line 23, The "rollup" npm script currently hardcodes the CLI path ("rollup" script key) to ../../node_modules/rollup/bin/rollup which breaks with Rollup v4; change the script value to use the resolved binary name ("rollup -c rollup.config.js") so npm will use node_modules/.bin; apply this same change to the "rollup" script in each affected package manifest (ice, utils, search, models, redux, content, classes) to remove the hardcoded path and ensure compatibility with Rollup v4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ice/package.json`:
- Line 23: The "rollup" npm script currently hardcodes the CLI path ("rollup"
script key) to ../../node_modules/rollup/bin/rollup which breaks with Rollup v4;
change the script value to use the resolved binary name ("rollup -c
rollup.config.js") so npm will use node_modules/.bin; apply this same change to
the "rollup" script in each affected package manifest (ice, utils, search,
models, redux, content, classes) to remove the hardcoded path and ensure
compatibility with Rollup v4.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2ef2075-6302-474d-80c8-d35a9be65bc2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/classes/package.jsonpackages/content/package.jsonpackages/ice/package.jsonpackages/models/package.jsonpackages/redux/package.jsonpackages/search/package.jsonpackages/utils/package.json
|
Outside diff range comments addressed (rollup path was still working, but it is now simplified) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/redux/package.json (1)
41-41: Consider:node-fetchversion specifier is pinned without^prefix.The version
"3.3.2"is pinned exactly, while other dependencies use^for semver ranges. This prevents automatic patch/minor updates.If intentional (to avoid potential breaking changes from the v2→v3 ESM migration), this is fine. Otherwise, consider
"^3.3.2"for consistency.Optional: Add caret for consistency
- "node-fetch": "3.3.2", + "node-fetch": "^3.3.2",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/redux/package.json` at line 41, The package.json currently pins "node-fetch" to an exact version ("3.3.2") which prevents automatic minor/patch updates; update the dependency specifier for node-fetch in package.json to a caret range (e.g., "^3.3.2") to match other dependencies and allow semver-compatible updates, unless the exact pin was intentional (in which case add a comment or update the dependency list to indicate the reason).packages/classes/package.json (1)
38-38: Same note:node-fetchis pinned without^prefix.Consistent with
packages/redux/package.json, but inconsistent with other dependencies in this file. Consider adding^if you want automatic patch updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/classes/package.json` at line 38, The dependency "node-fetch" is pinned to "3.3.2" without a caret; update the package.json dependency entry for node-fetch to use a careted semver (e.g. "^3.3.2") so it can receive patch/minor updates and match the style used in packages/redux/package.json; modify the "node-fetch" entry in the dependencies block accordingly and run install/lockfile update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/search/package.json`:
- Line 35: Add an explicit Node engine constraint in the root package.json by
adding an "engines" field with "node": ">=18.0.0" to prevent users on Node 14–17
from installing dependencies that require Rollup v4; edit the package.json and
add the "engines": { "node": ">=18.0.0" } entry at root level (ensure it is
valid JSON and commas adjusted around existing keys).
---
Nitpick comments:
In `@packages/classes/package.json`:
- Line 38: The dependency "node-fetch" is pinned to "3.3.2" without a caret;
update the package.json dependency entry for node-fetch to use a careted semver
(e.g. "^3.3.2") so it can receive patch/minor updates and match the style used
in packages/redux/package.json; modify the "node-fetch" entry in the
dependencies block accordingly and run install/lockfile update.
In `@packages/redux/package.json`:
- Line 41: The package.json currently pins "node-fetch" to an exact version
("3.3.2") which prevents automatic minor/patch updates; update the dependency
specifier for node-fetch in package.json to a caret range (e.g., "^3.3.2") to
match other dependencies and allow semver-compatible updates, unless the exact
pin was intentional (in which case add a comment or update the dependency list
to indicate the reason).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b73701e-c746-41a7-9225-68d4a4362546
📒 Files selected for processing (7)
packages/classes/package.jsonpackages/content/package.jsonpackages/ice/package.jsonpackages/models/package.jsonpackages/redux/package.jsonpackages/search/package.jsonpackages/utils/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ice/package.json
- packages/content/package.json
| }, | ||
| "devDependencies": { | ||
| "@rollup/plugin-commonjs": "^25.0.7", | ||
| "@rollup/plugin-commonjs": "^29.0.2", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repo Node engine constraints (all package.json files) =="
fd '^package\.json$' -E node_modules -x sh -c '
file="$1"
name=$(jq -r ".name // \"<unnamed>\"" "$file")
engine=$(jq -r ".engines.node // \"<none>\"" "$file")
printf "%s | %s | engines.node=%s\n" "$file" "$name" "$engine"
' sh {}
echo
echo "== NPM metadata checks for bumped build deps =="
echo "-- rollup@4.59.0"
curl -s https://registry.npmjs.org/rollup/4.59.0 \
| jq -r '"engines.node=" + (.engines.node // "<none>")'
echo "-- `@rollup/plugin-commonjs`@29.0.2"
curl -s https://registry.npmjs.org/%40rollup%2fplugin-commonjs/29.0.2 \
| jq -r '"peerDependencies.rollup=" + (.peerDependencies.rollup // "<none>"), "engines.node=" + (.engines.node // "<none>")'
echo "-- rollup-plugin-sourcemaps@0.6.3"
curl -s https://registry.npmjs.org/rollup-plugin-sourcemaps/0.6.3 \
| jq -r '"peerDependencies.rollup=" + (.peerDependencies.rollup // "<none>"), "engines.node=" + (.engines.node // "<none>")'
echo "-- terser@5.46.0"
curl -s https://registry.npmjs.org/terser/5.46.0 \
| jq -r '"engines.node=" + (.engines.node // "<none>")'
echo
echo "== Scan rollup config for potentially legacy keys to manually re-check =="
fd 'rollup\.config\.(js|cjs|mjs|ts)$' -t f -E node_modules -x rg -n -C2 \
'acorn|output\.preferConst|treeshake\.pureExternalModules|legacy|commonjs\(' {} || trueRepository: craftercms/js-sdk
Length of output: 1161
🏁 Script executed:
cat -n packages/search/package.json | sed -n '30,45p'Repository: craftercms/js-sdk
Length of output: 623
🏁 Script executed:
fd 'rollup' -t f -E node_modules | head -20Repository: craftercms/js-sdk
Length of output: 371
🏁 Script executed:
cat -n packages/search/rollup.config.js | head -50Repository: craftercms/js-sdk
Length of output: 984
🏁 Script executed:
cat -n package.json | sed -n '1,30p'Repository: craftercms/js-sdk
Length of output: 1097
🏁 Script executed:
cat -n util/rollup-utils.jsRepository: craftercms/js-sdk
Length of output: 4400
Add explicit Node engine constraint to prevent Rollup v4 compatibility issues.
Rollup v4.59.0 requires Node >=18.0.0, but the repository has no engines.node constraint defined. This will allow developers on Node 14-17 to install dependencies and encounter runtime failures. The bumped plugins (@rollup/plugin-commonjs@29.0.2, rollup-plugin-sourcemaps@0.6.3) are all compatible with Rollup v4, and the rollup config (util/rollup-utils.js) uses no deprecated APIs, so the toolchain is otherwise safe to upgrade. Add "engines": { "node": ">=18.0.0" } to the root package.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/search/package.json` at line 35, Add an explicit Node engine
constraint in the root package.json by adding an "engines" field with "node":
">=18.0.0" to prevent users on Node 14–17 from installing dependencies that
require Rollup v4; edit the package.json and add the "engines": { "node":
">=18.0.0" } entry at root level (ensure it is valid JSON and commas adjusted
around existing keys).
1 participant
Bump rollup - #150
Bump mocha and @types/mocha - #143
Bump terser - #140
Bump @rollup/plugin-commonjs - #139
Bump rollup - #134
Bump node-fetch - #119
Summary by CodeRabbit