Defer webpack/rspack package-index side effects - #1107
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Code ReviewOverall: The approach is correct and well-scoped. Deferring the A few things worth addressing before merging: Bug risk –
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Follow-up ReviewThis is a follow-up to my earlier review. The three issues I raised have been addressed in the latest commit:
Remaining nit
Overall assessmentThe implementation is correct:
Ready to merge once the draft status is removed. |
Code ReviewOverall: A well-targeted fix for issue #1095. The lazy-getter approach is correct and the regression tests are a good addition. Two gaps worth addressing before merging. Critical:
|
Code ReviewOverviewThis PR fixes #1095 by removing the eager Must-Fix1. The runtime property now has a getter but no setter. TypeScript callers who write // package/index.d.ts, line 29
readonly baseConfig: Configuration2. Missing setter / breaking-change documentation See the inline thread on Should-Fix3. Inconsistency between first and third test
Nits / Suggestions
What's Working Well
|
Review response (commit 54bc327)Addressed the follow-up review (issue comments #4412036948 and #4412144693):
The three follow-up items confirmed addressed in #4411942887 ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Code Review — PR #1107: Avoid package index plugin side effectsOverall: The goal is solid and the fix is well-motivated. Preventing eager side-effectful imports when merely Bug: Dead code in
|
54bc327 to
74d9cb1
Compare
Review of PR #1107 — Avoid package index plugin side effectsOverall: The goal is well-motivated and the approach (lazy getter via Bug — dead code in
|
|
@claude[bot] all four review summary comments (#4351248577, #4411942887, #4412036948, #4412144693, #4417236313, #4427414791) have been triaged:
Thanks for the thorough reviews. |
Review pass — commit 63d8c8dAddressed all open review items:
Verification
Resolved threads
|
Code ReviewOverall: Clean, well-motivated fix for #1095. The lazy-getter approach is the right tool here, tests are comprehensive, and the CHANGELOG/type declaration updates are correct. Issues worth addressing1.
2. Silent failure on The property descriptor has no set(_v) {
throw new TypeError(
'shakapacker.baseConfig is read-only. Use Object.defineProperty to override it.'
)
}3. Minor: test side-effect access
Notes / no action needed
|
Fixes the must-fix isolation flaw plus six optional hardening/coverage items from PR #1107 review: - test/spec/dummy/rspackConfig.test.js: clear WEBPACK_SERVE in loadConfig so configFactory() always returns the [client, server] array the spec indexes, matching rspack.config.js's WEBPACK_SERVE short-circuit and the sibling CLIENT_BUNDLE_ONLY/SERVER_BUNDLE_ONLY deletes. - package/rspack/index.ts: document why RuleSetRule is imported from "webpack" (single shared rule set consumed by both bundlers). - test/package/indexSideEffects.test.js: add a symmetric "memoizes rules" test. - test/package/rspack/indexTypes.test.js: inherit stdout for the tsc compiles so a compile failure surfaces readable diagnostics instead of a raw Buffer. - package/index.ts, package/rspack/index.ts: widen lazy-export setters to accept undefined (the documented lazy-reset path). - package/index.ts: add the load-time lazy-getter guard for parity with the rspack entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- package/index.ts, package/rspack/index.ts: point the lazy-getter install guard at the published .js entry instead of the .ts source, which npm consumers never receive. - package/index.ts: document that only direct `baseConfig` assignment runs the setter; a value-descriptor `Object.defineProperty` override bypasses it and won't propagate to `generateWebpackConfig`. - test/package/indexSideEffects.test.js: clarify that the defineProperty override test only proves the export stays redefinable, not that it reaches config generation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…641) - Surface the Object.defineProperty({value}) override caveat in the public baseConfig JSDoc (index.d.ts and its template) so it shows up in consumers' editors, not just the setter source comment. - Add a TODO(#641) marker to the rspack entry's explanatory comment block, tying the reliance on TypeScript's CommonJS emit to the deferred #641 work. Comment/JSDoc-only changes; no runtime or type-shape changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tsconfig note, test teeth - Correct the baseConfig override documentation (codex P2): direct assignment overrides the read-back value but only feeds generateWebpackConfig/ generateRspackConfig in the fallback where no environments/<NODE_ENV>.js exists. Normal NODE_ENV builds load environments/<env>.js (which require the real base directly), so the override does not affect them. Updated the webpack and rspack setter comments and the baseConfig JSDoc (index.d.ts + template). - Clarify the webpack lazy-getter guard is kept for parity/documentation, not runtime protection (defineProperty on a plain object throws synchronously). - Note in tsconfig.json why module: "commonjs" is required for the rspack lazy named-export mechanism (TODO(#641)). - Give the rspack "does not eagerly load" test teeth: assert the initial require makes zero requireOrError calls, so a newly-added eager dependency can't slip past the mock's empty-object fallthrough. Docs/comment/test-only; no runtime behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…override caveat - test/package/indexTypes.test.js: inherit tsc stdout like the rspack analogue so compile failures surface readable diagnostics instead of a raw Buffer in the thrown error - package/rspack/index.ts: point the lazy-getter mechanism comment at tsconfig.json so the module:commonjs dependency is discoverable from either file - CHANGELOG.md: note in the #1107 Fixed entry that direct baseConfig assignment only affects generate*Config output in the fallback case with no environments/<NODE_ENV>.js Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The getters are installed on a freshly-created plain object, so the Object.defineProperty calls throw synchronously if they ever fail and the guard could never fire. The rspack entry keeps its guard, where getters installed on the CommonJS exports object are real protection. The getter contract is locked by test/package/indexSideEffects.test.js. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consolidate the duplicated lazy getter/setter pattern for `rules` and `baseConfig` in the webpack and rspack entry points into a single `createLazyExport` helper. Both entries now install accessor descriptors from the helper instead of hand-rolled getters, setters, and load flags. Override semantics (lazy first-get caching, assignment override, undefined reset, and defineProperty value-descriptor bypass) are preserved and documented on the helper. Match rspack manifest plugins by constructor-name suffix in the dummy app config. Update and consolidate the side-effect and type tests accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Track computation with a dedicated `loaded` flag instead of an `=== undefined` sentinel, so a `load` that legitimately returns `undefined` is cached once rather than silently re-running its side effects on every access. - Tighten the returned descriptor to a literal type so the configurable/enumerable/accessor invariants are enforced at the call sites instead of being erased to the loose built-in PropertyDescriptor. - Add createLazyExport unit tests covering lazy load, memoization, undefined caching, descriptor shape, assignment override/reset, and value-descriptor bypass. - Add webpack + rspack baseConfig override tests covering both the no-environment-file fallback (override flows in) and the normal NODE_ENV path (override intentionally ignored). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The spec/dummy rspack server config strips manifest plugins by constructor-name suffix. Note in the comment that the match is intentionally broad: any plugin whose constructor name ends in `ManifestPlugin` is removed from the server config, not just the known RspackManifestPlugin/WebpackManifestPlugin aliases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Assigning `undefined` to a lazy export previously re-armed lazy loading instead of caching the assigned value. That inverted standard property semantics and created a footgun (`x = custom || undefined` silently reset to the loader). Since the lazy value type `T` is always a non-nullable object in practice (Configuration, RspackConfigWithDevServer, RuleSetRule[]) and nothing depended on the reset behavior, the setter now caches whatever is assigned — including `undefined` — like any other property. - Narrow the descriptor setter type from `T | undefined` to `T`. - Update the createLazyExport JSDoc and the webpack/rspack entry-point override-semantics comments. - Flip the five reset tests to assert assignment caches the value as-is without re-running the loader. This behavior is unreleased (PR #1107), so there is no breaking change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two small review nits from the latest re-review: - createLazyExport: note in the JSDoc that a throwing `load` leaves `loaded` false, so the next `get` retries — a side-effectful loader that keeps failing can run more than once. Makes the retry behavior an explicit contract rather than an implementation detail. - indexTypes.test.js: guard the `lib/` symlink with an existsSync check so a checkout missing `lib/` fails with a clear message instead of a bare ENOENT from symlinkSync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "with custom env prefix" test required package/config before setting NODE_ENV/RAILS_ENV. config resolves dev_server from shakapacker.yml at load time, and the production environment has no dev_server section. Under a reordered run (jest --randomize) where the production test executed first, NODE_ENV=production leaked into this test, config.dev_server was undefined, and the test crashed with "Cannot set properties of undefined (setting 'env_prefix')". Set the development env vars before requiring config so the test is independent of execution order. Surfaced while running the suite under --randomize during review of #1107. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
97e8603 to
5ac330f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac330ff9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // single shared rule set (../rules/rspack.js) consumed by both bundlers, and | ||
| // rspack's rule shape is compatible with webpack's. Using the webpack type keeps | ||
| // the `rules` export type identical across the webpack and rspack entry points. | ||
| import type { RuleSetRule } from "webpack" |
There was a problem hiding this comment.
This type-only import is emitted into package/rspack/index.d.ts as import type { RuleSetRule } from "webpack", so an rspack-only TypeScript app fails declaration resolution even when it only imports generateRspackConfig. The rspack installer preset only installs rspack packages (lib/install/package.json lines 2-7), not webpack or @types/webpack, so this makes fresh rspack TS configs require an unrelated webpack type package; use an rspack/local structural rule type or avoid exposing a webpack-typed rules declaration here.
Useful? React with 👍 / 👎.
Review SummaryThis PR correctly fixes #1095 by introducing Three findings surfaced that are worth addressing before merge: 1. PR description contradicts implementation — setter caches
|
| ) | ||
| const shakapackerObject = source.match( | ||
| /const shakapacker = \{([\s\S]*?)\n\}/ | ||
| )[1] |
There was a problem hiding this comment.
Unguarded null dereference: if the regex doesn't match (e.g. the variable is renamed or the closing } loses its leading newline), source.match(...) returns null and [1] throws a TypeError — an opaque crash rather than a useful assertion failure.
| )[1] | |
| const shakapackerObject = source.match( | |
| /const shakapacker = \{([\s\S]*?)\n\}/ | |
| )?.[1] | |
| if (!shakapackerObject) { | |
| throw new Error( | |
| "Could not locate `const shakapacker = { ... }` in package/index.ts — was it renamed or reformatted?" | |
| ) | |
| } |
| exports.config = config | ||
| exports.devServer = devServer | ||
| exports.generateWebpackConfig = generateWebpackConfig | ||
| exports.env = env | ||
| exports.moduleExists = moduleExists | ||
| exports.canProcess = canProcess | ||
| exports.inliningCss = inliningCss | ||
| exports.isRspack = isRspack | ||
| exports.isWebpack = isWebpack | ||
| exports.getBundler = getBundler | ||
| exports.getCssExtractPlugin = getCssExtractPlugin | ||
| exports.getCssExtractPluginLoader = getCssExtractPluginLoader | ||
| exports.getDefinePlugin = getDefinePlugin | ||
| exports.getEnvironmentPlugin = getEnvironmentPlugin | ||
| exports.getProvidePlugin = getProvidePlugin | ||
| exports.merge = webpackMerge.merge | ||
| exports.mergeWithCustomize = webpackMerge.mergeWithCustomize | ||
| exports.mergeWithRules = webpackMerge.mergeWithRules | ||
| exports.unique = webpackMerge.unique |
There was a problem hiding this comment.
This 19-line block must manually stay in sync with the shakapacker object above. A new export added to the object but omitted here silently breaks import { newHelper } from 'shakapacker' for native-ESM consumers — require() still works because module.exports = shakapacker captures it, so the regression is invisible to CJS tests.
The consistency is guarded by the regex cross-check in test/package/indexTypes.test.js, but that test runs only after a full tsc compile and requires the regex to match correctly. Consider adding a unit-test guard that reads this block at the source level (e.g. parse exports\.\w+ = lines and compare against the object's own property names) so the check doesn't depend on the compile step or the fragile object-literal regex.
The "with custom env prefix" test required package/config before setting NODE_ENV/RAILS_ENV. config resolves dev_server from shakapacker.yml at load time, and the production environment has no dev_server section. Under a reordered run (jest --randomize) where the production test executed first, NODE_ENV=production leaked into this test, config.dev_server was undefined, and the test crashed with "Cannot set properties of undefined (setting 'env_prefix')". Set the development env vars before requiring config so the test is independent of execution order. Surfaced while running the suite under --randomize during review of #1107. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Fixes an order-dependent (flaky) test in
`test/package/dev_server.test.js` that fails intermittently under `jest
--randomize`.
## Root cause
The `"with custom env prefix"` test required `package/config` **before**
setting `NODE_ENV`/`RAILS_ENV`:
```js
const config = require("../../package/config") // loads dev_server from shakapacker.yml under whatever env is current
config.dev_server.env_prefix = "TEST_SHAKAPACKER_DEV_SERVER"
process.env.NODE_ENV = "development" // too late
```
`config` resolves `dev_server` from `shakapacker.yml` at load time, and
the `production` section has no `dev_server`. When `jest --randomize`
runs the later `"production"` test first, `NODE_ENV=production` leaks
into this test, so `config.dev_server` is `undefined` and line 26
throws:
```
TypeError: Cannot set properties of undefined (setting 'env_prefix')
```
This reproduces ~1 in 8 randomized runs of the file in isolation.
## Fix
Set the development env vars **before** requiring `config`, making the
test independent of execution order. No production code changes.
## Verification
- `yarn jest test/package/dev_server.test.js --randomize` — 20/20 runs
green (previously ~1/8 failed).
- `yarn eslint test/package/dev_server.test.js` — clean.
Surfaced while running the suite under `--randomize` during review of
#1107; kept separate to stay focused.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Test-only reordering and comments; no runtime or config behavior
changes.
>
> **Overview**
> Fixes an order-dependent flake in `test/package/dev_server.test.js`
for the **"with custom env prefix"** case.
>
> The test now sets `NODE_ENV` and `RAILS_ENV` to `development` (and the
prefixed dev-server env vars) **before**
`require("../../package/config")`, because `config` reads `dev_server`
from `shakapacker.yml` at load time and production has no `dev_server`
block. Under `jest --randomize`, a prior production test could leave
`NODE_ENV=production`, leaving `config.dev_server` undefined and causing
`config.dev_server.env_prefix = ...` to throw.
>
> Comments in the test document that behavior. **No production code
changes.**
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
4045332. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Tests**
* Improved coverage for development server environment handling.
* Strengthened test setup and cleanup to prevent environment settings
from leaking between runs.
* Updated the custom environment prefix test to align with how
configuration is loaded during module startup.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Stamps the **`v10.2.0`** release section in `CHANGELOG.md` and adds the user-visible entries that were still missing for PRs merged since `v10.1.0`. Header format matches the repo convention (`## [v10.2.0] - July 3, 2026`) and is parseable by `rakelib/release.rake`'s `extract_changelog_section` (`## [v<npm-version>]`), so `bundle exec rake release` / `sync_github_release` will pick up the notes automatically. ### Changelog changes - **Version header**: inserted `## [v10.2.0] - July 3, 2026` immediately after `## [Unreleased]`; all accumulated entries now live under it, and `## [Unreleased]` is empty. - **Compare links**: `[unreleased]` now compares `v10.2.0...main`; added `[v10.2.0]: …/compare/v10.1.0...v10.2.0`. ### New entries added (were missing) | PR | Section | Note | | --- | --- | --- | | [#1187](#1187) | Added | Babel 8 peer dependency support + preset option compatibility | | [#1184](#1184) | Added | Folded into the #695 AI-prompt entry (gates the React on Rails section on app detection) | | [#1142](#1142) | Fixed | Rspack dev-server config no longer loads in static watch mode (fixes #1137) | ### Already documented (carried into v10.2.0) `#1180`, `#695`, `#1141`, `#1150`, `#1179`, `#1192`, `#1127`, `#1178`, `#1161`, `#1147`. ### Reviewed and intentionally excluded (not user-visible) Docs: `#1145`, `#1148`, `#1152`, `#1155`, `#1183`, `#1188`, `#1189`, `#1193`. CI: `#1151`, `#1168`, `#1171`. Tests / fixtures: `#1128`, `#1154`, `#1167`, `#1186`. Workflow / agent tooling: `#1153`, `#1176`, `#1182`. (`#1107` is already documented under `## [v10.1.0]`.) ## Next step After merge, run the repo's release task (no args) — it reads `v10.2.0` from the changelog and creates the GitHub release from this section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
baseConfig/rulesloading behind lazy, configurable exports sorequire("shakapacker")andrequire("shakapacker/rspack")do not initialize plugin, rules, or manifest code at package-index load time.generateWebpackConfig()andgenerateRspackConfig()still load environment/base config when config generation is requested.baseConfig/ruleswrites the value the getter returns (assigningundefinedresets to lazy loading), andObject.defineProperty(..., { value, writable: true, configurable: true })is also supported. The TypeScript declarations type them as plain (non-readonly) properties to match.generateRspackConfig, while exposing lazy accessor values through the default/CommonJS namespace.spec/dummy/Gemfile.lockfrom10.1.0.rc.1to10.1.0so Bundler frozen mode matches the released gemspec version.Fixes #1095.
Review / Discussion Notes
baseConfig/rulesexports defer their side effects, remain overridable by direct assignment (a strayundefinedresets to lazy loading rather than being cached), and both entry points coverbaseConfigandrules. (An earlier iteration made thesereadonlywith throwing setters; that was reverted — assignment now succeeds and the declarations are non-readonly.)rulesshape suggestion is covered intest/package/indexSideEffects.test.jsbylazily exposes rules with the expected shape.Object.definePropertymigration path is covered for bothbaseConfigandrulesintest/package/indexSideEffects.test.js.package/rspack/index.ts; the relevantrequire()calls cast toRuleSetRule[]andRspackConfigWithDevServer.package/rspack/index.ts, andtest/package/rspack/indexTypes.test.jsverifies the compiled entry still supports the intended ESM interop paths.baseConfigandrulesare dynamic CommonJS accessor descriptors. Node's native ESM named-import detection only works for statically detected CJS exports, so ESM consumers should use the default import namespace for these lazy values:import rspack from "shakapacker/rspack"; const { baseConfig, rules } = rspack. My advice is to keep this pattern until the broader module/export strategy is handled under Replaceexport =#641; switching rspack toexport = rspackExportswould simplify the source, but would risk the native named-import compatibility currently protected for eager exports.CI Notes
Test with RSpack.RspackManifestPlugin, butrspack-manifest-plugininstances use constructor nameWebpackManifestPlugin. The server-only config kept a manifest writer and overwrote the client manifest, leaving onlyserver-bundle.js.WebpackManifestPluginfrom the dummy server-bundle config and add a Jest regression proving the server config does not write over the client manifest.max-classes-per-fileintest/spec/dummy/rspackConfig.test.jsandjest/expect-expectintest/package/rspack/indexTypes.test.js.Validation
git rebase origin/mainyarn --ignore-engines test --runInBand test/package/rspack/indexTypes.test.js test/spec/dummy/rspackConfig.test.js test/package/rspack/indexSideEffects.test.js test/package/indexSideEffects.test.jsyarn --ignore-engines eslint package/rspack/index.ts test/package/rspack/indexTypes.test.js test/spec/dummy/rspackConfig.test.js test/package/rspack/indexSideEffects.test.js test/package/indexSideEffects.test.js --max-warnings 0yarn --ignore-engines eslint . --max-warnings 5yarn --ignore-engines test --runInBandyarn --ignore-engines type-checkgit diff --checkNote
Medium Risk
Public package entry behavior and native ESM import paths change (documented breaking); core config generation paths are heavily tested but touch all consumers of
shakapacker/shakapacker/rspack.Overview
Defers plugin, rules, and manifest initialization until
baseConfig/rulesare read or config is generated, so plainrequire("shakapacker")andrequire("shakapacker/rspack")no longer run that work at load time (fixes #1095). Both entry points use a newcreateLazyExporthelper: memoized getters, overridable via assignment, withgenerateWebpackConfig/generateRspackConfigunchanged for normal env files.Breaking: lazy accessor exports are not Node static CJS named exports. Native ESM must use default import and destructure
baseConfig/rules; on the webpack entry, named imports likeimport { config }now fail at load. CommonJS andgenerate*exports are unchanged.Also strips server-bundle manifest plugins by constructor name
ManifestPluginsuffix (rspack-manifest-plugin reports asWebpackManifestPlugin), with regression tests for dummy rspack multi-compiler setup and compiled ESM interop.Reviewed by Cursor Bugbot for commit 6f8291d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Breaking Changes
Bug Fixes
Tests
Documentation