chore(deps): update dependency @rollup #11626
Conversation
|
Preview: https://patternfly-react-pr-11626.surge.sh A11y report: https://patternfly-react-pr-11626-a11y.surge.sh Preview: https://pf-react-pr-11626.surge.sh A11y report: https://pf-react-pr-11626-a11y.surge.sh |
|
Adding the "Do not merge" label for now as we need to investigate what - if any - consumer impact there is. Depending on that this may need to be held off until a breaking change release. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Mash707
force-pushed
the
update-rollup-dependency
branch
from
c57c150 to
72095a8
Compare
WalkthroughBumped three Rollup-related devDependencies in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @package.json:
- Around line 35-37: Update the Rollup plugin configs in rollup.base.mjs: when
calling nodeResolve() pass explicit exportConditions (e.g., exportConditions:
['default','module','import']) to preserve previous v15 resolution behavior; for
commonjs(), either set strictRequires: false to retain v26 loose require
semantics or audit and refactor dynamic/top-level-this code and remove that
fallback; leave replace() unchanged but be aware objectGuards behavior changed;
after making these config changes for nodeResolve and commonjs, run the full
test suite across all ./packages to validate bundles and module resolution.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
| "@rollup/plugin-commonjs": "^28.0.2", | ||
| "@rollup/plugin-node-resolve": "^16.0.0", | ||
| "@rollup/plugin-replace": "^6.0.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@rollup/plugin-commonjs version 28 breaking changes migration guide
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🌐 Web query:
@rollup/plugin-node-resolve version 16 breaking changes migration guide
💡 Result:
Summary of breaking change (v16)
- The plugin now includes a "development|production" conditional when resolving package exports. By default it will pick "production" unless NODE_ENV is set to something other than "production" (so resolution can differ between dev and prod). This is the breaking change introduced in v16. [1][2]
What this can break
- Packages with conditional exports that provide different files for the "development" vs "production" condition may resolve to a different entry than in v15 (potentially changing runtime code or requiring different bundler handling).
- Interaction with @rollup/plugin-commonjs v16+: require-resolution semantics changed (commonjs v16+ uses require conditions), so behavior for require() vs import may differ if you don't configure conditions. [2]
Migration steps / quick fixes
-
Audit packages with conditional exports
- Search your dependencies for "exports" fields in package.json and look for branches keyed by "development" / "production".
-
Explicitly set exportConditions in node-resolve config to reproduce previous behavior
- If you want the old (pre-v16) behavior for ESM imports, set exportConditions to an explicit array:
- For Node-like resolution: exportConditions: ['node']
- To keep only ESM/import resolution: exportConditions: ['default','module','import']
- Example:
nodeResolve({ exportConditions: ['default','module','import'] })
- To include require semantics for require() calls (commonjs interop), pair with @rollup/plugin-commonjs v16+ and/or use: exportConditions: ['default','module','require'] as needed. [2]
- If you want the old (pre-v16) behavior for ESM imports, set exportConditions to an explicit array:
-
Control dev vs prod choice via NODE_ENV (if you rely on the plugin's default)
- The plugin defaults to "production" unless NODE_ENV !== 'production', so set NODE_ENV in your build scripts if you rely on the dev condition:
- e.g. cross-env NODE_ENV=development rollup -c
- Or avoid relying on NODE_ENV and set explicit exportConditions in the plugin config (recommended). [2]
- The plugin defaults to "production" unless NODE_ENV !== 'production', so set NODE_ENV in your build scripts if you rely on the dev condition:
-
Note small API/typing additions
- v16.x added options/typing fixes such as ignoreSideEffectsForRoot (see v16.0.1). If you rely on sideEffects behavior for the root package, consider the new ignoreSideEffectsForRoot option. [1]
Where to read the authoritative details
- plugin-node-resolve package page / options (describes exportConditions default and behavior). [2]
- plugin changelog / releases (v16.0.0 breaking-change entry and subsequent patch notes). [1]
If you want, I can:
- produce the exact patch you should apply to your rollup.config.js (showing the nodeResolve(...) change), or
- scan a package.json + rollup config you paste and recommend the exact exportConditions to use.
🌐 Web query:
@rollup/plugin-replace version 6 breaking changes migration guide
💡 Result:
Summary — breaking change in v6
- The v6 release notes state one breaking change: "fix!: objectGuards doesn't take effects" (v6.0.0, 2024-09-23). [1]
What that means (short)
- The objectGuards option no longer treats property reads as having side effects for the purpose of replacement guards. If your replacements relied on objectGuards to detect or preserve side-effectful property accesses (e.g., code paths that threw or invoked getters), that behavior changed: objectGuards will no longer consider those accesses as “effects.”
Quick migration steps
- Audit uses of objectGuards in your Rollup config (@rollup/plugin-replace options). If you passed guards expecting side-effect detection, update those guards to explicit checks that don’t rely on property-read side‑effects.
- Prefer explicit replacement strategies:
- Use preventAssignment: true when you want to avoid replacing left‑hand assignments. [1]
- Use typeofReplacements / values / function-based replacements for finer control (replace values with functions that inspect module id/match). [1]
- Run a full build and unit/integration tests to find any regressions where replacements were previously skipped/kept due to side-effect assumptions; convert those cases to explicit guards or manual conditional code.
- If you need previous behavior for a specific case and can’t change source, wrap or pre-process the code (or keep using an older plugin version) until you refactor.
References
- @rollup/plugin-replace ChangeLog (v6.0.0 breaking change: "objectGuards doesn't take effects"). [1]
Would you like me to: (a) scan your rollup config for uses of objectGuards and suggest exact changes, or (b) fetch the related PR/issue (#1764) and show the discussion?
🏁 Script executed:
#!/bin/bash
# Check for Rollup configuration files and the structure of the repository
echo "=== Rollup Configuration Files ==="
find . -maxdepth 3 -type f \( -name "rollup.config.*" -o -name ".rolluprc*" \) | head -20
echo ""
echo "=== Repository Structure (top level) ==="
ls -la | head -30
echo ""
echo "=== package.json location and size ==="
find . -name "package.json" -type f | head -5Repository: patternfly/patternfly-react
Length of output: 2318
🏁 Script executed:
#!/bin/bash
# Check npm registry for the specific versions to verify they exist
echo "=== Checking if versions exist on npm ==="
npm view @rollup/plugin-commonjs@28.0.2 version 2>/dev/null || echo "Version 28.0.2 not found or error"
npm view @rollup/plugin-node-resolve@16.0.0 version 2>/dev/null || echo "Version 16.0.0 not found or error"
npm view @rollup/plugin-replace@6.0.2 version 2>/dev/null || echo "Version 6.0.2 not found or error"Repository: patternfly/patternfly-react
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Read the rollup config files to check current plugin configurations
echo "=== Root level package.json (lines 30-40) ==="
sed -n '30,40p' ./package.json
echo ""
echo "=== ./packages/react-core/rollup.config.mjs ==="
cat ./packages/react-core/rollup.config.mjs
echo ""
echo "=== ./packages/react-table/rollup.config.mjs ==="
cat ./packages/react-table/rollup.config.mjs
echo ""
echo "=== ./packages/react-templates/rollup.config.mjs ==="
cat ./packages/react-templates/rollup.config.mjsRepository: patternfly/patternfly-react
Length of output: 1374
🌐 Web query:
@rollup/plugin-commonjs v28 v27 breaking changes changelog
💡 Result:
Summary of breaking changes:
- v28.0.0 — switched to fdir (fewer dependencies). [1] (gitlab.com)
- v27.0.0 — default strictRequires changed to true; replaced top-level this with exports name. [2][3] (online-gymnasium.gitlab.yandexcloud.net)
Sources: [1] v28 changelog, [2] v27/v28 release notes, [3] packaged CHANGELOG.
Citations:
- 1: https://gitlab.com/gitlab-org/gitlab-services/design.gitlab.com/-/merge_requests/4664?utm_source=openai
- 2: https://online-gymnasium.gitlab.yandexcloud.net/frontend-modules/template/-/merge_requests/53?utm_source=openai
🌐 Web query:
@rollup/plugin-commonjs v26 to v28 migration guide
💡 Result:
Short migration checklist (v26 → v28)
- Upgrade package
- npm: npm install --save-dev @rollup/plugin-commonjs@^28
- pnpm/yarn similarly. [1]
- Key breaking changes to watch
- v27: strictRequires defaults to true — if your build relied on loose/dynamic require behavior, set strictRequires: false in the plugin options (or audit dynamic require usage). [1][2]
- v27: top-level this replacement — code that relied on top-level this may behave differently because top-level this is replaced with the exports identifier; audit modules that expect global/this semantics. [1][2]
- v28: internal dependency change (switched to fdir) — fewer transitive deps; unlikely to break your config but note the change if you pin dependencies. [1]
- Notable fixes / recommendations in v28
- Docs: output.exports recommendation also applies to IIFE builds — ensure your Rollup output.export settings are appropriate for bundles that wrap CJS modules. [1]
- Several bugfixes around dynamicRequireRoot, proxy modules, node: builtins and modules with falsy __esModule — update if you saw related runtime errors previously. [1][3]
- Practical steps to migrate safely
- Pin and run tests: install v28 in a branch and run full test suite and CI.
- Search your code for dynamic require patterns and top-level this usage; update or set plugin options: e.g.
- commonjs({ strictRequires: false }) if you need old behavior.
- If you bundle for Rollup 4, follow Rollup 4 migration notes (Node >=18 and update plugins). [4]
- If you hit regressions, check the plugin changelog/issues for the specific v28.x patch that addresses it. [1][3]
References
- @rollup/plugin-commonjs changelog (v26→v28 notes, breaking changes and release notes). [1]
- v27 breaking changes (strictRequires default, top-level this change). [1][2]
- v28 bugfix patches (dynamicRequireRoot, proxy modules, node: builtins). [1][3]
- Rollup 4 migration notes (general plugin compatibility reminders). [4]
Sources
[1] @rollup/plugin-commonjs changelog / release notes.
[2] v27 release notes / changelog entries.
[3] v28 patch release notes (v28.0.2–v28.0.6).
[4] Rollup migration docs.
🏁 Script executed:
#!/bin/bash
# Find and read the base rollup config
echo "=== ./rollup.base.mjs ==="
cat ./rollup.base.mjsRepository: patternfly/patternfly-react
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Also check for any Rollup config at the root
echo "=== Files in root containing 'rollup' ==="
find . -maxdepth 1 -type f -name "*rollup*"
echo ""
echo "=== Check for other config files that might use these plugins ==="
fd -e mjs -e js -e ts "rollup" --max-depth 2 | grep -v node_modules | grep -v ".git"Repository: patternfly/patternfly-react
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Read the base rollup config file
cat ./packages/rollup.base.mjsRepository: patternfly/patternfly-react
Length of output: 1962
Configuration updates required for Rollup plugin major version upgrades.
Breaking changes have been confirmed in two of the three updated plugins that require config changes:
@rollup/plugin-node-resolve v16 (from v15.3.1):
- Breaking change: Export conditions logic now includes "development|production" conditional (defaults to "production" unless NODE_ENV set during build)
- Current config in
./packages/rollup.base.mjsline 30 usesnodeResolve()with noexportConditionsspecified - This will change package resolution behavior for dependencies with conditional exports
- Fix: Set explicit
exportConditionsin config (e.g.,nodeResolve({ exportConditions: ['default','module','import'] })) to preserve v15 behavior
@rollup/plugin-commonjs v28 (from v26.0.3):
- Breaking changes in v27:
strictRequiresdefaults to true (was false); top-levelthisreplacement behavior changed - Current config line 31 uses
commonjs()with no options - If code relies on loose/dynamic require behavior or top-level
thissemantics, builds may break - Fix: Audit for dynamic
require()patterns and top-levelthisusage; setstrictRequires: falseif needed to restore v26 behavior, or update code to match new semantics
@rollup/plugin-replace v6 (from v5.0.7):
- Breaking change:
objectGuardsno longer treats property reads as side effects - Current config uses only
replace({ 'process.env.NODE_ENV': ... })withoutobjectGuards, so this change has minimal impact
Run full test suite on each package in ./packages after applying config updates to verify no regressions in output bundles or module resolution.
🤖 Prompt for AI Agents
In @package.json around lines 35 - 37, Update the Rollup plugin configs in
rollup.base.mjs: when calling nodeResolve() pass explicit exportConditions
(e.g., exportConditions: ['default','module','import']) to preserve previous v15
resolution behavior; for commonjs(), either set strictRequires: false to retain
v26 loose require semantics or audit and refactor dynamic/top-level-this code
and remove that fallback; leave replace() unchanged but be aware objectGuards
behavior changed; after making these config changes for nodeResolve and
commonjs, run the full test suite across all ./packages to validate bundles and
module resolution.
5 participants
Towards #11491
The following dependencies are updated
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.