-
-
Notifications
You must be signed in to change notification settings - Fork 632
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
base: master
Are you sure you want to change the base?
Changes from all commits
1b4f997
b8c63dc
9b63d92
a2094de
5ed4e2b
a8d6d10
eebb139
fc89460
4c3be0e
20fa53a
943a09d
a9f3e1a
222aef8
21ec6b1
40c4050
e22d9ca
78b61d7
4410f29
de99b23
fd24a52
4a36576
e555e97
07e701e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
|
@@ -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 | ||
|
||
def generated_components_pack_path(component_name) | ||
"#{ReactOnRails::PackerUtils.packer_source_entry_path}/generated/#{component_name}.js" | ||
end | ||
|
@@ -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 | ||
|
@@ -636,6 +657,9 @@ def server_rendered_react_component(render_options) | |
js_code: js_code) | ||
end | ||
|
||
# TODO: handle errors for rsc streams | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be fixed later on the PR that fixes |
||
return result if render_options.rsc? | ||
|
||
Comment on lines
+660
to
+662
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification RSC error handling needs implementation following existing patterns Based on the code analysis, I can see that:
The error handling for RSC streams should be implemented following the existing pattern for regular streams, but with RSC-specific considerations:
🔗 Analysis chainAddress TODO for RSC error handling. This TODO is related to the critical issue in Run this script to check for any existing error handling patterns we can follow: 🏁 Scripts executedThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,10 @@ def stream? | |
options[:stream?] | ||
end | ||
|
||
def rsc? | ||
options[:rsc?] | ||
end | ||
|
||
private | ||
|
||
attr_reader :options | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added enough specs in
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve error handling specificity The current error handling using - 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
|
||
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. | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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:
📝 Committable suggestion