Skip to content
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.

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

[WIP] Add support for RSC #1644

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1b4f997
tmp
AbanoubGhadban Jul 22, 2024
b8c63dc
tmp
AbanoubGhadban Jul 29, 2024
9b63d92
make rsc get the right rsc bundle path
AbanoubGhadban Jul 30, 2024
a2094de
fix ts error after rebasing
AbanoubGhadban Nov 4, 2024
5ed4e2b
tmp
AbanoubGhadban Nov 5, 2024
a8d6d10
DRY helper functions
AbanoubGhadban Dec 5, 2024
eebb139
remove duplicate function
AbanoubGhadban Dec 6, 2024
fc89460
revert commiting some lines
AbanoubGhadban Dec 6, 2024
4c3be0e
refactor and add more tests for utils.rb
AbanoubGhadban Dec 18, 2024
20fa53a
add rsc_bundle_js_file config and update utils spec
AbanoubGhadban Dec 18, 2024
943a09d
linting
AbanoubGhadban Dec 18, 2024
a9f3e1a
linting
AbanoubGhadban Dec 18, 2024
222aef8
don't set AsyncLocalStorage in global scope
AbanoubGhadban Dec 18, 2024
21ec6b1
fix mock in webpack assets status checker test
AbanoubGhadban Dec 18, 2024
40c4050
mock that webpacker gem is not installed
AbanoubGhadban Dec 18, 2024
e22d9ca
mock availablity of webpacker and shakapacker gems
AbanoubGhadban Dec 18, 2024
78b61d7
ensure that webpacker is not used when shakapacker in use in tests
AbanoubGhadban Dec 18, 2024
4410f29
remove instance variables of PackerUtils on utils tests
AbanoubGhadban Dec 18, 2024
de99b23
make utils rspec test run using the packer installed on CI
AbanoubGhadban Dec 19, 2024
fd24a52
test bundle_js_file_path with webpacker and shakapacker
AbanoubGhadban Dec 19, 2024
4a36576
linting
AbanoubGhadban Dec 19, 2024
e555e97
remove webpacker test dependency
AbanoubGhadban Dec 19, 2024
07e701e
don't mock gem availablity function
AbanoubGhadban Dec 19, 2024
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
3 changes: 3 additions & 0 deletions .github/workflows/rspec-package-specs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ jobs:
git config user.email "[email protected]"
git config user.name "Your Name"
git commit -am "stop generators from complaining about uncommitted code"
- name: Set packer version environment variable
run: |
echo "CI_PACKER_VERSION=${{ matrix.versions == 'oldest' && 'old' || 'new' }}" >> $GITHUB_ENV
- name: Run rspec tests
run: bundle exec rspec spec/react_on_rails
- name: Store test results
Expand Down
6 changes: 6 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,11 @@ GEM
nokogiri (~> 1.6)
rubyzip (>= 1.3.0)
selenium-webdriver (~> 4.0, < 4.11)
webpacker (6.0.0.rc.6)
activesupport (>= 5.2)
rack-proxy (>= 0.6.1)
railties (>= 5.2)
semantic_range (>= 2.3.0)
webrick (1.8.1)
websocket (1.2.10)
websocket-driver (0.7.6)
Expand Down Expand Up @@ -439,6 +444,7 @@ DEPENDENCIES
turbolinks
uglifier
webdrivers (= 5.3.0)
webpacker (= 6.0.0.rc.6)

BUNDLED WITH
2.5.9
9 changes: 6 additions & 3 deletions lib/react_on_rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def self.configuration
# generated_assets_dirs is deprecated
generated_assets_dir: "",
server_bundle_js_file: "",
rsc_bundle_js_file: "",
prerender: false,
auto_load_bundle: false,
replay_console: true,
Expand Down Expand Up @@ -55,7 +56,7 @@ class Configuration
:server_render_method, :random_dom_id, :auto_load_bundle,
:same_bundle_for_client_and_server, :rendering_props_extension,
:make_generated_server_bundle_the_entrypoint,
:defer_generated_component_packs,
:defer_generated_component_packs, :rsc_bundle_js_file,
:force_load

# rubocop:disable Metrics/AbcSize
Expand All @@ -71,7 +72,8 @@ def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender
same_bundle_for_client_and_server: nil,
i18n_dir: nil, i18n_yml_dir: nil, i18n_output_format: nil,
random_dom_id: nil, server_render_method: nil, rendering_props_extension: nil,
components_subdirectory: nil, auto_load_bundle: nil, force_load: nil)
components_subdirectory: nil, auto_load_bundle: nil, force_load: nil,
rsc_bundle_js_file: nil)
self.node_modules_location = node_modules_location.present? ? node_modules_location : Rails.root
self.generated_assets_dirs = generated_assets_dirs
self.generated_assets_dir = generated_assets_dir
Expand All @@ -97,6 +99,7 @@ def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender

# Server rendering:
self.server_bundle_js_file = server_bundle_js_file
self.rsc_bundle_js_file = rsc_bundle_js_file
self.same_bundle_for_client_and_server = same_bundle_for_client_and_server
self.server_renderer_pool_size = self.development_mode ? 1 : server_renderer_pool_size
self.server_renderer_timeout = server_renderer_timeout # seconds
Expand Down Expand Up @@ -241,7 +244,7 @@ def ensure_webpack_generated_files_exists

files = ["manifest.json"]
files << server_bundle_js_file if server_bundle_js_file.present?

files << rsc_bundle_js_file if rsc_bundle_js_file.present?
self.webpack_generated_files = files
end

Expand Down
66 changes: 45 additions & 21 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,29 +124,15 @@ def react_component(component_name, options = {})
# @option options [Boolean] :raise_on_prerender_error Set to true to raise exceptions during server-side rendering
# Any other options are passed to the content tag, including the id.
def stream_react_component(component_name, options = {})
unless ReactOnRails::Utils.react_on_rails_pro?
raise ReactOnRails::Error,
"You must use React on Rails Pro to use the stream_react_component method."
end

if @rorp_rendering_fibers.nil?
raise ReactOnRails::Error,
"You must call stream_view_containing_react_components to render the view containing the react component"
run_stream_inside_fiber do
internal_stream_react_component(component_name, options)
end
end

rendering_fiber = Fiber.new do
stream = internal_stream_react_component(component_name, options)
stream.each_chunk do |chunk|
Fiber.yield chunk
end
def rsc_react_component(component_name, options = {})
run_stream_inside_fiber do
internal_rsc_react_component(component_name, options)
end

@rorp_rendering_fibers << rendering_fiber

# return the first chunk of the fiber
# It contains the initial html of the component
# all updates will be appended to the stream sent to browser
rendering_fiber.resume
end

# react_component_hash is used to return multiple HTML strings for server rendering, such as for
Expand Down Expand Up @@ -388,6 +374,32 @@ def load_pack_for_generated_component(react_component_name, render_options)

private

def run_stream_inside_fiber
unless ReactOnRails::Utils.react_on_rails_pro?
raise ReactOnRails::Error,
"You must use React on Rails Pro to use the stream_react_component method."
end

if @rorp_rendering_fibers.nil?
raise ReactOnRails::Error,
"You must call stream_view_containing_react_components to render the view containing the react component"
end

rendering_fiber = Fiber.new do
stream = yield
stream.each_chunk do |chunk|
Fiber.yield chunk
end
end

@rorp_rendering_fibers << rendering_fiber

# return the first chunk of the fiber
# It contains the initial html of the component
# all updates will be appended to the stream sent to browser
rendering_fiber.resume
end

def internal_stream_react_component(component_name, options = {})
options = options.merge(stream?: true)
result = internal_react_component(component_name, options)
Expand All @@ -398,6 +410,15 @@ def internal_stream_react_component(component_name, options = {})
)
end

def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
json_stream.transform do |chunk|
chunk[:html].html_safe
end
end
Comment on lines +413 to +420
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and type safety for RSC

The RSC implementation should include:

  1. Type validation for the JSON stream
  2. Error handling for transformation failures
 def internal_rsc_react_component(react_component_name, options = {})
   options = options.merge(rsc?: true)
   render_options = create_render_options(react_component_name, options)
   json_stream = server_rendered_react_component(render_options)
+  raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
   json_stream.transform do |chunk|
+    raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
     chunk[:html].html_safe
   end
+rescue StandardError => e
+  raise ReactOnRails::PrerenderError.new(
+    component_name: react_component_name,
+    err: e,
+    props: options[:props]
+  )
 end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
json_stream.transform do |chunk|
chunk[:html].html_safe
end
end
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
json_stream.transform do |chunk|
raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
chunk[:html].html_safe
end
rescue StandardError => e
raise ReactOnRails::PrerenderError.new(
component_name: react_component_name,
err: e,
props: options[:props]
)
end


def generated_components_pack_path(component_name)
"#{ReactOnRails::PackerUtils.packer_source_entry_path}/generated/#{component_name}.js"
end
Expand Down Expand Up @@ -590,7 +611,7 @@ def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
end

# Returns object with values that are NOT html_safe!
def server_rendered_react_component(render_options)
def server_rendered_react_component(render_options) # rubocop:disable Metrics/CyclomaticComplexity
return { "html" => "", "consoleReplayScript" => "" } unless render_options.prerender

react_component_name = render_options.react_component_name
Expand Down Expand Up @@ -636,6 +657,9 @@ def server_rendered_react_component(render_options)
js_code: js_code)
end

# TODO: handle errors for rsc streams
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be fixed later on the PR that fixes
https://github.com/shakacode/react_on_rails_pro/issues/465

return result if render_options.rsc?

Comment on lines +660 to +662
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

RSC error handling needs implementation following existing patterns

Based on the code analysis, I can see that:

  1. RSC streams are currently bypassing error handling (return result if render_options.rsc?)
  2. Regular streaming has robust error handling using should_raise_streaming_prerender_error? and raise_prerender_error
  3. RSC uses eval_streaming_js like regular streaming but doesn't process the errors

The error handling for RSC streams should be implemented following the existing pattern for regular streams, but with RSC-specific considerations:

  • lib/react_on_rails/helper.rb: Implement error handling before the early return for RSC, similar to the streaming error handling pattern
  • lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb: Consider RSC-specific error parsing in parse_result_and_replay_console_messages
🔗 Analysis chain

Address TODO for RSC error handling.

This TODO is related to the critical issue in internal_rsc_react_component. The error handling implementation should be coordinated between both methods to ensure consistent error management.

Run this script to check for any existing error handling patterns we can follow:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for existing error handling patterns in streaming code
# Check for error handling in streaming methods
rg -A 10 'raise.*PrerenderError'
# Check for error handling in RSC-related code
rg -A 10 'hasErrors.*rsc'

Length of output: 2465


Script:

#!/bin/bash
# Let's check for RSC-related error handling patterns more broadly
# Look for RSC-related error handling in the codebase
rg -B 5 -A 10 'rsc\?' 

# Check for any RSC stream handling
rg -B 5 -A 10 'stream.*rsc|rsc.*stream'

# Look for any error handling in RSC context
ast-grep --pattern 'if $_ {
  $$$
  rsc
  $$$
}'

Length of output: 15507

if render_options.stream?
result.transform do |chunk_json_result|
if should_raise_streaming_prerender_error?(chunk_json_result, render_options)
Expand Down
5 changes: 3 additions & 2 deletions lib/react_on_rails/packer_utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ def self.bundle_js_uri_from_packer(bundle_name)
# the webpack-dev-server is provided by the config value
# "same_bundle_for_client_and_server" where a value of true
# would mean that the bundle is created by the webpack-dev-server
is_server_bundle = bundle_name == ReactOnRails.configuration.server_bundle_js_file
is_bundle_running_on_server = (bundle_name == ReactOnRails.configuration.server_bundle_js_file) ||
(bundle_name == ReactOnRails.configuration.rsc_bundle_js_file)

if packer.dev_server.running? && (!is_server_bundle ||
if packer.dev_server.running? && (!is_bundle_running_on_server ||
ReactOnRails.configuration.same_bundle_for_client_and_server)
"#{packer.dev_server.protocol}://#{packer.dev_server.host_with_port}#{hashed_bundle_name}"
else
Expand Down
4 changes: 4 additions & 0 deletions lib/react_on_rails/react_component/render_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ def stream?
options[:stream?]
end

def rsc?
options[:rsc?]
end

private

attr_reader :options
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def reset_pool_if_server_bundle_was_modified
# Note, js_code does not have to be based on React.
# js_code MUST RETURN json stringify Object
# Calling code will probably call 'html_safe' on return value before rendering to the view.
# rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
def exec_server_render_js(js_code, render_options, js_evaluator = nil)
js_evaluator ||= self
if render_options.trace
Expand All @@ -56,7 +56,7 @@ def exec_server_render_js(js_code, render_options, js_evaluator = nil)
@file_index += 1
end
begin
result = if render_options.stream?
result = if render_options.stream? || render_options.rsc?
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
js_evaluator.eval_streaming_js(js_code, render_options)
else
js_evaluator.eval_js(js_code, render_options)
Expand All @@ -76,13 +76,16 @@ def exec_server_render_js(js_code, render_options, js_evaluator = nil)
raise ReactOnRails::Error, msg, err.backtrace
end

return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream?
unless render_options.stream? || render_options.rsc?
return parse_result_and_replay_console_messages(result,
render_options)
end

# Streamed component is returned as stream of strings.
# We need to parse each chunk and replay the console messages.
result.transform { |chunk| parse_result_and_replay_console_messages(chunk, render_options) }
end
# rubocop:enable Metrics/CyclomaticComplexity
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity

def trace_js_code_used(msg, js_code, file_name = "tmp/server-generated.js", force: false)
return unless ReactOnRails.configuration.trace || force
Expand Down Expand Up @@ -227,6 +230,8 @@ def file_url_to_string(url)
end

def parse_result_and_replay_console_messages(result_string, render_options)
return { html: result_string } if render_options.rsc?

result = nil
begin
result = JSON.parse(result_string)
Expand Down
47 changes: 25 additions & 22 deletions lib/react_on_rails/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def self.server_bundle_path_is_http?
server_bundle_js_file_path =~ %r{https?://}
end

def self.server_bundle_js_file_path
def self.bundle_js_file_path(bundle_name)
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
# Either:
# 1. Using same bundle for both server and client, so server bundle will be hashed in manifest
# 2. Using a different bundle (different Webpack config), so file is not hashed, and
Expand All @@ -76,28 +76,17 @@ def self.server_bundle_js_file_path
# a. The webpack manifest plugin would have a race condition where the same manifest.json
# is edited by both the webpack-dev-server
# b. There is no good reason to hash the server bundle name.
return @server_bundle_path if @server_bundle_path && !Rails.env.development?

bundle_name = ReactOnRails.configuration.server_bundle_js_file
@server_bundle_path = if ReactOnRails::PackerUtils.using_packer?
begin
bundle_js_file_path(bundle_name)
rescue Object.const_get(
ReactOnRails::PackerUtils.packer_type.capitalize
)::Manifest::MissingEntryError
File.expand_path(
File.join(ReactOnRails::PackerUtils.packer_public_output_path,
bundle_name)
)
end
else
bundle_js_file_path(bundle_name)
end
end

def self.bundle_js_file_path(bundle_name)
if ReactOnRails::PackerUtils.using_packer? && bundle_name != "manifest.json"
ReactOnRails::PackerUtils.bundle_js_uri_from_packer(bundle_name)
begin
ReactOnRails::PackerUtils.bundle_js_uri_from_packer(bundle_name)
rescue Object.const_get(
ReactOnRails::PackerUtils.packer_type.capitalize
)::Manifest::MissingEntryError
Comment on lines +82 to +84
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it could easily raise an error itself. Any way around it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added enough specs in utils_spec.rb to ensure that it doesn't cause errors if the packer is webpacker or shaka packer'. For now, React on Rails only supports these packers. If we plan to add support for more packers, we can refactor the PackerUtils` module in a follow-up PR.

Comment on lines +82 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling specificity

The current error handling using Object.const_get is overly broad and could mask other legitimate errors. Consider being more specific about the error type.

-        rescue Object.const_get(
-          ReactOnRails::PackerUtils.packer_type.capitalize
-        )::Manifest::MissingEntryError
+        rescue => e
+          packer_error = "#{ReactOnRails::PackerUtils.packer_type.capitalize}::Manifest::MissingEntryError"
+          raise e unless e.class.name == packer_error

Committable suggestion skipped: line range outside the PR's diff.

File.expand_path(
File.join(ReactOnRails::PackerUtils.packer_public_output_path,
bundle_name)
)
end
else
# Default to the non-hashed name in the specified output directory, which, for legacy
# React on Rails, this is the output directory picked up by the asset pipeline.
Expand All @@ -106,6 +95,20 @@ def self.bundle_js_file_path(bundle_name)
end
end

def self.server_bundle_js_file_path
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
return @server_bundle_path if @server_bundle_path && !Rails.env.development?

bundle_name = ReactOnRails.configuration.server_bundle_js_file
@server_bundle_path = bundle_js_file_path(bundle_name)
end

def self.rsc_bundle_js_file_path
return @rsc_bundle_path if @rsc_bundle_path && !Rails.env.development?

bundle_name = ReactOnRails.configuration.rsc_bundle_js_file
@rsc_bundle_path = bundle_js_file_path(bundle_name)
end

def self.running_on_windows?
(/cygwin|mswin|mingw|bccwin|wince|emx/ =~ RUBY_PLATFORM) != nil
end
Expand Down
10 changes: 9 additions & 1 deletion node_package/src/ReactOnRails.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ReactElement } from 'react';
import type { Readable } from 'stream';
import type { Readable, PassThrough } from 'stream';

import * as ClientStartup from './clientStartup';
import handleError from './handleError';
Expand Down Expand Up @@ -256,6 +256,14 @@ ctx.ReactOnRails = {
return streamServerRenderedReactComponent(options);
},

/**
* Used by rsc payload generation by Rails
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
serverRenderRSCReactComponent(options: RenderParams): PassThrough {
throw new Error('serverRenderRSCReactComponent is supported in RSC bundle only.');
},

/**
* Used by Rails to catch errors in rendering
* @param options
Expand Down
Loading
Loading