Skip to content

Update Rspack v2 dev-server defaults - #1179

Merged
justin808 merged 5 commits into
mainfrom
jg-codex/rspack-v2-lazy-dev-server
Jun 30, 2026
Merged

Update Rspack v2 dev-server defaults#1179
justin808 merged 5 commits into
mainfrom
jg-codex/rspack-v2-lazy-dev-server

Conversation

@justin808

@justin808 justin808 commented Jun 29, 2026

Copy link
Copy Markdown
Member

Summary

  • update Rspack support to v2-only package, install, switch-bundler, dummy-app, and docs metadata, including @rspack/dev-server and rspack-manifest-plugin ^5.2.2
  • make generated Rspack dev-server configs set top-level lazyCompilation: false by default while preserving explicit top-level user overrides
  • tighten doctor diagnostics for missing/stale Rspack v2 dependencies and refresh docs/examples for Rspack v2 semantics

Before / After

  • Before: generated Rspack dev-server configs could inherit Rspack lazy compilation behavior that is unsafe for Rails split dev-server topology, and docs still referenced stale v1/experiments.lazyCompilation behavior.
  • After: generated Rspack dev-server config disables top-level lazyCompilation by default under WEBPACK_SERVE unless the app explicitly configures a top-level value.
  • Before: Rspack package/install/docs allowed v1-compatible ranges and stale manifest/dev-server assumptions.
  • After: Rspack support targets v2 only, requires @rspack/dev-server, uses rspack-manifest-plugin ^5.2.2, and keeps the Rspack css-loader ^7.1.4 floor scoped to Rspack-specific paths while preserving webpack-compatible core ranges.
  • Before: doctor could miss mixed or incomplete Rspack v2 stacks.
  • After: doctor reports missing @rspack/dev-server, unsupported Rspack v1 packages, and stale rspack-manifest-plugin floors.

#984 / default preservation evidence

  • Regression coverage in test/package/rspack/index.test.js preserves user Rspack defaults for optimization/runtimeChunk, splitChunks, compression/minimizer behavior, and browser target propagation.
  • The dev-server lazyCompilation tests cover default normalization and explicit top-level override preservation.

Test plan

  • RUBOCOP_CACHE_ROOT=tmp/rubocop_cache .agents/bin/validate
  • RUBOCOP_CACHE_ROOT=tmp/rubocop_cache bundle exec rake run_spec:generator
  • yarn test --runInBand test/packages/package-metadata.test.js test/packages/install-smoke.test.js
  • RUBOCOP_CACHE_ROOT=tmp/rubocop_cache bundle exec rspec spec/shakapacker/doctor_spec.rb spec/shakapacker/doctor_optional_peer_spec.rb
  • /Users/justin/.agents/skills/autoreview/scripts/autoreview --mode local --prompt 'Final closeout review after full validation passed...'

Refs #984.

Summary by CodeRabbit

  • New Features

    • Rspack support now targets v2 only, with updated install guidance and package requirements.
    • Dev-server configs now default to lazyCompilation: false during development, while still allowing custom overrides.
  • Bug Fixes

    • Improved Rspack dependency handling to include the full required package set, reducing install and switch issues.
    • Updated compatibility checks and warnings to better surface outdated or mixed Rspack versions.
  • Documentation

    • Clarified Rspack installation, migration, and peer-dependency guidance across the docs.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5fd6cb0e-1ad4-4227-a28d-4dce9c2d1b31

📥 Commits

Reviewing files that changed from the base of the PR and between 78c16fb and 4b833df.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • lib/install/template.rb
  • lib/shakapacker/bundler_switcher.rb
  • lib/shakapacker/doctor.rb
  • package/environments/development.ts
  • spec/shakapacker/doctor_spec.rb
  • test/package/environments/development-rspack-react-refresh.test.js
  • test/packages/package-metadata.test.js

Walkthrough

Rspack support is restricted to v2 only (^2.0.0). @rspack/dev-server is added as a required peer/dependency across all manifests, install templates, and the shakapacker-rspack supplemental package. The generated Rspack dev-server config now sets lazyCompilation: false. React Refresh plugin resolution is simplified to the v2 named export. Doctor diagnostics are expanded for v2 enforcement. All docs, tests, and fixtures are aligned.

Changes

Rspack v2-only upgrade

Layer / File(s) Summary
Package manifests and install template: v2 pins and @rspack/dev-server
package.json, lib/install/package.json, packages/shakapacker-rspack/package.json, lib/install/template.rb, lib/shakapacker/bundler_switcher.rb
All Rspack peer/dep ranges updated to ^2.0.0; @rspack/dev-server added as a required entry; css-loader floored at ^7.1.4 for rspack builds in the install template; bundler_switcher gains a package_names helper to strip version suffixes before uninstall operations.
dev-server lazyCompilation: false and React Refresh v2 resolution
package/environments/development.ts
Sets lazyCompilation: false in the rspack dev-server config spread; simplifies ReactRefreshRspackPlugin resolution to check only the v2 named export (with a warning/skip for other shapes); adds post-merge traversal of builtin:swc-loader rules to set jsc.transform.react.refresh = true.
Doctor: Rspack v2 peer enforcement and version helpers
lib/shakapacker/doctor.rb
check_rspack_peer_deps requires @rspack/dev-server and rspack-manifest-plugin >=5.2.2; detects v1-resolved packages in v2-only positions; rspack_major_version now checks both @rspack/core and @rspack/cli; new helpers declared_package_dependencies, package_version_status, package_version_from_specifier, installed_package_version centralize version resolution; package_installed? uses the broader declared-dep set.
JS tests: lazyCompilation and React Refresh export shape
test/package/environments/development-rspack-react-refresh.test.js, test/package/rspack/index.test.js
Mock updated to named export shape; SWC refresh transform assertions added; legacy export shapes assert console.warn and no plugin instance; lazyCompilation asserted false when dev server runs and undefined otherwise; loadRspackIndex reworked and lazyCompilation override tests added.
Ruby specs: doctor, bundler switcher, optional deps, rake tasks, dev server runner
spec/shakapacker/doctor_spec.rb, spec/shakapacker/bundler_switcher_spec.rb, spec/shakapacker/doctor_optional_peer_spec.rb, spec/shakapacker/optional_dependencies_spec.rb, spec/shakapacker/rake_tasks_spec.rb, spec/shakapacker/dev_server_runner_spec.rb
Doctor specs extended for v1 mix and missing/stale peer scenarios; bundler switcher expects @rspack/dev-server@^2.0.0 in add/remove calls; package_installed? tests cover peerDependencies/optionalDependencies; rake tasks spec adds shakapacker.yml isolation; dev server runner stubs detect_port!.
Package metadata and install-smoke tests
test/packages/package-metadata.test.js, test/packages/install-smoke.test.js
New assertions that @rspack/* peers are v2-only in root and lib/install manifests and that the install template applies a css-loader version floor for rspack; @rspack/dev-server added to rspack supplemental requiredPeers.
spec/dummy and Rakefile fixture updates
spec/dummy/package.json, spec/dummy/README.md, Rakefile
spec/dummy bumped to Rspack v2 and Node `^20.19.0
Documentation
docs/rspack.md, docs/dependency-strategy.md, docs/peer-dependencies.md, docs/optional-peer-dependencies.md, docs/common-upgrades.md, docs/installation.md, docs/migration/v10.1-supplemental-packages.md, docs/rspack_migration_guide.md, docs/blog/..., packages/shakapacker-rspack/README.md, CHANGELOG.md
All docs updated to reflect v2-only support, @rspack/dev-server as required, new lazyCompilation section, updated install commands, and corrected version ranges throughout.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • shakacode/shakapacker#1116: Both PRs modify ReactRefreshRspackPlugin resolution in package/environments/development.ts and the corresponding React Refresh dev-environment Jest tests.
  • shakacode/shakapacker#1133: Both PRs update the shakapacker-rspack supplemental package's peer-dependency contract and Rspack version ranges.
  • shakacode/shakapacker#1142: Both PRs modify the rspack generated dev-server environment in package/environments/development.ts (this PR sets lazyCompilation: false, that PR conditionally omits the devServer block).

Suggested labels

documentation, p2

Suggested reviewers

  • G-Rath
  • tomdracz
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately highlights the main dev-server default change in the Rspack v2 update.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/rspack-v2-lazy-dev-server

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves Rspack support to the v2-only stack. The main changes are:

  • Rspack install and package metadata now require v2 packages.
  • Generated Rspack dev-server config disables top-level lazy compilation by default.
  • Doctor checks now flag missing, stale, or unsupported Rspack dependencies.
  • Rspack docs, examples, and lockfiles were refreshed for the new defaults.

Confidence Score: 5/5

This looks safe to merge after small diagnostic and dependency-hardening cleanups.

  • No blocking issues found in the changed code.
  • The remaining findings are conditional edge cases in doctor reporting and switch-bundler dependency resolution.

lib/shakapacker/doctor.rb and lib/shakapacker/bundler_switcher.rb

Important Files Changed

Filename Overview
package/environments/development.ts Adds the Rspack dev-server lazy-compilation default and narrows React Refresh plugin loading to the v2 named export.
lib/shakapacker/doctor.rb Adds Rspack v2 dependency checks, with one diagnostic edge case for peer-only declarations.
lib/install/template.rb Updates installer dependency merging so Rspack installs get the css-loader floor needed for v2.
lib/shakapacker/bundler_switcher.rb Adds @rspack/dev-server to the Rspack switcher dependency set, though the switcher still installs it without the v2 range.
packages/shakapacker-rspack/package.json Updates the supplemental package peer set for Rspack v2, including @rspack/dev-server and rspack-manifest-plugin ^5.2.2.

Reviews (1): Last reviewed commit: "Update Rspack v2 dev-server defaults" | Re-trigger Greptile

Comment thread lib/shakapacker/doctor.rb Outdated
Comment thread lib/shakapacker/bundler_switcher.rb Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md`:
- Line 16: Update the “optional peers” wording in the blog post so it no longer
lists `@rspack/dev-server` as opt-in; the earlier supplemental-packages section
already makes it required. Adjust the later paragraph that describes optional
peers to match the behavior of shakapacker-rspack, keeping the wording
consistent with the supplemental package list and the mention of
shakapacker-webpack.

In `@lib/shakapacker/doctor.rb`:
- Around line 979-985: The version check in rspack_major_version_for currently
prefers installed_rspack_major_version over the declared
package_json_dependency_version, which can mask unsupported package.json ranges
when node_modules is newer. Update the doctor logic so the diagnostic validates
both sources independently: keep using rspack_major_from_specifier for the
declared dependency and installed_rspack_major_version for the resolved install,
and ensure the check fails if either one maps to an unsupported major. Apply the
same behavior to the related helper path in the 1025-1030 logic so `@rspack/cli`
and rspack-manifest-plugin do not pass just because the local install is newer.
🪄 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: 1669ef8b-9465-4480-82bc-19af3b49c7b8

📥 Commits

Reviewing files that changed from the base of the PR and between c65b0a7 and 78cf7a6.

⛔ Files ignored due to path filters (3)
  • spec/dummy/package-lock.json is excluded by !**/package-lock.json
  • spec/dummy/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (32)
  • CHANGELOG.md
  • Rakefile
  • docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md
  • docs/common-upgrades.md
  • docs/dependency-strategy.md
  • docs/installation.md
  • docs/migration/v10.1-supplemental-packages.md
  • docs/optional-peer-dependencies.md
  • docs/peer-dependencies.md
  • docs/rspack.md
  • docs/rspack_migration_guide.md
  • lib/install/package.json
  • lib/install/template.rb
  • lib/shakapacker/bundler_switcher.rb
  • lib/shakapacker/doctor.rb
  • package.json
  • package/environments/development.ts
  • packages/shakapacker-rspack/README.md
  • packages/shakapacker-rspack/package.json
  • spec/dummy/README.md
  • spec/dummy/package.json
  • spec/generator_specs/e2e_template/template.rb
  • spec/shakapacker/bundler_switcher_spec.rb
  • spec/shakapacker/dev_server_runner_spec.rb
  • spec/shakapacker/doctor_optional_peer_spec.rb
  • spec/shakapacker/doctor_spec.rb
  • spec/shakapacker/optional_dependencies_spec.rb
  • spec/shakapacker/rake_tasks_spec.rb
  • test/package/environments/development-rspack-react-refresh.test.js
  • test/package/rspack/index.test.js
  • test/packages/install-smoke.test.js
  • test/packages/package-metadata.test.js

Comment thread docs/blog/2026-05-10-shakapacker-10-1-supplemental-packages.md
Comment thread lib/shakapacker/doctor.rb
@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch from 78cf7a6 to d243f45 Compare June 30, 2026 13:46

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d243f458fb

ℹ️ 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".

Comment thread lib/shakapacker/bundler_switcher.rb
Comment thread lib/shakapacker/doctor.rb Outdated
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Review

This PR updates Rspack support to target v2 only, adds @rspack/dev-server as a required dependency, sets lazyCompilation: false by default for the Rails split dev-server topology, and tightens doctor diagnostics for Rspack v2. The scope is large (34 files, +1607/-903) but the changes are coherent and the test coverage for the new lazy-compilation default and doctor checks is solid.

Two issues found:


1. Bug — npm install --save in migration guide installs build tools as production deps

File: docs/rspack_migration_guide.md (Step 1 of the manual migration)

npm install --save @rspack/core rspack-manifest-plugin   # wrong: goes to dependencies
npm install --save-dev @rspack/cli @rspack/dev-server

@rspack/core and rspack-manifest-plugin are build-time tools and should be devDependencies. Every other doc file in the repo (docs/rspack.md, docs/common-upgrades.md, docs/migration/v10.1-supplemental-packages.md) correctly uses --save-dev / -D for all four packages. The correct command is:

npm install --save-dev @rspack/core rspack-manifest-plugin @rspack/cli @rspack/dev-server

2. Bug — version specifiers in DEFAULT_RSPACK_DEPS break manager.remove when switching away from Rspack

File: lib/shakapacker/bundler_switcher.rb

DEFAULT_RSPACK_DEPS = {
  dev: %w[@rspack/cli@^2.0.0 @rspack/dev-server@^2.0.0 @rspack/plugin-react-refresh@^2.0.0],
  prod: %w[@rspack/core@^2.0.0 rspack-manifest-plugin@^5.2.2]
}

The version specifiers (e.g. @rspack/cli@^2.0.0) are correct for manager.add (all package managers accept pkg@version for install). However, remove_dependencies passes these same strings verbatim to manager.remove, which expects bare package names. When a user runs rake shakapacker:switch_bundler webpack -- --install-deps while on Rspack, the remove call becomes:

manager.remove(["@rspack/cli@^2.0.0", "@rspack/dev-server@^2.0.0", ...])

Package managers handle version specifiers in remove inconsistently — pnpm may reject them outright; Yarn Berry is stricter still. DEFAULT_WEBPACK_DEPS uses bare names for comparison. There is also no test covering the rspack→webpack switch with install_deps: true, so this path has zero CI coverage.

Fix: strip the version specifier before calling remove, or store names and versions separately. The stale comment on line 22 (# package names only, no versions) should also be updated to reflect the new format.

Comment thread docs/rspack_migration_guide.md Outdated
Comment thread docs/rspack_migration_guide.md Outdated
Comment thread lib/shakapacker/bundler_switcher.rb

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74f4ed42b7

ℹ️ 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".

Comment thread package/environments/development.ts
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review: Update Rspack v2 dev-server defaults

This PR targets Rspack v2 exclusively: it bumps all @rspack/* peer/install ranges to ^2.0.0, adds @rspack/dev-server as a required dependency, simplifies React Refresh plugin resolution to the v2 named export only, and disables lazyCompilation by default in the Rspack dev-server config. The doctor diagnostics are tightened to flag v1 packages, stale compound ranges, and missing dev-server dependencies.

Two issues worth addressing:


1. package_installed? scope is inconsistent with declared_package_dependencies (medium)

check_bundler_dependencies (called at line 132 of run_all_checks) uses package_installed?, which only looks at dependencies and devDependencies:

# lib/shakapacker/doctor.rb:1132
def package_installed?(package_name)
  package_json = read_package_json
  dependencies = (package_json["dependencies"] || {}).merge(package_json["devDependencies"] || {})
  dependencies.key?(package_name)
end

The PR expanded declared_package_dependencies to also cover peerDependencies and optionalDependencies, and the new test "with essential rspack dependencies declared as peers" validates that check_peer_dependencies no longer false-alarms for that layout. However, check_bundler_dependencies runs in the same run_all_checks call and still uses the narrow package_installed?. A package.json with @rspack/core only in peerDependencies will:

  • pass check_peer_dependencies — no "Missing essential rspack dependency" ✓
  • fail check_bundler_dependencies — "Missing required dependency '@rspack/core' for Rspack" ✗ (false alarm)

The existing test covers the isolated check_peer_dependencies call, not the full doctor run. The fix is to update package_installed? to delegate to declared_package_dependencies, or to eliminate the redundant check.


2. package_version_below? error message conflates stale range with stale install (low)

# lib/shakapacker/doctor.rb:1043
[declared, installed].compact.any? { |version| version < minimum }

declared is the floor extracted from the semver specifier — "^5.1.0" yields Gem::Version("5.1.0"). If node_modules has 5.2.2 installed, the check fires because 5.1.0 < 5.2.2, even though the running version is fine. The resulting message —

Unsupported rspack-manifest-plugin version: Shakapacker requires rspack-manifest-plugin ^5.2.2 for Rspack v2.

— reads as "your installed version is too old" when the installed version IS 5.2.2. Consider separating the two cases in the message so users can tell whether they need to npm install or just update their package.json constraint.


Everything else looks solid: package_names version-stripping is correct for all scoped/unscoped formats, rspack_major_from_specifier's ||-splitting handles surrounding whitespace cleanly, lazyCompilation: false scoped to the dev-server spread is intentional and tested, and the declared_package_dependencies merge order (dependencies wins last) is consistent with prior package_json_dependency_version behaviour.

Comment thread lib/shakapacker/doctor.rb Outdated
Comment thread lib/shakapacker/doctor.rb
@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch from 249b4c9 to 20bff84 Compare June 30, 2026 14:22
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Review

This PR makes a well-structured upgrade to Rspack v2 support: drops v1 compatibility, sets lazyCompilation: false for dev-server configs, adds @rspack/dev-server to the required stack, tightens the doctor checks, and drops legacy React Refresh export-shape fallbacks. The package_names helper that strips version suffixes for removal/display is correctly applied at every call site. Test coverage is thorough.

Three findings survived verification, ranked by severity:

1. Doctor requires @rspack/dev-server declared for ALL rspack setups — including production-only apps

check_rspack_peer_deps lists @rspack/dev-server in essential_rspack with no guard for whether the dev server is actually in use. Any app whose package.json does not explicitly declare @rspack/dev-server in some section will get: 'Missing essential rspack dependency: @rspack/dev-server (^2.0.0)'.

Two affected populations: (a) Production/CI-only rspack apps that only run rake assets:precompile — @rspack/dev-server arrives as a transitive dep of @rspack/cli@^2.0.0 but is not declared, so the doctor warns. (b) npm 7+ supplemental-package users whose app package.json contains only shakapacker-rspack — the peers are auto-installed into node_modules but not declared in the app manifest.

Inline comment on the specific line below.

2. package_version_below? uses any? on [declared_floor, installed] — generates false positives

package_version_from_specifier('^5.0.0') returns Gem::Version('5.0.0') (the range floor). The check [Gem::Version('5.0.0'), Gem::Version('5.3.0')].any? { |v| v < Gem::Version('5.2.2') } returns true even though 5.3.0 satisfies the requirement. So an app declaring rspack-manifest-plugin ^5.0.0 with 5.3.0 installed receives 'Unsupported rspack-manifest-plugin version' even though its runtime is fine. Consider using only the installed version when available, or change the error text to 'declared range floor is stale' rather than 'Unsupported version'.

Inline comment on the specific line below.

3. Comment above DEFAULT_RSPACK_DEPS is now incorrect

The comment says 'package names only, no versions' but the constant now holds version-decorated strings like @rspack/cli@^2.0.0. Inline comment below.

Comment thread lib/shakapacker/doctor.rb Outdated
Comment thread lib/shakapacker/doctor.rb Outdated
Comment thread lib/shakapacker/bundler_switcher.rb
@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch from 20bff84 to 78c16fb Compare June 30, 2026 14:27
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review: Update Rspack v2 dev-server defaults

This PR upgrades Rspack support to v2-only, makes @rspack/dev-server a required managed dependency, sets lazyCompilation: false by default in the generated dev-server config, and drops the legacy React Refresh plugin fallback exports. The overall direction is correct. Three items worth addressing:

1. @rspack/dev-server treated as a blocking essential for all rspack users

check_rspack_peer_deps adds @rspack/dev-server to the essential_rspack hash and emits a hard @issues error when it is absent. Any rspack app running a build-only workflow (CI asset precompile, no dev server) that hasn't explicitly declared @rspack/dev-server in its package.json will hit this blocking error even though its build is valid. The doctor's @issues path is intended for broken configurations, not missing optional tooling.

If @rspack/cli@^2 hard-depends on @rspack/dev-server at import time (not only at serve time), the package will always be present in node_modules and the distinction doesn't matter. But declared_package_dependencies reads the app's package.json, not node_modules — so apps that rely on transitive hoisting (common with npm 7+ auto-peer-install via shakapacker-rspack) will still trigger the error even when the package is physically installed.

Suggested fix: move @rspack/dev-server to @warnings unless the project's shakapacker config has dev-server enabled (similar to how @rspack/plugin-react-refresh is handled as an optional check), or confirm that @rspack/cli@^2 lists @rspack/dev-server as a regular (non-peer) dependency so it always resolves transitively.

2. Dead method package_version_below?

package_version_below? is defined in this PR but never called anywhere. All actual call sites use package_version_status directly to distinguish between declared-below and installed-below. The helper should either be removed or wired up.

3. Overlapping hash literals in check_rspack_peer_deps

essential_rspack and rspack_v2_only_packages share three identical keys (@rspack/core, @rspack/cli, @rspack/dev-server). If the minimum required version of any of these changes in a future patch, it's easy to update one hash and forget the other, silently producing stale v1-detection results. Consider extracting those three into a shared constant or deriving one hash from the other.

Comment thread lib/shakapacker/doctor.rb
Comment thread lib/shakapacker/doctor.rb
Comment thread lib/shakapacker/doctor.rb Outdated
Comment thread lib/shakapacker/doctor.rb
Comment thread package/environments/development.ts
Comment thread package/environments/development.ts Outdated
Comment thread lib/shakapacker/doctor.rb
Comment thread lib/shakapacker/doctor.rb
Comment thread lib/shakapacker/doctor.rb
Comment thread package/environments/development.ts Outdated
Comment thread package/environments/development.ts Outdated
Comment thread package/environments/development.ts Outdated
Comment thread CHANGELOG.md Outdated
Comment thread package/environments/development.ts
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review — Update Rspack v2 dev-server defaults

This PR migrates Shakapacker's Rspack support to v2-only, adds @rspack/dev-server as a required peer, sets lazyCompilation: false in generated dev configs to prevent Rails dev-server topology breakage from Rspack CLI's auto-lazy behavior, and tightens doctor diagnostics for the new v2 dependency floor. The core logic is sound and well-tested. Four issues worth addressing before merge:


1. declared_below in package_version_status is dead code — doctor gives no warning on fresh clones with a stale declared range

File: lib/shakapacker/doctor.rb line 1062

exact_version_specifier? only accepts bare version numbers (5.0.0) and rejects anything starting with ^ or ~. Every real package.json uses a range specifier, so declared_below is permanently false and the elsif manifest_status[:declared_below] branch at line 362 is unreachable.

Concrete failure: an app with "rspack-manifest-plugin": "^5.0.0" in package.json and no node_modules (e.g. a fresh git clone before npm install) receives zero doctor warning, even though the declared range allows installing 5.0.x or 5.1.x which are below the new 5.2.2 minimum. The installed_below path will catch it after install, but not before. The fix is to drop the exact_version_specifier? guard and compare the floor of the declared range directly (inline comment has a suggested replacement).


2. declared_package_dependencies merges peerDependencies, which can suppress missing-dep errors

File: lib/shakapacker/doctor.rb line 1083

The new declared_package_dependencies helper merges the app's peerDependencies section as the base layer. The check_rspack_peer_deps check at line 337 uses deps[package] as a presence signal, so a package declared only in peerDependencies (not physically installed — common for library packages that peer-dep on their bundler) satisfies the check and silences the "Missing essential rspack dependency" error. The original code only merged dependencies + devDependencies, where declared → installed is the reliable invariant.


3. Module-level mutable flag in development.ts is fragile

File: package/environments/development.ts line 20

rspackReactRefreshTransformEnabled is written as a side effect inside rspackDevConfig() and consumed at module scope after the merge. This works for the single call path today but breaks under any scenario where rspackDevConfig() is called more than once (e.g. in tests), and the implicit data flow makes the code harder to reason about. Returning the flag as part of the function return value would make the dependency explicit (inline comment has a suggested refactor).


4. CHANGELOG "Changed" entry is missing PR attribution

File: CHANGELOG.md line 21

The "Changed Rspack support to target Rspack v2 only" entry has no [PR #1179](...) by [username](...) link, unlike every other entry in this file (including the adjacent "Fixed" entry from the same PR). A code suggestion with the attribution is attached as an inline comment.


Overall: The lazy-compilation default and v2-only migration are well-designed. Items 1 and 2 are correctness issues in the new doctor diagnostics worth fixing before the release ships. Items 3 and 4 are lower-priority but easy to clean up.

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Summary

This PR updates Shakapacker's Rspack support to v2-only: it tightens installer package ranges, adds @rspack/dev-server as a required dep, defaults lazyCompilation: false during dev-server runs to avoid Rails topology issues, and refreshes doctor diagnostics for mixed/stale Rspack v1 stacks. The overall direction is correct and well-motivated. There are a few correctness issues in the doctor logic and the React Refresh transform wiring worth addressing before merge.


Bugs

doctor.rbdeclared_below false positive for caret ranges (line 1056)
package_version_from_specifier extracts only the floor of the range (strips ^/~), so "^5.2.0" returns 5.2.0. That compares 5.2.0 < 5.2.2 → true, firing the "declared_below" error — but ^5.2.0 resolves to >=5.2.0 <6.0.0, which fully satisfies 5.2.2. Users coming from the old Shakapacker default of "rspack-manifest-plugin": "^5.0.0" will hit a spurious error on their first doctor run even though npm install would already give them a compliant version.

doctor.rbrspack_major_version_for prefers stale declaration over installed v2 (line 1005)
The old code explicitly preferred the installed version (the deleted comment documented this). The new return 1 if declared == 1 || installed == 1 means a user who has @rspack/core v2 in node_modules but a stale "^1.0.0" in package.json gets a hard error even though their build is running v2. If the intent is to also warn about stale declarations, that case should be a warning rather than a blocking error.

development.tsenableRspackReactRefreshTransform skips oneOf/nested rules (line 122)
The function only walks top-level module.rules entries. Any builtin:swc-loader rule inside a oneOf or nested rules array is silently skipped — refresh: true is never set and React Refresh fails without any error.

development.ts — user extraConfig swc-loader rules miss the transform (line 147)
The transform runs on environmentConfig at module-load time, before generateRspackConfig merges user-supplied extraConfig. Custom builtin:swc-loader rules added by users never receive refresh: true, so React Refresh is silently broken for those files even though the plugin is active.


Smaller issues

development.ts — silent no-op when jsc.transform.react block is absent (line 135)
If a builtin:swc-loader rule omits options.jsc.transform.react, the function skips it silently instead of creating the block. The built-in Shakapacker rules include it today, but any future rule change or user addition would cause a silent miss.

doctor.rb — optional @rspack/plugin-react-refresh v1 fires a hard error (line 344)
RSPACK_V2_ONLY_DEPS includes this optional package, so a user who still has it at v1 with all core packages at v2 gets a hard @issues error with no indication it is optional. A warning is more appropriate here.

doctor.rbdeclared_below is dead code before npm install (line 1066)
package_version_from_specifier returns nil for any ||-joined range, so the declared-range check silently produces declared_below: false for any multi-range specifier. In CI without a populated node_modules, only installed_below works — and it also returns nil — so the check is a no-op before installation.

@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch 2 times, most recently from f5232c1 to 9618b51 Compare June 30, 2026 14:50
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review: Update Rspack v2 dev-server defaults

This PR updates Shakapacker's Rspack support to v2-only, sets lazyCompilation: false when the Shakapacker-managed dev server is running (scoped to WEBPACK_SERVE=true), refactors the doctor's dependency detection to cover more package.json sections, and tightens the installer's css-loader floor for Rspack. The overall direction is correct and the test coverage looks solid.

Four things worth addressing before merge:

1. css-loader floor bump in template.rb is fragile (medium) — The bump to ^7.1.4 is applied to common_deps before the merge, but the new merge order (common_deps.merge(peers)) gives peers last-write-wins semantics. It only works today because the "rspack" section in lib/install/package.json has no css-loader entry. If that entry is ever added at any version, the floor bump is silently overridden. The merge order flip also changes behaviour for the webpack path: common deps used to win over webpack peers; they no longer do.

2. declared_below in package_version_status has a blind spot (medium)declared_below: !installed && declared && declared < minimum fires only when the package is not installed. A project that currently has rspack-manifest-plugin@5.3.0 installed but declares "^4.0.0" in package.json gets no Doctor warning. After rm -rf node_modules && npm install, npm could resolve to 4.x and the user would hit the bug. Consider removing the !installed guard so both checks can fire independently.

3. peerDependencies of the host app included in dependency detection (medium)declared_package_dependencies now merges the host app's peerDependencies section. For apps that are themselves libraries, packages listed there for their own consumers may not be installed. check_rspack_peer_deps and check_webpack_peer_deps will then silently suppress "missing dependency" warnings for them. Consider whether peerDependencies should be included in the doctor's declared-deps lookup.

4. stimulus_likely_used? not updated alongside package_installed? (low)stimulus_likely_used? still builds its own dependencies + devDependencies hash (old pattern) instead of calling installable_package_dependencies. This creates an inconsistency: if Stimulus is in optionalDependencies, package_installed? returns true while stimulus_likely_used? returns false, silently skipping the keepClassNames SWC warning.

Comment thread lib/install/template.rb Outdated
Comment thread lib/shakapacker/doctor.rb
Comment thread lib/shakapacker/doctor.rb
@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch from 9618b51 to 75b1877 Compare June 30, 2026 14:55

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75b1877f45

ℹ️ 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".

Comment thread lib/shakapacker/doctor.rb
@justin808
justin808 force-pushed the jg-codex/rspack-v2-lazy-dev-server branch from 75b1877 to 4b833df Compare June 30, 2026 15:08
@justin808
justin808 merged commit 536a5fb into main Jun 30, 2026
22 checks passed
@justin808
justin808 deleted the jg-codex/rspack-v2-lazy-dev-server branch June 30, 2026 15:21
justin808 added a commit that referenced this pull request Jul 4, 2026
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant