Conversation
Update peer dependencies to require @rspack/core and @rspack/cli ^2.0.0-0, and pin dev dependencies to 2.0.0-beta.6 for testing. Rspack v2 ships as pure ESM, which can't be require()'d by Jest's CJS runtime. Add requireOrError mocks to the two rspack test files that previously loaded @rspack/core directly, consistent with the pattern already used in other rspack test files (plugins, optimization, rules). No source code changes needed — shakapacker's rspack integration code doesn't use any of the 30+ APIs removed or changed in v2. Closes #974 Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThis pull request adds Rspack v2 beta support while maintaining v1 compatibility. Changes include updated peer and dev dependencies to allow both major versions, expanded documentation with version compatibility details and installation instructions, and Jest mocks to handle Rspack v2's ESM-only nature. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ 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 |
Review: Upgrade rspack support from v1 to v2Overall this is a clean and well-reasoned upgrade. The "no source code changes needed" story checks out based on the referenced breaking-changes analysis, and the test mock approach is consistent with what the other rspack test files already do. A few things to address before merging: Required1. Node.js engine version must be updated The PR description explicitly states: "rspack v2 requires Node 20.19.0+" (for native 2. The peer dep is still 3. Missing CHANGELOG entry Dropping rspack v1 and requiring rspack v2 is a breaking change for existing rspack users. Per the project guidelines this warrants a Minor4. The existing rspack test files ( 5. Dev dep beta pin has no range
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Change peer deps from ^2.0.0-0 to ^1.0.0 || ^2.0.0-0 so users aren't forced to upgrade rspack just to get the latest shakapacker. Add version compatibility section to docs/rspack.md noting that rspack v2 requires Node.js 20.19.0+ (due to pure ESM). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summarize key v2 benefits: persistent cache with proper invalidation, stable incremental compilation, better tree shaking, unified target config, stricter export validation, RSC support, and performance. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Review SummaryThe approach here is solid — accepting Issues to addressMust fix:
Should fix:
Nice to have:
What looks good
|
PR Review: Add rspack v2 supportOverall this is a clean, well-scoped change. The approach of mocking Issues1. The PR updates 2. Inconsistency between the two new mocks
3. Documentation inconsistency: beta status and semver range
Minor
|
Review: Add rspack v2 supportOverall this is a clean, well-scoped PR. The approach — accepting both v1 and v2 in peer deps, pinning v2 beta in dev deps for CI, and mocking IssuesTest mock duplication (medium)
Observations
|
|
Review: Add rspack v2 support The approach is sound - accepting both v1 and v2 in peer deps with no source changes is clean, and using requireOrError mocks to handle ESM-only rspack v2 in Jest follows the existing pattern. Must-fix: None - no bugs or security issues found. Should-fix:
Informational:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const CssExtractRspackPlugin = jest.fn(function (options) { | ||
| this.options = options | ||
| }) | ||
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" |
There was a problem hiding this comment.
The CssExtractRspackPlugin constructor is created once at the module factory level (outside requireOrError), meaning the same mock instance is shared across all calls within this test file. If any test spies on or modifies this constructor, that state leaks to subsequent tests.
In contrast, rspack/index.test.js creates it fresh inside requireOrError on each call. Consider moving it inside the requireOrError callback here too, for consistency and to avoid potential test pollution.
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" | |
| return { | |
| requireOrError: (moduleName) => { | |
| if (moduleName === "@rspack/core") { | |
| const CssExtractRspackPlugin = jest.fn(function (options) { | |
| this.options = options | |
| }) | |
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" | |
| return { |
| this.options = options | ||
| }) | ||
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" | ||
|
|
There was a problem hiding this comment.
The CssExtractRspackPlugin mock here is missing the .loader static property. getStyleRule.ts:29 accesses requireOrError("@rspack/core").CssExtractRspackPlugin.loader at runtime, so if any test in this file exercises the CSS style rule path, it'll get undefined as the loader string instead of a real value.
bundlerUtils.rspack.test.js correctly sets CssExtractRspackPlugin.loader = "css-extract-rspack-loader" — this mock should do the same.
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" | |
| return { |
Greptile SummaryThis PR adds rspack v2 support to Shakapacker by widening peer dependency ranges to accept both v1 (
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["package.json peerDeps"] -->|"^1.0.0 || ^2.0.0-0"| B{"@rspack/core version?"}
B -->|v1 CJS| C["require() works natively"]
B -->|v2 ESM| D{"Runtime environment?"}
D -->|"Node 20.19.0+"| E["require() of ESM works natively"]
D -->|"Jest CJS runtime"| F["requireOrError mock needed"]
F --> G["bundlerUtils.rspack.test.js"]
F --> H["rspack/index.test.js"]
F --> I["rspack/plugins.test.js (existing)"]
F --> J["rspack/optimization.test.js (existing)"]
C --> K["No code changes needed"]
E --> K
Last reviewed commit: eb3cb27 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| jest.mock("../../../package/utils/requireOrError", () => ({ | ||
| requireOrError: (moduleName) => { | ||
| if (moduleName === "@rspack/core") { | ||
| const CssExtractRspackPlugin = jest.fn(function (options) { | ||
| this.options = options | ||
| }) | ||
| CssExtractRspackPlugin.loader = "css-extract-rspack-loader" | ||
|
|
||
| return { | ||
| DefinePlugin: jest.fn(function (definitions) { | ||
| this.definitions = definitions | ||
| }), | ||
| EnvironmentPlugin: jest.fn(function (env) { | ||
| this.env = env | ||
| }), | ||
| ProvidePlugin: jest.fn(function (definitions) { | ||
| this.definitions = definitions | ||
| }), | ||
| HotModuleReplacementPlugin: jest.fn(), | ||
| ProgressPlugin: jest.fn(), | ||
| CssExtractRspackPlugin, | ||
| SubresourceIntegrityPlugin: jest.fn(function (options) { | ||
| this.options = options | ||
| }), | ||
| SwcJsMinimizerRspackPlugin: jest.fn(), | ||
| LightningCssMinimizerRspackPlugin: jest.fn() | ||
| } | ||
| } | ||
| if (moduleName === "rspack-manifest-plugin") { | ||
| return { | ||
| RspackManifestPlugin: jest.fn(function (options) { | ||
| this.options = options | ||
| }) | ||
| } | ||
| } | ||
| return jest | ||
| .requireActual("../../../package/utils/requireOrError") | ||
| .requireOrError(moduleName) | ||
| } |
There was a problem hiding this comment.
Duplicated mock creates new objects per call
The requireOrError mock for "@rspack/core" creates new CssExtractRspackPlugin (and other constructor) instances on every invocation, since the factory is defined inline inside the if branch. Compare with bundlerUtils.rspack.test.js where CssExtractRspackPlugin is defined once outside the if block, so repeated calls to requireOrError("@rspack/core") return the same constructor reference.
This matters because the production code calls requireOrError("@rspack/core") multiple times (e.g. in bundlerUtils.ts:getBundler() and getCssExtractPlugin()). With this mock, each call gets a different CssExtractRspackPlugin constructor, so getCssExtractPlugin() and getBundler().CssExtractRspackPlugin would not be referentially equal. The tests happen to pass because none assert referential equality here, but this inconsistency between the two test files could cause confusing failures if tests are expanded later.
Consider hoisting the @rspack/core mock object outside the if branch (matching the pattern in bundlerUtils.rspack.test.js), or extracting a shared mock factory to reduce duplication across the three rspack test files that mock requireOrError.
| } | ||
| if (moduleName === "rspack-manifest-plugin") { | ||
| return { | ||
| RspackManifestPlugin: jest.fn(function (options) { |
There was a problem hiding this comment.
The @rspack/core mock is now duplicated across three test files (bundlerUtils.rspack.test.js, rspack/index.test.js, and rspack/plugins.test.js). Each one varies slightly (e.g., plugins.test.js throws on unknown modules and doesn't include DefinePlugin/ProvidePlugin; this file lacks the .loader property on CssExtractRspackPlugin).
Consider extracting a shared __mocks__/@rspack/core.js factory or a test/helpers/rspackCoreMock.js helper that all three files can import. This would make future plugin additions (or removals) a single-site change and prevent the current divergence from growing.
| @@ -51,8 +51,8 @@ | |||
| "devDependencies": { | |||
There was a problem hiding this comment.
Pinning to an exact beta version (2.0.0-beta.6) is intentional and documented, which is good. Just a reminder that when rspack v2 hits stable, these two lines need a follow-up PR to either widen to ^2.0.0 or update to the stable release — there's no automated signal for that. Might be worth opening a tracking issue now so it doesn't get lost.
Review: Add rspack v2 supportThe approach here is sound: accepting both v1 and v2 in peer deps, pinning CI devDeps to a beta, and mocking the ESM-only v2 in Jest tests is the right way to handle this. The docs section is clear and useful. A few things worth following up on: Bugs / latent issues Missing Maintainability Duplicate
Beta pin needs a follow-up issue: the docs note that Minor The test plan integration-test item (real Rails app with rspack v2) is still unchecked. Low risk given no source changes, but worth a follow-up post-merge. |
### Summary Adds the v9.7.0 changelog section with release notes for all user-visible changes since v9.6.1: - **Added**: rspack v2 support (PR #975) - **Fixed**: Config exporter path traversal and annotation format validation (PR #914) - **Fixed**: `webpack-subresource-integrity` v5 named export handling (PR #978, fixes #972) Version diff links at the bottom of the file are updated accordingly. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] Update CHANGELOG file ### Other Information Non-user-visible PRs (#920, #965, #970, #971, #977, #979, #981, #982) were intentionally excluded per changelog policy. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change updating `CHANGELOG.md`; no runtime code or dependency changes are introduced in this PR. > > **Overview** > Adds a new `v9.7.0` section to `CHANGELOG.md` documenting user-visible changes (rspack v2 support and two fixes around config export security/validation and `webpack-subresource-integrity` v5 exports). > > Updates the compare links at the bottom so `[Unreleased]` now compares from `v9.7.0`, and adds the new `[v9.7.0]` tag link. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8942a43. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added rspack v2 support * **Bug Fixes** * Improved security and validation handling <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
## Summary - Accept both rspack v1 and v2 in peer deps (`^1.0.0 || ^2.0.0-0`) so users aren't forced to upgrade rspack - CI tests against rspack v2 beta (dev deps pinned to `2.0.0-beta.6`) - Add `requireOrError` mocks to 2 rspack test files (Jest can't `require()` rspack v2's pure ESM) - Add version compatibility docs noting rspack v2 requires Node.js 20.19.0+ - **No source code changes** — shakapacker doesn't use any of the 30+ APIs removed/changed in v2 Closes #974 ## Key details **Pure ESM:** Rspack v2 ships as ESM-only. This works at runtime because Node 20.19.0+ supports `require()` of ESM modules natively (rspack v2 itself requires Node 20.19.0+). Jest's CJS runtime can't handle this, so tests mock `requireOrError` — the same pattern already used by other rspack test files. **No API changes needed:** `SubresourceIntegrityPlugin`, `CssExtractRspackPlugin`, `SwcJsMinimizerRspackPlugin`, `LightningCssMinimizerRspackPlugin`, `EnvironmentPlugin`, `builtin:swc-loader` — all work identically in v2. **Backward compatible:** Users on rspack v1 are unaffected. The peer dep range accepts both `^1.0.0` and `^2.0.0-0`. ## Test plan - [x] `yarn install` — no resolution errors - [x] `yarn build` — TypeScript compilation succeeds - [x] `yarn test` — 44 suites, 438 tests all passing - [x] `yarn lint` — clean - [ ] CI green - [ ] Integration test with a real Rails app using rspack v2 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
### Summary Adds the v9.7.0 changelog section with release notes for all user-visible changes since v9.6.1: - **Added**: rspack v2 support (PR #975) - **Fixed**: Config exporter path traversal and annotation format validation (PR #914) - **Fixed**: `webpack-subresource-integrity` v5 named export handling (PR #978, fixes #972) Version diff links at the bottom of the file are updated accordingly. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] Update CHANGELOG file ### Other Information Non-user-visible PRs (#920, #965, #970, #971, #977, #979, #981, #982) were intentionally excluded per changelog policy. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change updating `CHANGELOG.md`; no runtime code or dependency changes are introduced in this PR. > > **Overview** > Adds a new `v9.7.0` section to `CHANGELOG.md` documenting user-visible changes (rspack v2 support and two fixes around config export security/validation and `webpack-subresource-integrity` v5 exports). > > Updates the compare links at the bottom so `[Unreleased]` now compares from `v9.7.0`, and adds the new `[v9.7.0]` tag link. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8942a43. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added rspack v2 support * **Bug Fixes** * Improved security and validation handling <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
1 participant
Summary
^1.0.0 || ^2.0.0-0) so users aren't forced to upgrade rspack2.0.0-beta.6)requireOrErrormocks to 2 rspack test files (Jest can'trequire()rspack v2's pure ESM)Closes #974
Key details
Pure ESM: Rspack v2 ships as ESM-only. This works at runtime because Node 20.19.0+ supports
require()of ESM modules natively (rspack v2 itself requires Node 20.19.0+). Jest's CJS runtime can't handle this, so tests mockrequireOrError— the same pattern already used by other rspack test files.No API changes needed:
SubresourceIntegrityPlugin,CssExtractRspackPlugin,SwcJsMinimizerRspackPlugin,LightningCssMinimizerRspackPlugin,EnvironmentPlugin,builtin:swc-loader— all work identically in v2.Backward compatible: Users on rspack v1 are unaffected. The peer dep range accepts both
^1.0.0and^2.0.0-0.Test plan
yarn install— no resolution errorsyarn build— TypeScript compilation succeedsyarn test— 44 suites, 438 tests all passingyarn lint— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Chores