Skip to content

[EXPERIMENT] Send render request body as JSON (closes #3280 hypothesis) #3293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AbanoubGhadban wants to merge 1 commit into from
Closed

[EXPERIMENT] Send render request body as JSON (closes #3280 hypothesis) #3293

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion react_on_rails_pro/lib/react_on_rails_pro/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,15 @@ def reset_connection
def render_code(path, js_code, send_bundle)
Rails.logger.info { "[ReactOnRailsPro] Perform rendering request #{path}" }
form = form_with_code(js_code, send_bundle)
perform_request(path, form: form)
# Experiment (issue #3280): use JSON for steady-state render to skip
# form-urlencode CPU/GC. Keep multipart when send_bundle=true because
# form_with_code embeds the bundle file as a Pathname which can't be
# JSON-serialized.
Comment on lines +27 to +30

@claude claude Bot May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

4-line block comment and "Experiment" language are fine for an experiment branch, but project conventions (CLAUDE.md) cap comments at one line and restrict them to non-obvious WHY. For the production PR this should be reduced to something like:

Suggested change
# Experiment (issue #3280): use JSON for steady-state render to skip
# form-urlencode CPU/GC. Keep multipart when send_bundle=true because
# form_with_code embeds the bundle file as a Pathname which can't be
# JSON-serialized.
# Keep multipart when send_bundle=true — bundle Pathname can't be JSON-serialized.

if send_bundle
perform_request(path, form: form)
else
perform_request(path, json: form)

@claude claude Bot May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Watch out for a subtle nil vs omitted difference here. Utils.common_form_data includes "dependencyBundleTimestamps" => dependencies where dependencies is nil when RSC support is disabled. With form encoding that value is typically omitted or sent as an empty string; JSON.generate will produce "dependencyBundleTimestamps":null. The node renderer needs to explicitly handle null for that field — worth a unit test and a Node-side check before the production PR.

end
Comment on lines +31 to +35

@greptile-apps greptile-apps Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 No renderer-side version guard before switching content type

Switching from multipart/form-data to application/json is a wire-protocol change. Any Node renderer that has disabled Fastify's default JSON body parser (e.g. a custom plugin stack, an older version, or a user-patched renderer) will start responding with 415 Unsupported Media Type or silently misparse the body, and the gem has no way to detect or fall back. The existing STATUS_INCOMPATIBLE check only fires when the renderer explicitly returns that status code; it won't catch a JSON-parse failure on the renderer side. The PR description calls this out for the follow-up, but it is worth documenting in a TODO comment near the branch so the production PR doesn't inadvertently ship without it.

end

def render_code_as_stream(path, js_code, is_rsc_payload:)
Expand Down
Loading