Restructure supplemental package dependencies (#1131) - #1133
Conversation
Move bundler singletons from `dependencies` to required `peerDependencies` in both supplemental packages, and loosen `~` to `^` for all non-shakapacker packages. - shakapacker-webpack: webpack, webpack-cli, webpack-assets-manifest move to required peerDependencies; terser-webpack-plugin moves from optional peer to direct dependency (package/optimization/webpack.ts always requires it for the default minimizer). - shakapacker-rspack: @rspack/core, @rspack/cli, rspack-manifest-plugin move to required peerDependencies. - Both: shakapacker stays as a tilde-pinned direct dependency (lockstep with core); all other constraints use caret with floors aligned to main shakapacker's peer ranges, so the supplemental never narrows what bare shakapacker accepts. Modern package managers (npm 7+, pnpm, yarn 2+) auto-install required peers, preserving the one-command install experience. Yarn classic 1.x users see a peer warning listing the required packages; the Rails installer continues to write all required deps explicitly into the user's package.json. Adds test/packages/package-metadata.test.js to lock in the contract: singletons are required peers, terser-webpack-plugin is a direct dep, shakapacker uses tilde, no other tilde constraints exist, and supplemental peer ranges don't narrow main's ranges unless intentionally curated. Refs: #1131
Revises every place that described the supplemental packages as bundling
the bundler stack via direct `dependencies` to instead describe the new
required-peer-dependency shape. Adds yarn 1 caveats wherever the
one-command install claim previously held unconditionally.
- docs/dependency-strategy.md: rewrite the summary, per-package tables,
Version Pinning Philosophy section, and What Each User Type Installs
examples. Add the rationale for choosing peer deps over direct deps
(singleton bundler instance invariant). Update Resolved Decisions and
the release-sequencing note.
- docs/migration/v10.1-supplemental-packages.md: add a package-manager
support table and split the "After" examples into modern-PM (auto-peer-
install) and yarn-1 (explicit peer list) variants.
- docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md: revise
the 30-second pitch and "What changed" section.
- packages/shakapacker-{webpack,rspack}/README.md: rewrite Install sections
with auto-peer-install vs yarn-1 commands.
- CHANGELOG.md: add an Unreleased entry describing the restructure with
rationale and modern-PM behavior.
No installer changes — the Rails installer (lib/install/template.rb)
already writes core + all peers explicitly into the user's package.json,
which gives true one-command UX on every package manager. Switching the
installer to use supplemental packages instead would be a behavior change
beyond the scope of issue #1131 (PR #1096 kept supplemental adoption
opt-in for the v10.x line).
Refs: #1131
WalkthroughThe PR restructures supplemental packages so bundler singletons become required peer dependencies (caret ranges), keeps ChangesPeer-Dependency Strategy and Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review — PR #1133: Restructure supplemental package dependenciesSummaryThe dependency topology change is well-motivated and correctly implemented. Moving bundler singletons ( Issues1. Missing rspack peer-alignment test (moderate)The 2.
|
Greptile SummaryRestructures
Confidence Score: 4/5The package.json restructuring is correct and the documentation is thorough; the new test file guards the key contracts well for the webpack supplemental but leaves the rspack supplemental's peer-range narrowings without test coverage. The core dependency changes are structurally sound and webpack test coverage is solid. The rspack peer-range alignment test is missing — the helper function is parameterized but was never wired up for rspack — leaving three current range divergences from main undocumented and unguarded against future drift. The collectTildeOffenders OR-range gap is a smaller future trap. test/packages/package-metadata.test.js — the rspack peer-range alignment test is absent; packages/shakapacker-rspack/package.json — the intentional narrowings (rspack v1 dropped, react-refresh missing -0) have no test documentation. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[npm 7+ / pnpm / yarn 2+
yarn add shakapacker-webpack] --> B[shakapacker-webpack installed]
B --> C[Required peers auto-installed:
webpack ^5.101.0
webpack-cli ^7.0.2
webpack-assets-manifest ^6.0.0]
B --> D[Direct deps installed:
shakapacker ~10.1.0-rc.1
terser-webpack-plugin ^5.3.1]
E[yarn 1
yarn add shakapacker-webpack] --> F[shakapacker-webpack installed]
F --> G[Direct deps installed:
shakapacker ~10.1.0-rc.1
terser-webpack-plugin ^5.3.1]
F --> H[Peer warning printed:
webpack, webpack-cli,
webpack-assets-manifest missing]
H --> I[User adds peers manually]
J[npm 7+ / pnpm / yarn 2+
yarn add shakapacker-rspack] --> K[shakapacker-rspack installed]
K --> L[Required peers auto-installed:
@rspack/core ^2.0.0
@rspack/cli ^2.0.0
rspack-manifest-plugin ^5.0.0]
K --> M[Direct dep installed:
shakapacker ~10.1.0-rc.1]
Reviews (1): Last reviewed commit: "Update docs and CHANGELOG for supplement..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shakapacker-webpack/package.json (1)
98-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd trailing newline.
The file must end with a trailing newline character. As per coding guidelines, all files matching
**/*should end with a trailing newline character.📝 Proposed fix
"publishConfig": { "access": "public" } } +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shakapacker-webpack/package.json` at line 98, Add a single trailing newline character at the end of the package.json file so the file ends with a newline; update the file content to ensure the final character is '\n' (no other changes required).packages/shakapacker-rspack/package.json (1)
61-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd trailing newline.
The file must end with a trailing newline character. As per coding guidelines, all files matching
**/*should end with a trailing newline character.📝 Proposed fix
"publishConfig": { "access": "public" } } +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shakapacker-rspack/package.json` at line 61, Add a single trailing newline character at the end of the shakapacker-rspack package.json file so the file ends with a newline (ensure the final byte is '\n'); this satisfies the repository's requirement that all files matching **/* end with a trailing newline.
🧹 Nitpick comments (1)
docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md (1)
24-24: 💤 Low valueConsider simplifying the redundant phrase.
The phrase "duplicate copies" is redundant since "duplicate" already implies "copies." Consider using "duplicates", "duplicate instances", or "multiple copies" for more concise writing.
✍️ Suggested simplification
-10.1 shifts that responsibility to the supplemental packages. Each one declares the singleton bundler stack as **required peer dependencies** so modern package managers auto-install them while still surfacing version conflicts as warnings rather than silently installing duplicate copies of webpack: +10.1 shifts that responsibility to the supplemental packages. Each one declares the singleton bundler stack as **required peer dependencies** so modern package managers auto-install them while still surfacing version conflicts as warnings rather than silently installing duplicates of webpack:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md` at line 24, Replace the redundant phrase "duplicate copies of webpack" with a more concise alternative such as "duplicate instances of webpack", "duplicates of webpack", or "multiple copies of webpack" in the sentence that currently reads "Each one declares the singleton bundler stack as **required peer dependencies** so modern package managers auto-install them while still surfacing version conflicts as warnings rather than silently installing duplicate copies of webpack:"; update the phrase "duplicate copies of webpack" accordingly to improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: The CHANGELOG entry is missing the PR number and contributor
attribution; update the leading sentence of the existing entry (the line
starting "- **Restructured `shakapacker-webpack` and `shakapacker-rspack`
dependency declarations**") to include the PR link and author in the required
format — e.g. prepend " [PR
`#1133`](https://github.com/shakacode/shakapacker/pull/1133) by
[justin808](https://github.com/justin808)" immediately after the bold title so
the line reads like the provided example.
---
Outside diff comments:
In `@packages/shakapacker-rspack/package.json`:
- Line 61: Add a single trailing newline character at the end of the
shakapacker-rspack package.json file so the file ends with a newline (ensure the
final byte is '\n'); this satisfies the repository's requirement that all files
matching **/* end with a trailing newline.
In `@packages/shakapacker-webpack/package.json`:
- Line 98: Add a single trailing newline character at the end of the
package.json file so the file ends with a newline; update the file content to
ensure the final character is '\n' (no other changes required).
---
Nitpick comments:
In `@docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md`:
- Line 24: Replace the redundant phrase "duplicate copies of webpack" with a
more concise alternative such as "duplicate instances of webpack", "duplicates
of webpack", or "multiple copies of webpack" in the sentence that currently
reads "Each one declares the singleton bundler stack as **required peer
dependencies** so modern package managers auto-install them while still
surfacing version conflicts as warnings rather than silently installing
duplicate copies of webpack:"; update the phrase "duplicate copies of webpack"
accordingly to improve clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e9ac6f7-3cce-449c-a68f-cb4362fff728
📒 Files selected for processing (9)
CHANGELOG.mddocs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.mddocs/dependency-strategy.mddocs/migration/v10.1-supplemental-packages.mdpackages/shakapacker-rspack/README.mdpackages/shakapacker-rspack/package.jsonpackages/shakapacker-webpack/README.mdpackages/shakapacker-webpack/package.jsontest/packages/package-metadata.test.js
- CHANGELOG: add PR #1133 link and author attribution per project format - shakapacker-rspack: align @rspack/plugin-react-refresh peer with main (add `-0` prerelease suffix so 2.0.0-rc.x still resolves) - shakapacker-webpack: lower webpack-cli floor from ^7.0.2 to ^7.0.0 (no documented reason for the patch-level floor) - package-metadata test: detect tildes inside OR ranges (e.g. "^1.0.0 || ~2.0.0"), return structured mismatch objects so filtering is robust to format changes, and add a parallel rspack alignment test with intentional-narrowing exceptions for @rspack/core and @rspack/cli
Code ReviewOverviewThe approach is sound — moving bundler singletons to required peer dependencies is the correct fix for the duplicate-webpack issue in #1131. Bundler packages are singletons by design ( FindingsBug: In In npm semver, Maintenance concern: esbuild OR-chain needs extending with every new minor
This is technically correct (npm semver treats Minor:
Test: The name is slightly misleading — the set also catches cases where the supplemental is wider than main (e.g. if a supplemental peer accepts older versions main doesn't). Not a bug (the Set-based exclusion works correctly), but renaming to What looks good
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Pnpm docs omit terser-webpack-plugin
- Updated pnpm and Yarn webpack migration/install docs to require explicit terser-webpack-plugin so core Shakapacker can resolve it under strict package managers.
Or push these changes by commenting:
@cursor push b661ea9fac
Preview (b661ea9fac)
diff --git a/docs/migration/v10.1-supplemental-packages.md b/docs/migration/v10.1-supplemental-packages.md
--- a/docs/migration/v10.1-supplemental-packages.md
+++ b/docs/migration/v10.1-supplemental-packages.md
@@ -15,14 +15,14 @@
| yarn 2+ (Berry) | **No** — see below |
| yarn 1 (classic) | **No** — see below |
-pnpm and Yarn PnP keep dependency boundaries strict: packages imported by your app's config files must be listed directly in your app's `package.json`. The default generated configs import `shakapacker`, and many customized webpack configs import `webpack`, so keep those direct dependencies alongside the supplemental package:
+pnpm and Yarn PnP keep dependency boundaries strict: packages imported by your app's config files or resolved by core Shakapacker must be listed directly in your app's `package.json`. The default generated configs import `shakapacker`, many customized webpack configs import `webpack`, and production webpack builds resolve `terser-webpack-plugin`, so keep those direct dependencies alongside the supplemental package:
```sh
# webpack, pnpm
-pnpm add --save-dev shakapacker-webpack shakapacker webpack webpack-cli webpack-assets-manifest
+pnpm add --save-dev shakapacker-webpack shakapacker terser-webpack-plugin webpack webpack-cli webpack-assets-manifest
# webpack, yarn
-yarn add --dev shakapacker-webpack shakapacker webpack webpack-cli webpack-assets-manifest
+yarn add --dev shakapacker-webpack shakapacker terser-webpack-plugin webpack webpack-cli webpack-assets-manifest
# rspack, pnpm
pnpm add --save-dev shakapacker-rspack shakapacker @rspack/core @rspack/cli rspack-manifest-plugin
@@ -124,6 +124,7 @@
"devDependencies": {
"shakapacker-webpack": "~10.1.0",
"shakapacker": "~10.1.0",
+ "terser-webpack-plugin": "^5.3.1",
"webpack": "^5.101.0",
"webpack-cli": "^7.0.0",
"webpack-assets-manifest": "^6.0.0"
diff --git a/packages/shakapacker-webpack/README.md b/packages/shakapacker-webpack/README.md
--- a/packages/shakapacker-webpack/README.md
+++ b/packages/shakapacker-webpack/README.md
@@ -11,14 +11,14 @@
npm install --save-dev shakapacker-webpack-pnpm and Yarn PnP keep dependency boundaries strict: packages imported by your app's config files must be listed directly in your app's package.json. The default generated webpack config imports shakapacker, and many customized configs import webpack, so keep those direct dependencies alongside the supplemental package:
+pnpm and Yarn PnP keep dependency boundaries strict: packages imported by your app's config files or resolved by core Shakapacker must be listed directly in your app's package.json. The default generated webpack config imports shakapacker, many customized configs import webpack, and production builds resolve terser-webpack-plugin, so keep those direct dependencies alongside the supplemental package:
# pnpm
-pnpm add --save-dev shakapacker-webpack shakapacker webpack webpack-cli webpack-assets-manifest
+pnpm add --save-dev shakapacker-webpack shakapacker terser-webpack-plugin webpack webpack-cli webpack-assets-manifest
# yarn
-yarn add --dev shakapacker-webpack shakapacker webpack webpack-cli webpack-assets-manifest
+yarn add --dev shakapacker-webpack shakapacker terser-webpack-plugin webpack webpack-cli webpack-assets-manifest(The Rails shakapacker:install task writes all required deps into your package.json regardless of package manager.)
@@ -52,7 +52,7 @@
}
-Optional peers (transpilers, `webpack-dev-server`, `mini-css-extract-plugin`, CSS preprocessors, etc.) stay only if your app uses those features. Run `yarn install` (or the npm/pnpm equivalent) and the lockfile collapses to the managed stack. npm 7+ can auto-install the required peers; pnpm and Yarn users should keep `shakapacker`, `webpack`, `webpack-cli`, and `webpack-assets-manifest` as explicit `devDependencies` unless their config imports the wrapper package directly.
+Optional peers (transpilers, `webpack-dev-server`, `mini-css-extract-plugin`, CSS preprocessors, etc.) stay only if your app uses those features. Run `yarn install` (or the npm/pnpm equivalent) and the lockfile collapses to the managed stack. npm 7+ can auto-install the required peers; pnpm and Yarn users should keep `shakapacker`, `terser-webpack-plugin`, `webpack`, `webpack-cli`, and `webpack-assets-manifest` as explicit `devDependencies` so core Shakapacker and app config imports resolve under strict dependency boundaries.
### Migrating from core's webpack peer set
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 7a00207. Configure here.
|
Reviewed the two follow-up commits ( A few smaller things worth tracking, none blocking merge:
The current state is a clear improvement over rc.1 and the doc story is now accurate. 👍 from my end, with item 1 as a meaningful follow-up before GA. |
Code ReviewOverall: The restructuring is technically correct and addresses #1131 well. Moving bundler singletons to required peer dependencies is the right call — the duplicate-webpack silent failure mode is a real bug class. The contract tests in Substantive issuesYarn 2+ (Berry)
|
Locks in the resolution matrix that docs/migration/v10.1-supplemental-packages.md now documents: - npm wrapper-only: shakapacker resolves from app root (npm hoisting) - npm explicit-deps: shakapacker resolves - pnpm wrapper-only: shakapacker does NOT resolve (strict isolation under .pnpm/) - pnpm explicit-deps: shakapacker resolves The pnpm wrapper-only assertion is the exact failure mode that issue #1131 review feedback flagged — a user adopting `shakapacker-webpack` alone on pnpm hits `Cannot find module 'shakapacker'` because the default generated config does `require("shakapacker")` from app code. Implementation: - Packs `shakapacker` core and `shakapacker-webpack` into tarballs in a tmpdir - Builds two temp app fixtures (wrapper-only, explicit-deps) using `file:` references + `overrides`/`pnpm.overrides`/`resolutions` so the supplemental's `dependencies.shakapacker: "~10.1.0-rc.1"` resolves to the local tarball regardless of what's on the registry - Runs `npm install` and `pnpm install` against each fixture - Verifies resolution via `node -e "require.resolve('shakapacker')"` from the app CWD (not the jest CWD — pnpm's strict layout only shows up when you actually probe from inside the installed app) Opt-in via `RUN_INSTALL_SMOKE=1` because it shells out to package managers, hits the npm registry for bundler peers, and takes ~15s end-to-end on a warm cache. Auto-skips with a visible `test.todo` placeholder when the env var is unset, when shakapacker hasn't been built (no `package/index.js`), or when neither npm nor pnpm is on PATH. The PM probe spawns from `os.tmpdir()` so corepack's project-spec interception doesn't false-negative pnpm in workspaces that pin a different `packageManager`. Refs: #1131
Adds a worked example to docs/migration/v10.1-supplemental-packages.md
showing how to change `require("shakapacker")` to
`require("shakapacker-webpack")` (and the rspack equivalent) so apps on
pnpm or Yarn PnP can drop `shakapacker` from their explicit
`devDependencies` without breaking the default config's import.
Also adds an honest framing note about the path's limits: the wrapper
re-exports core but does not proxy webpack itself, so configs that
import webpack directly (e.g., `new webpack.DefinePlugin(...)`) still
need webpack declared as a direct dep on strict-resolution PMs. The
wrapper-import path saves exactly one direct dep (`shakapacker`); it is
not a path to a truly minimal `devDependencies` block.
Refs: #1131
|
Review: Restructure supplemental package dependencies (#1131) The structural shift is technically correct and addresses the issue well. The documentation is thorough, the package manager compatibility table is accurate, and the package-metadata contract tests are a good addition that will catch future regressions. Correctness
Required peer semantics: Omitting an entry from Version ranges: Singleton floors are well-chosen. The Tests The Inline comments flag a few smoke-test issues:
Documentation Good. One pre-GA issue: the pnpm/yarn "After" examples in the migration guide use Summary The dependency topology decision is sound and the documentation accurately captures the package-manager nuances. The issues are all in the test/docs layer — nothing structurally wrong with the |
- Assert appRootAssertions in the npm wrapper-only install-smoke test so npm's flat hoisting of shakapacker-webpack direct deps (terser-webpack-plugin) is exercised explicitly. - Bump beforeAll timeout from 120s to 180s to match per-test budget; three sequential npm pack invocations can push past the smaller limit on cold CI. - Note that node-linker=isolated and auto-install-peers=true are pnpm-only keys (npm silently ignores them) so the shared .npmrc isn't misread as affecting npm resolution. - Add an rc-pin callout to the supplemental packages migration guide so users copying examples during the 10.1 rc window pin to ~10.1.0-rc.1 rather than the unpublished ~10.1.0 stable range. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review follow-up summary (63ee61b)Four actionable review threads addressed in this commit:
All 19 previously-unresolved threads on this PR are now resolved:
Local checks before push: |
Code ReviewOverviewSolid architectural fix. Moving bundler singletons ( Issues1. Doc/code mismatch —
2. The pnpm wrapper-only test correctly asserts that 3. The Nits
What's correct
VerdictOne concrete doc/code mismatch to fix (the |
| | sass-loader | `16.0.7` | Paired with sass | | ||
| | Package | Range | When needed | | ||
| | ---------------------------- | ------------------------------------------------ | ---------------- | | ||
| | @rspack/plugin-react-refresh | `^1.0.0 \|\| ^2.0.0-0` | React HMR | |
There was a problem hiding this comment.
Doc/code mismatch: the table shows ^1.0.0 || ^2.0.0-0 (prerelease-compatible) but packages/shakapacker-rspack/package.json now declares ^1.0.0 || ^2.0.0 (stable-only). The PR description explicitly says the -0 suffix was removed because Rspack 2.x is stable — the doc table should match.
| | @rspack/plugin-react-refresh | `^1.0.0 \|\| ^2.0.0-0` | React HMR | | |
| | @rspack/plugin-react-refresh | `^1.0.0 \|\| ^2.0.0` | React HMR | |
| // pnpm isolates shakapacker under .pnpm/, so app-level | ||
| // `require("shakapacker")` (e.g. from webpack.config.js) cannot find it. | ||
| expect(resolvesFromAppRoot(dir, "shakapacker")).toBe(false) | ||
| }, 180000) |
There was a problem hiding this comment.
The test validates the failure case documented in the migration guide (shakapacker is transitive and not reachable under pnpm's strict isolation). But it doesn't validate the success case: that the required peers (webpack, @rspack/core, etc.) do resolve from the app root after pnpm's auto-install-peers installs them.
That positive assertion is the key outcome of moving singletons to required peerDependencies — without it, this test can pass even if auto-install-peers silently failed to install the required peers.
Consider adding a resolvesFromAppRoot check for spec.packageName's required peers (available on the spec as the keys of spec.explicitDeps, minus terser-webpack-plugin which is a direct dep not a peer). For example:
// Required peers should be auto-installed at app root by pnpm 8+ with auto-install-peers=true
const requiredPeers =
spec.name === "webpack"
? ["webpack", "webpack-cli", "webpack-assets-manifest"]
: ["@rspack/core", "@rspack/cli", "rspack-manifest-plugin"]
requiredPeers.forEach((mod) => {
expect(resolvesFromAppRoot(dir, mod)).toBe(true)
})![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: 761f7db72f
ℹ️ 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".
| }) | ||
|
|
||
| const packTarball = (cwd, destDir, spawn = spawnSync) => { | ||
| const r = spawn("npm", ["pack", "--json", "--pack-destination", destDir], { |
There was a problem hiding this comment.
shouldRun only requires that npm exists, but beforeAll always calls packTarball(), which executes npm pack --json --pack-destination. Those flags are not available in npm 6 (the v6 npm pack synopsis only supports --dry-run), so an environment with pnpm >=7 plus npm 6 will enter the suite and fail before any pnpm assertions run. Add an npm-version guard (or a fallback packing path) so the smoke test can skip cleanly instead of hard-failing on older npm.
Useful? React with 👍 / 👎.
| "-e", | ||
| `const { createRequire } = require("module"); ` + | ||
| `const path = require("path"); ` + | ||
| `const [packageName, mod] = process.argv.slice(1); ` + |
There was a problem hiding this comment.
Bug: off-by-one in process.argv slice
When Node runs with the -e flag, process.argv[1] is an empty string (not the first user argument), so slice(1) yields ['', packageName, mod]. The destructuring then assigns packageName = '' and mod = <the actual packageName>, causing appRequire.resolve("" + "/package.json") to try to resolve the absolute path /package.json, which throws and makes the function always return false.
This would cause the pnpm wrapper-only assertion (expect(resolvesFromInstalledPackage(...)).toBe(true)) to fail whenever RUN_INSTALL_SMOKE=1 is set.
| `const [packageName, mod] = process.argv.slice(1); ` + | |
| `const [packageName, mod] = process.argv.slice(2); ` + |
Code ReviewOverviewThis PR correctly addresses #1131 by restructuring the supplemental packages so bundler singletons are required peer dependencies rather than direct dependencies. The core design — avoiding duplicate webpack/rspack instances and the silent Bug
|
Code Review: Restructure supplemental package dependencies (#1131)Overall: Solid change. The design is correct, the documentation is thorough, and the test coverage is the best I have seen for a package metadata change. What the PR doesMoves bundler singletons ( Design consideration:
|
| Placement | npm 7+ | pnpm (auto-install-peers=true, pnpm 8+ default) |
|---|---|---|
| Direct dep (current) | App root via hoisting ✓ | Inside .pnpm/shakapacker-webpack.../ — NOT accessible to shakapacker ✗ |
| Required peer | App root via auto-install ✓ | App root via auto-install ✓ |
The call site is requireOrError("terser-webpack-plugin") inside shakapacker (not shakapacker-webpack). Under pnpm's strict layout, shakapacker-webpack's direct dep lives in an isolated subtree that shakapacker cannot reach. If it were a required peer, pnpm's auto-install-peers=true would install it at app root where shakapacker can find it — same result as npm, without any explicit app-level declaration.
The migration docs correctly document the current pnpm workaround (add terser-webpack-plugin explicitly), but a required peer would make it automatic on modern pnpm. The one downside is yarn 1, which only warns about missing peers rather than installing them — but the migration guide already tells yarn 1 users to add explicit deps anyway.
Worth considering before GA.
Other observations
Version ranges
webpack: "^5.101.0"is a relatively high floor but matches mainshakapacker's peer range, so consistent.esbuild: ">=0.14.0 <1.0.0"is a clean simplification of the previous per-minor OR chain.@rspack/plugin-react-refresh: "^1.0.0 || ^2.0.0"(dropping^2.0.0-0) correctly excludes 2.x pre-releases as documented.
Tests
The package-metadata.test.js design is excellent. collectTildeOffenders correctly handles OR-separated ranges ("^1.0.0 || ~2.0.0"), and expectOnlyKnownMismatches catches stale allowlist entries as well as unexpected regressions.
The install-smoke.test.js opt-in pattern (RUN_INSTALL_SMOKE=1) is the right call for a test that shells out to package managers. The .npmrc approach to pin node-linker=isolated explicitly defends against a caller environment with shamefully-hoist=true masking the pnpm failure mode.
RC version strings
packages/shakapacker-webpack/package.json and packages/shakapacker-rspack/package.json both carry "shakapacker": "~10.1.0-rc.1" in their dependencies. The docs note these must be updated to "~10.1.0" before GA publish. A release script assertion or CI check on this would prevent an RC pin shipping in the GA package.
Minor issues are attached as inline comments.
| "webpack": "~5.106.2", | ||
| "webpack-cli": "~7.0.2", | ||
| "webpack-assets-manifest": "~6.5.1" | ||
| "terser-webpack-plugin": "^5.3.1" |
There was a problem hiding this comment.
Design question: Under pnpm with auto-install-peers=true (pnpm 8+ default), a required peer of shakapacker-webpack would be installed at the app root — where shakapacker's requireOrError("terser-webpack-plugin") can find it. As a direct dep, it lands inside .pnpm/shakapacker-webpack.../node_modules/, which pnpm's strict isolation keeps invisible to shakapacker. The end result is that pnpm users still need to declare terser-webpack-plugin explicitly (correctly documented), even though auto-install-peers=true would handle it automatically if this were a required peer. The only case where a direct dep is strictly better than a required peer is yarn 1, which only warns about missing peers — but the migration guide already tells yarn 1 users to add explicit deps anyway.
| return Object.entries(peers) | ||
| .filter( | ||
| ([name, supplementalRange]) => | ||
| mainPeers[name] && mainPeers[name] !== supplementalRange |
There was a problem hiding this comment.
The equality check (mainPeers[name] !== supplementalRange) flags any deviation from main, including cases where the supplemental has a wider range than main (e.g., supplemental adds an OR arm that main doesn't have). The comment on line 106 says "should never be stricter", but the test is more conservative than that — it catches drift in either direction. This is probably the right call (prevents silent widening too), but the comment slightly undersells what the test actually enforces. Consider updating the comment to "supplemental peer ranges should match main exactly, except for known intentional narrowings".
| "terser-webpack-plugin": "^5.3.1" | ||
| }, | ||
| requiredPeers: ["webpack", "webpack-cli", "webpack-assets-manifest"], | ||
| appRootAssertions: ["terser-webpack-plugin"] |
There was a problem hiding this comment.
The appRootAssertions array (["terser-webpack-plugin"] for webpack) is checked in the npm wrapper-only test and the pnpm explicit-deps test, but not in the pnpm wrapper-only test (line ~1129 in the diff only checks requiredPeers). Adding a negative assertion there — expect(resolvesFromAppRoot(dir, "terser-webpack-plugin")).toBe(false) — would document the exact pnpm failure mode that the migration guide warns about, and prevent a future pnpm version change (e.g., a shamefully-hoist policy shift) from silently masking the documented behaviour.
…coverage-v1 * origin/main: Teach /update-changelog to drop RC-only regression fixes (#1125) Restructure supplemental package dependencies (#1131) (#1133) [codex] Add Shakapacker brand assets (#1135) [codex] Fix shakapacker config helper binstubs (#1132) Surface v10.1 supplemental packages from README and key docs (#1134) CHANGELOG: merge PR #1120 into PR #1110 entry for v10.1.0-rc.1 (#1126) # Conflicts: # lib/install/bin/diff-bundler-config # lib/install/bin/shakapacker-config # package/configExporter/cli.ts # spec/dummy/bin/shakapacker-config # spec/shakapacker/binstub_sync_spec.rb # spec/shakapacker/helper_binstubs_spec.rb
### Summary Updates `CHANGELOG.md` for `v10.1.0-rc.2` by collapsing the prior `v10.1.0-rc.1` prerelease notes into the new section, moving PR #1133 from Unreleased into Changed, and adding the PR #1132 config helper binstub/export task fix. The release compare links now point Unreleased from `v10.1.0-rc.2` and compare `v10.0.0...v10.1.0-rc.2`. ### Pull Request checklist - [x] ~Add/update test to cover these changes~ - [x] ~Update documentation~ - [x] Update CHANGELOG file ### Other Information Validated with `yarn lint`, `git diff --check`, and review of `git diff origin/main...`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change updating release notes and compare links; no runtime code or dependency changes in this PR. > > **Overview** > Updates `CHANGELOG.md` to publish `v10.1.0-rc.2` by moving the prior prerelease content under the new version header, adding the `shakapacker:export_bundler_config`/binstub fix entry (PR #1132), and relocating the dependency declaration change (PR #1133) from *Unreleased* into `Changed`. > > Also updates the bottom compare links so `Unreleased` now compares from `v10.1.0-rc.2` and adds the `v10.1.0-rc.2` tag comparison link. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4d4dfea. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Summary Stamps the v10.1.0 stable release header (was `v10.1.0-rc.2`) and curates the prerelease entries per `/update-changelog` guidance. ## Curation applied - **Merged PR #1133 → PR #1096** (Added: supplemental npm packages). #1133 restructured the supplemental deps from direct → peer in rc.2, so the original entry's "bundle the managed-build stack as direct dependencies" description was wrong for stable consumers. Rewrote to describe the final peer-deps shape (auto-installed on npm 7+) and credit both PRs. The #1133 standalone Changed entry was removed. - **Merged PR #1132 → PR #1104** (Fixed: ESM binstubs). #1132 fixed the `export_bundler_config` dispatch regression that #1104 introduced in rc.1 when switching from JS to Ruby binstubs; stable users never see the bug, so the fix is RC-only churn that belongs folded into the original entry. Also folded in #1132's minor improvements (NODE_ENV from RAILS_ENV mapping, Node lookup without execution, friendlier non-Error CLI messages). - **Merged PR #1106 → PR #1086** (Fixed: local-path version detection). Both shipped in rc.0 as iterations of the same regex fix; merged into a single entry covering `../`, `./`, and `file:` patterns. - **Lightened Migration Notes**: removed "bundled stack" language now that supplementals declare required peers instead of direct deps. - **Skipped post-rc.2 commit**: PR #1138 (skill tooling guidance) is internal — not user-visible. - **Compare links**: `[Unreleased]` now points from `v10.1.0`; replaced `[v10.1.0-rc.2]` with `[v10.1.0]: v10.0.0...v10.1.0`. ## Test plan - [x] `yarn lint` passes - [x] `yarn prettier --check CHANGELOG.md` passes - [x] File ends with trailing newline - [x] No duplicate section headings - [x] All merged PRs credited After merge, run `bundle exec rake release` (no args) — the rake task picks up `v10.1.0` from CHANGELOG.md and auto-creates the GitHub release. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only changelog edits with no runtime or dependency behavior changes. > > **Overview** > **Curates `CHANGELOG.md` for the v10.1.0 stable release** (replacing the `v10.1.0-rc.2` header dated May 25, 2026) so release notes match what stable users actually ship. > > Prerelease churn is folded into fewer, accurate bullets: supplemental npm packages (**#1096** + **#1133**) now describe **required peer dependencies** (not direct “bundled” deps), with the standalone **#1133** *Changed* entry removed; ESM/config binstub fixes (**#1104** + **#1132**) and local-path version detection (**#1086** + **#1106**) are single *Fixed* items. **Migration Notes** no longer imply a direct bundled stack on supplementals. > > Footer compare links: **`[Unreleased]`** from `v10.1.0`, **`[v10.1.0]`** replaces the rc.2 link (`v10.0.0...v10.1.0`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2b57d5f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nalyze-webpack-output * origin/main: (164 commits) Make Rspack the default bundler for new installs (#1150) Add "Why Shakapacker with Rspack" comparison guide (#1155) Support sass-loader v17 (#1141) [codex] Fix Claude workflow permissions (#1153) Fix README CI badges and add prominent docs link (#1152) Trim README into a docs-site index; relocate content to docs/ and fix deep links (#1148) Tighten helper binstub parity and PATH lookup (#1128) Optimize CI: minimal version matrix on PRs, full matrix on main (#1151) Fix instance-scoped compiler strategies (#1147) Fix Rspack devServer in static watch mode (#1142) Add homepage links for supplemental npm packages (#1145) Release 10.1.0 Update CHANGELOG.md for v10.1.0 (#1139) Expand prerelease-to-stable changelog coalesce guidance in /update-changelog (#1138) Release 10.1.0-rc.2 [codex] Update changelog for v10.1.0-rc.2 (#1136) Teach /update-changelog to drop RC-only regression fixes (#1125) Restructure supplemental package dependencies (#1131) (#1133) [codex] Add Shakapacker brand assets (#1135) [codex] Fix shakapacker config helper binstubs (#1132) ...
1 participant
Summary
Addresses #1131 (G-Rath) on the v10.1.0-rc.1 supplemental packages. Three structural changes:
Bundler singletons → required peer dependencies.
shakapacker-webpackmoveswebpack,webpack-cli, andwebpack-assets-manifestfromdependenciesto requiredpeerDependencies.shakapacker-rspackdoes the same for@rspack/core,@rspack/cli, andrspack-manifest-plugin. Eliminates the silent duplicate-bundler failure mode where an app or transitive dep pinning a different bundler version could end up with two webpacks in the tree (and silently brokeninstanceofchecks in plugins/loaders).~→^for all non-shakapackerdeps. Tilde locked the supplemental to the upstream patch range, forcing a coordinated release for every webpack/rspack/loader minor. Caret lets users pick up compatible upstream releases without waiting.shakapackeritself stays tilde-pinned because the wrapper imports core's internalpackage/configsubpath and needs the matching minor.terser-webpack-plugin→ direct dependency ofshakapacker-webpack.package/optimization/webpack.tsdoesrequireOrError("terser-webpack-plugin")for the default minimizer, so it's always required for any production build using shakapacker's defaults. Moves from optional peer (where npm doesn't warn when missing) to direct dep.Plus one bonus cleanup raised in the issue thread: the supplemental peer ranges were tighter than main
shakapacker's peer ranges (e.g.,webpack-dev-server: ~5.2.3vs main's^5.2.2). Widened them to match — the supplemental should never narrow what bare core accepts.Review confirmation:
@rspack/plugin-react-refreshnow intentionally uses^1.0.0 || ^2.0.0in both core and the rspack supplemental. Rspack 2.x is stable, so this excludes2.0.0-*prereleases instead of preserving the older^2.0.0-0prerelease-compatible range.Behavior on each package manager
nodeLinker: node-modulesThe Rails
shakapacker:installtask continues to write all required deps explicitly into the user'spackage.json(it doesn't use the supplemental packages), so installed apps still get true one-command UX on every package manager.What this PR doesn't do
lib/install/template.rb) already provides true one-command UX by writing core + all peers explicitly. Switching the installer default to use supplemental packages is a behavior change beyond Dependency feedback for v10.1.0 #1131's scope — PR RFC: Tighten dependencies for Shakapacker v11 #1096 explicitly kept supplemental adoption opt-in for the v10.x line.shakapacker'spackage.json. It already follows these conventions (webpack as a peer with^5.101.0, terser-webpack-plugin as a peer with^5.3.1).docs/dependency-strategy.md.Files changed
packages/shakapacker-webpack/package.json— restructure per recommendationspackages/shakapacker-rspack/package.json— restructure per recommendationstest/packages/package-metadata.test.js(new) — locks in the contract (singletons are required peers, terser-webpack-plugin is direct dep, no~outsideshakapacker, supplemental peer ranges don't narrow main's)docs/dependency-strategy.md— rewrite summary, per-package tables, Version Pinning Philosophy, examples, Resolved Decisionsdocs/migration/v10.1-supplemental-packages.md— add PM support table, split "After" examples by PMdocs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md— revise 30-second pitch and What Changedpackages/shakapacker-{webpack,rspack}/README.md— rewrite Install sections with auto-peer-install vs yarn-1 commandsCHANGELOG.md— Unreleased entry documenting the changeTest plan
yarn jest test/packages/package-metadata.test.js— 12 new tests pass (asserts the contract above)yarn jest— full JS suite: 514 tests, 53 suites, all passyarn lint— cleanyarn prettier --check— clean (after autofix)Fixes #1131
Note
Medium Risk
Medium risk because it changes published npm dependency topology for
shakapacker-webpack/shakapacker-rspack, which can affect install/resolution behavior across package managers (npm vs pnpm/Yarn PnP) and production builds relying onterser-webpack-plugin.Overview
Restructures the supplemental npm packages’ dependency model so bundler singletons are no longer bundled as direct deps.
shakapacker-webpacknow declareswebpack,webpack-cli, andwebpack-assets-manifestas requiredpeerDependencies(and movesterser-webpack-pluginto a directdependency), whileshakapacker-rspacksimilarly peers@rspack/core,@rspack/cli, andrspack-manifest-plugin. Version constraints are loosened from~to^/broader ranges for non-shakapackerpackages (includingesbuild), and docs/migration guidance is updated to spell out npm 7+ auto-peer-install vs pnpm/Yarn PnP explicit-deps requirements.Adds new Jest coverage (
test/packages/package-metadata.test.jsand an opt-ininstall-smoke.test.js) to lock in the peer/dependency contract and validate resolution behavior across npm/pnpm, plus updates the changelog/blog/design-rationale docs accordingly.Reviewed by Cursor Bugbot for commit dc1c725. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Chores
terser-webpack-pluginis a direct dependency for the webpack wrapper. Version ranges loosened (tilde → caret) for broader compatibility.Documentation
Tests