Remove inert config.server_render_method option (#4415)

[justin808]

Summary

The troubleshooting guide recommended config.server_render_method = 'NodeJS' # for better error messages, but the config validator raised ReactOnRails::Error at boot for any value other than blank or "ExecJS" — so anyone following that doc crashed their app on startup. The option was also completely inert: no runtime code path ever read it (only rake react_on_rails:doctor echoed it back). The configuration README documented it as a live option.

This PR fixes both docs and removes the dead option. Because 17.0.0 is in the RC window, the breaking removal is free.

Fixes #4415

Breaking Change

config.server_render_method is removed.

• The attr, its boot validator check_server_render_method_is_only_execjs, and its RBS signatures are gone.
• Setting config.server_render_method = ... in config/initializers/react_on_rails.rb now raises NoMethodError at boot. Users setting nil or "ExecJS" (which previously booted fine) must delete the line; users setting 'NodeJS' already crashed today via the validator.
• rake react_on_rails:doctor now flags a stale config.server_render_method line under "Deprecated Configuration Settings" so users are guided to delete it, instead of echoing the value back as a custom extension.

Correct current mechanism: the open-source gem always renders on the server with ExecJS; there is no option to select a different server render method. For a standalone Node rendering process, use React on Rails Pro's Node renderer, configured via ReactOnRailsPro.configure.

Changes

• Docs (docs/oss/deployment/troubleshooting.md, docs/oss/configuration/README.md): removed the boot-raising server_render_method recommendation / live-option documentation; added a sentence explaining ExecJS is always used and pointing standalone-Node users at Pro's node renderer.
• configuration.rb: removed the default, attr_accessor entry, initialize kwarg, assignment, the check_server_render_method_is_only_execjs validator, and its call in setup_config_values.
• doctor.rb: removed both server_render_method reads in analyze_custom_extensions (runtime-config branch + initializer text-scan branch); added a removed-setting check to check_deprecated_configuration_settings.
• configuration.rbs: dropped the three signatures (attr, initialize kwarg, validator method).
• configuration_spec.rb: replaced the validator-exercising describe block with a NoMethodError regression assertion.
• doctor_spec.rb: dropped the now-invalid server_render_method: nil stub from the runtime-config instance_double (a verifying double that otherwise rejects the removed method). See "out-of-scope" note below.
• CHANGELOG.md: added a #### Breaking Changes entry under [Unreleased].

QA Evidence

Docs no longer recommend a boot-raising value:

$ grep -n "server_render_method" docs/oss/deployment/troubleshooting.md docs/oss/configuration/README.md
(no output — exit 1)
$ grep -rn "server_render_method = 'NodeJS'" docs/
(no output — exit 1)

Boot loads cleanly without the option (dummy Rails app full environment):

$ cd react_on_rails/spec/dummy && bundle exec ruby -e "require './config/environment'; c = ReactOnRails.configuration; puts 'Configuration loaded OK'; puts 'responds_to server_render_method: ' + c.respond_to?(:server_render_method).to_s"
Configuration loaded OK
responds_to server_render_method: false

Validation

$ (cd react_on_rails && bundle exec rspec spec/react_on_rails/configuration_spec.rb spec/lib/react_on_rails/doctor_spec.rb)
401 examples, 0 failures

$ (cd react_on_rails && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop lib/react_on_rails/configuration.rb lib/react_on_rails/doctor.rb spec/react_on_rails/configuration_spec.rb spec/lib/react_on_rails/doctor_spec.rb)
4 files inspected, no offenses detected

$ (cd react_on_rails && bundle exec rake rbs:validate)
✓ RBS validation passed

Out-of-scope note

react_on_rails/spec/lib/react_on_rails/doctor_spec.rb was outside this item's declared file scope, but its instance_double(ReactOnRails::Configuration, ..., server_render_method: nil) is a verifying double that raises once the attr is removed (the ReactOnRails::Configuration class does not implement the instance method: server_render_method). This one-line stub deletion is a direct, unavoidable consequence of the required attr removal (and is listed as step 6 in #4415), so it was included to keep the PR green. No sibling batch item touches this file.

Not done here (deferred / not in scope): the historical v12→13 upgrade-guide note (docs/oss/upgrading/upgrading-react-on-rails.md:392, already tells users to remove the setting) and llms-full.txt regeneration are outside this item's owned files.

Codex Decision Log

• Doctor guidance placement: chose to add the removed-setting warning to check_deprecated_configuration_settings (which already has the migration-guide link and mirrors the deprecated-attrs report) rather than reword inside analyze_custom_extensions, keeping all "delete this line" guidance in one canonical place.
• Spec approach: replaced the two validator tests with a single NoMethodError regression test to lock in the removal rather than silently deleting coverage.
• doctor_spec.rb edit: included the minimal verifying-double fix despite it being outside the declared scope, because the alternative was shipping a knowingly-red PR; flagged prominently above.

Confidence note: High. The option was provably inert (no runtime reader), the removal is validated by a clean dummy-app boot, and all targeted specs + rubocop + RBS validate pass locally. Risk is limited to the documented breaking change (initializers setting the attr now raise), which is expected in the RC window and covered by CHANGELOG + doctor guidance.

Summary by CodeRabbit

• Documentation
  • Clarified server-side rendering behavior for the open-source and Pro editions.
  • Updated guidance for performance comparison tooling.
  • Removed outdated configuration instructions and replaced them with current setup details.
  • Added a note about preload Subresource Integrity support for older Shakapacker versions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0947f6fe-6f77-4c56-857f-dff319623340

📥 Commits

Reviewing files that changed from the base of the PR and between dccfbda and bd7526a.

📒 Files selected for processing (1)

• llms-full.txt

---

Walkthrough

Changes

Documentation updates

Layer / File(s)	Summary
Server render method guidance llms-full.txt	Documentation removes server_render_method = 'NodeJS' guidance and states that open-source rendering uses ExecJS while Pro Node rendering is configured separately.
Preload SRI and ShakaPerf notes llms-full.txt	Documentation adds the Shakapacker preload SRI note and rewords the ShakaPerf sentence formatting.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR only shows docs regeneration; it does not include the requested config/code removal, RBS/spec updates, or changelog and upgrade-guide entries.	Add the config attr/validator removal, update RBS and specs, and include the breaking-change changelog and upgrade-guide notes for #4415.
Out of Scope Changes check	⚠️ Warning	The ShakaPerf reformatting and preload SRI note are unrelated to removing server_render_method and appear outside the linked issue scope.	Remove the unrelated ShakaPerf and preload SRI doc edits unless they are part of a separate approved objective.

✅ Passed checks (3 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 directly matches the main change: removing the inert server_render_method option.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/4415-remove-server-render-method

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR removes the inert server_render_method configuration option. The main changes are:

• Removed the Ruby configuration accessor, constructor keyword, validator, and RBS signatures.
• Updated doctor reporting to flag stale initializer lines.
• Replaced the old configuration and troubleshooting docs with ExecJS and Pro renderer guidance.
• Added breaking-change notes and updated focused specs.

Confidence Score: 4/5

The code changes look mergeable after updating the stale generated documentation.

• Configuration removal is internally consistent.
• Doctor now catches normal stale initializer lines.
• One shipped aggregate docs file can still direct users to a boot-breaking setting.

llms-full.txt

Important Files Changed

Filename	Overview
react_on_rails/lib/react_on_rails/configuration.rb	Removes the server_render_method option from the configuration API and setup validation.
react_on_rails/lib/react_on_rails/doctor.rb	Stops reporting server_render_method as a custom extension and adds stale-line detection.
react_on_rails/sig/react_on_rails/configuration.rbs	Removes the matching RBS API entries for the deleted option.
react_on_rails/spec/react_on_rails/configuration_spec.rb	Updates coverage to assert the removed setter raises NoMethodError.
react_on_rails/spec/lib/react_on_rails/doctor_spec.rb	Removes the obsolete verifying-double stub for the deleted accessor.
docs/oss/configuration/README.md	Replaces the removed option section with current ExecJS and Pro renderer guidance.
docs/oss/deployment/troubleshooting.md	Removes the stale NodeJS setting from the source troubleshooting guide, while generated docs still need the same update.
CHANGELOG.md	Documents the breaking removal and migration path.

_{Reviews (1): Last reviewed commit: "Fix CHANGELOG PR link to #4423" | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e4e16bb69

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

[claude]

Review: PR #4423 - Remove inert config.server_render_method option

What it does: Removes the dead config.server_render_method config option (attr, initializer kwarg, boot validator, RBS sigs) since it never had a live runtime code path and its validator would raise for the exact value the troubleshooting docs recommended ('NodeJS'). Adds a rake react_on_rails:doctor deprecation notice for stale initializer lines, updates docs/CHANGELOG, and updates specs accordingly.

Review scope: gh pr diff 4423 (8 files, +16/-54), plus full reads of the touched Ruby files (configuration.rb, doctor.rb) and specs for context.

Findings: None survived verification. Checked specifically for:

• Dangling references to server_render_method anywhere else in the repo (generator templates, other rake tasks, JS/TS, other docs) - none found; only the intentional doctor check and the new regression spec remain.
• Orphaned rubocop disable/enable comments around analyze_custom_extensions in doctor.rb after the mid-method deletion - both sides were removed together, no mismatch.
• RBS (configuration.rbs) staying in sync with the Ruby source - all three removed signatures match exactly what was deleted from configuration.rb.
• Call sites of Configuration.new and ReactOnRails.configuration - none pass server_render_method, so the trimmed initialize signature does not break anything.
• The instance_double verifying double in doctor_spec.rb no longer stubs the removed attr, matching that analyze_custom_extensions no longer reads it.
• CHANGELOG entry format against AGENTS.md changelog conventions - correct PR 4423 self-link and Breaking Changes header, precedented elsewhere in the file.

One minor, non-blocking observation: the new server_render_method branch in check_deprecated_configuration_settings (doctor.rb) has no direct spec coverage, but this is consistent with its three untested sibling checks in the same method (generated_assets_dirs, skip_display_none, defer_generated_component_packs) - a pre-existing pattern, not a regression introduced by this PR.

Overall this is a clean, well-scoped removal with docs, RBS, and specs all kept in sync.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[justin808]

Fixed in bd7526a. Ran node script/generate-llms-full.mjs from the repo root and committed the regenerated llms-full.txt.

• grep -n "server_render_method = 'NodeJS'" llms-full.txt → no matches (the boot-breaking snippet is gone).
• The #### server_render_method section and the config.server_render_method = nil example are also removed from the aggregate.
• node script/generate-llms-full.mjs --validate → exit 0.
• llms-full-pro.txt was unaffected (not committed).

The only remaining server_render_method mention in llms-full.txt is the historical v12→13 upgrade-guide line instructing users to remove the setting, which is correct guidance and aligns with this PR.

[justin808]

+ci-run-hosted

[claude]

Code Review

This PR removes the inert config.server_render_method option (dead config that never had a runtime reader, and whose validator raised on any non-blank/non-ExecJS value — directly contradicting the troubleshooting doc's own recommendation). The change is clean and well-scoped: attr_accessor, initialize kwarg, default, validator, and RBS signatures are removed together; doctor.rb's custom-extensions reporting is trimmed and a new stale-setting warning is added to check_deprecated_configuration_settings; specs are updated to assert NoMethodError instead of the old validator behavior. Verified: no other runtime references to server_render_method remain in react_on_rails/, the rubocop Metrics disable/enable pair around analyze_custom_extensions is correctly removed as a matched pair, and the RBS file has no dangling signatures.

One gap survived review:

docs/oss/configuration/configuration-deprecated.md — removed option isn't documented in the canonical removed-options reference, despite the PR establishing exactly the precedent this file exists for.
This file has a ## Removed Options section with a worked example (immediate_hydration, removed in v16.6.0) giving Status/description/Migration. docs/oss/configuration/README.md (edited by this PR) still tells readers "For deprecated configuration options, see configuration-deprecated.md" right after the new server_render_method blurb — but no entry for server_render_method was added there. A user who hits the new NoMethodError and follows that link will find immediate_hydration but nothing about server_render_method. Given the file's stated purpose and the fresh precedent, this looks like a straightforward addition to make (mirroring the immediate_hydration entry: removed in this version, why, and the one-line migration).

Minor/optional: check_deprecated_configuration_settings's new server_render_method branch (react_on_rails/lib/react_on_rails/doctor.rb:1616-1618) has no spec exercising it — though this is consistent with the pre-existing lack of coverage for its sibling checks (generated_assets_dirs, skip_display_none, defer_generated_component_packs) in doctor_spec.rb, so it's not a regression specific to this PR.

Everything else — the breaking-change framing, CHANGELOG entry, and the NoMethodError regression test — is solid and matches the stated scope.

[github-actions]

Hosted CI Requested

Triggered 9 workflow(s) for bd7526ae7b40.
Mode: optimized hosted CI (path-selected by script/ci-changes-detector).
Added ready-for-hosted-ci, so future commits will keep running optimized hosted CI until +ci-stop-hosted is used.

View progress in the Actions tab.