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

Add support for streaming rendered components using renderToPipeableStream #1633

Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
7e4d04b
tmp
AbanoubGhadban Jun 11, 2024
4fc054c
add support for streaming rendered component using renderToPipeableSt…
AbanoubGhadban Jun 17, 2024
4f95d2d
put ROR scripts after the first rendered chunk
AbanoubGhadban Jun 18, 2024
a651535
remove log statements
AbanoubGhadban Jul 23, 2024
337c5c2
add stream_react_component_async helper function
AbanoubGhadban Jun 20, 2024
849a0c5
add helper function to render a whole view
AbanoubGhadban Jul 24, 2024
7ca17d1
fix failing jest tests
AbanoubGhadban Jul 28, 2024
8e1c42d
linting
AbanoubGhadban Jul 28, 2024
66d1cc2
linting
AbanoubGhadban Jul 28, 2024
b086123
remove redundant new line when context is not prepended
AbanoubGhadban Jul 28, 2024
5d29c97
rename stream_react_component_async to stream_react_component
AbanoubGhadban Jul 28, 2024
80f463e
fix error caused by process on browser
AbanoubGhadban Jul 28, 2024
1d4a68e
remove new line appended to the page when has no rails context
AbanoubGhadban Jul 28, 2024
d3a4c64
fix the problem of not updating the first streaming chunk
AbanoubGhadban Jul 30, 2024
5b96a6a
rename stream_react_component_internal to internal_stream_react_compo…
AbanoubGhadban Aug 19, 2024
f1c3dbf
add unit tests for rails_context_if_not_already_rendered
AbanoubGhadban Aug 19, 2024
7cebb2f
remove :focus tag from rails_context_if_not_already_rendered spec
AbanoubGhadban Aug 20, 2024
096231d
make render function returns Readable stream instead of PassThrough
AbanoubGhadban Aug 24, 2024
809322c
use validateComponent function instead of manually validating it
AbanoubGhadban Oct 13, 2024
f6af606
linting
AbanoubGhadban Oct 13, 2024
8937c5f
add some comments
AbanoubGhadban Oct 15, 2024
bf3ef6c
don't return extra null at he end of the stream
AbanoubGhadban Oct 24, 2024
ebf180d
update CHANGELOG.md
AbanoubGhadban Oct 28, 2024
b6d9fd5
update docs
AbanoubGhadban Oct 29, 2024
2d49ce7
update docs
AbanoubGhadban Oct 29, 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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
### [Unreleased]
Changes since the last non-beta release.

### Added
- Added streaming server rendering support:
- New `stream_react_component` helper for adding streamed components to views
- New `streamServerRenderedReactComponent` function in the react-on-rails package that uses React 18's `renderToPipeableStream` API
- Enables progressive page loading and improved performance for server-rendered React components
[PR #1633](https://github.com/shakacode/react_on_rails/pull/1633) by [AbanoubGhadban](https://github.com/AbanoubGhadban).

#### Changed
- Console replay script generation now awaits the render request promise before generating, allowing it to capture console logs from asynchronous operations. This requires using a version of the Node renderer that supports replaying async console logs. [PR #1649](https://github.com/shakacode/react_on_rails/pull/1649) by [AbanoubGhadban](https://github.com/AbanoubGhadban).

Expand Down
1 change: 1 addition & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module.exports = {
preset: 'ts-jest/presets/js-with-ts',
testEnvironment: 'jsdom',
setupFiles: ['<rootDir>/node_package/tests/jest.setup.js'],
};
116 changes: 108 additions & 8 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,64 @@ def react_component(component_name, options = {})
end
end

# Streams a server-side rendered React component using React's `renderToPipeableStream`.
# Supports React 18 features like Suspense, concurrent rendering, and selective hydration.
# Enables progressive rendering and improved performance for large components.
#
# Note: This function can only be used with React on Rails Pro.
# The view that uses this function must be rendered using the
# `stream_view_containing_react_components` method from the React on Rails Pro gem.
#
# Example of an async React component that can benefit from streaming:
#
# const AsyncComponent = async () => {
# const data = await fetchData();
# return <div>{data}</div>;
# };
#
# function App() {
# return (
# <Suspense fallback={<div>Loading...</div>}>
# <AsyncComponent />
# </Suspense>
# );
# }
#
# @param [String] component_name Name of your registered component
# @param [Hash] options Options for rendering
# @option options [Hash] :props Props to pass to the react component
# @option options [String] :dom_id DOM ID of the component container
# @option options [Hash] :html_options Options passed to content_tag
# @option options [Boolean] :prerender Set to false to disable server-side rendering
# @option options [Boolean] :trace Set to true to add extra debugging information to the HTML
# @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"
end

rendering_fiber = Fiber.new do
stream = internal_stream_react_component(component_name, options)
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
Comment on lines +94 to +150
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

Add error handling for Fiber operations.

While the implementation is solid, consider adding error handling for Fiber operations to gracefully handle failures:

 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"
   end

   rendering_fiber = Fiber.new do
+    begin
       stream = internal_stream_react_component(component_name, options)
       stream.each_chunk do |chunk|
         Fiber.yield chunk
       end
+    rescue => e
+      @rorp_rendering_fibers.delete(rendering_fiber)
+      raise ReactOnRails::Error, "Error in streaming component #{component_name}: #{e.message}"
+    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
+  begin
+    rendering_fiber.resume
+  rescue FiberError => e
+    @rorp_rendering_fibers.delete(rendering_fiber)
+    raise ReactOnRails::Error, "Failed to start streaming component #{component_name}: #{e.message}"
+  end
 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
# Streams a server-side rendered React component using React's `renderToPipeableStream`.
# Supports React 18 features like Suspense, concurrent rendering, and selective hydration.
# Enables progressive rendering and improved performance for large components.
#
# Note: This function can only be used with React on Rails Pro.
# The view that uses this function must be rendered using the
# `stream_view_containing_react_components` method from the React on Rails Pro gem.
#
# Example of an async React component that can benefit from streaming:
#
# const AsyncComponent = async () => {
# const data = await fetchData();
# return <div>{data}</div>;
# };
#
# function App() {
# return (
# <Suspense fallback={<div>Loading...</div>}>
# <AsyncComponent />
# </Suspense>
# );
# }
#
# @param [String] component_name Name of your registered component
# @param [Hash] options Options for rendering
# @option options [Hash] :props Props to pass to the react component
# @option options [String] :dom_id DOM ID of the component container
# @option options [Hash] :html_options Options passed to content_tag
# @option options [Boolean] :prerender Set to false to disable server-side rendering
# @option options [Boolean] :trace Set to true to add extra debugging information to the HTML
# @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"
end
rendering_fiber = Fiber.new do
stream = internal_stream_react_component(component_name, options)
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 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"
end
rendering_fiber = Fiber.new do
begin
stream = internal_stream_react_component(component_name, options)
stream.each_chunk do |chunk|
Fiber.yield chunk
end
rescue => e
@rorp_rendering_fibers.delete(rendering_fiber)
raise ReactOnRails::Error, "Error in streaming component #{component_name}: #{e.message}"
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
begin
rendering_fiber.resume
rescue FiberError => e
@rorp_rendering_fibers.delete(rendering_fiber)
raise ReactOnRails::Error, "Failed to start streaming component #{component_name}: #{e.message}"
end
end


# react_component_hash is used to return multiple HTML strings for server rendering, such as for
# adding meta-tags to a page.
# It is exactly like react_component except for the following:
Expand Down Expand Up @@ -330,6 +388,16 @@ def load_pack_for_generated_component(react_component_name, render_options)

private

def internal_stream_react_component(component_name, options = {})
options = options.merge(stream?: true)
result = internal_react_component(component_name, options)
build_react_component_result_for_server_streamed_content(
rendered_html_stream: result[:result],
component_specification_tag: result[:tag],
render_options: result[:render_options]
)
end
Comment on lines +391 to +399
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

Avoid modifying the original options hash

In internal_stream_react_component, you are directly modifying the options hash by merging stream?: true. This might lead to unintended side effects if options is used elsewhere. Consider creating a new hash to prevent mutations to the original options.

Apply this diff to prevent side effects:

 def internal_stream_react_component(component_name, options = {})
-  options = options.merge(stream?: true)
+  stream_options = options.merge(stream?: true)
-  result = internal_react_component(component_name, options)
+  result = internal_react_component(component_name, stream_options)
   build_react_component_result_for_server_streamed_content(
     rendered_html_stream: result[:result],
     component_specification_tag: result[:tag],
     render_options: result[:render_options]
   )
 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_stream_react_component(component_name, options = {})
options = options.merge(stream?: true)
result = internal_react_component(component_name, options)
build_react_component_result_for_server_streamed_content(
rendered_html_stream: result[:result],
component_specification_tag: result[:tag],
render_options: result[:render_options]
)
end
def internal_stream_react_component(component_name, options = {})
stream_options = options.merge(stream?: true)
result = internal_react_component(component_name, stream_options)
build_react_component_result_for_server_streamed_content(
rendered_html_stream: result[:result],
component_specification_tag: result[:tag],
render_options: result[:render_options]
)
end


def generated_components_pack_path(component_name)
"#{ReactOnRails::PackerUtils.packer_source_entry_path}/generated/#{component_name}.js"
end
Expand Down Expand Up @@ -361,6 +429,33 @@ def build_react_component_result_for_server_rendered_string(
prepend_render_rails_context(result)
end

def build_react_component_result_for_server_streamed_content(
rendered_html_stream: required("rendered_html_stream"),
component_specification_tag: required("component_specification_tag"),
render_options: required("render_options")
alexeyr-ci marked this conversation as resolved.
Show resolved Hide resolved
)
content_tag_options_html_tag = render_options.html_options[:tag] || "div"
# The component_specification_tag is appended to the first chunk
# We need to pass it early with the first chunk because it's needed in hydration
# We need to make sure that client can hydrate the app early even before all components are streamed
is_first_chunk = true
rendered_html_stream = rendered_html_stream.transform do |chunk|
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The StreamRequest and StreamDecorator classes allow us to transform the stream of http body chunks

if is_first_chunk
is_first_chunk = false
html_content = <<-HTML
#{rails_context_if_not_already_rendered}
#{component_specification_tag}
<#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}>
HTML
next html_content.strip
end
chunk
end

rendered_html_stream.transform(&:html_safe)
# TODO: handle console logs
end
Comment on lines +456 to +457
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 30, 2024

Choose a reason for hiding this comment

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

Reminder: Handle console logs.

The TODO comment indicates that handling console logs is pending.

Do you want me to help with handling console logs or open a GitHub issue to track this task?

Copy link
Member

Choose a reason for hiding this comment

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

help

Copy link
Contributor

Choose a reason for hiding this comment

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

@justin808, to handle console logs in the build_react_component_result_for_server_streamed_content method, you can follow these steps:

  1. Capture Console Logs: Modify the server-side rendering logic to capture console logs. This can be done by redirecting the console output to a buffer or a similar structure during the rendering process.

  2. Append Logs to Stream: Once the logs are captured, append them to the streamed content. You can do this by transforming the stream to include the logs at the appropriate point, usually after the main HTML content.

  3. Ensure Logs are Safe: Make sure that the logs are properly escaped or sanitized to prevent any security issues, such as XSS, when they are sent to the client.

Here's a conceptual example of how you might implement this:

def build_react_component_result_for_server_streamed_content(
  rendered_html_stream: required("rendered_html_stream"),
  component_specification_tag: required("component_specification_tag"),
  render_options: required("render_options")
)
  content_tag_options_html_tag = render_options.html_options[:tag] || "div"
  is_first_chunk = true
  console_logs = capture_console_logs do
    # Your rendering logic here
  end

  rendered_html_stream = rendered_html_stream.transform do |chunk|
    if is_first_chunk
      is_first_chunk = false
      html_content = <<-HTML
        #{rails_context_if_not_already_rendered}
        #{component_specification_tag}
        <#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}>
        <script>console.log(#{console_logs.to_json});</script>
      HTML
      next html_content.strip
    end
    chunk
  end

  rendered_html_stream.transform(&:html_safe)
end

def capture_console_logs
  # Redirect console output to a buffer
  logs = []
  original_console_log = $stdout
  $stdout = StringIO.new

  yield

  logs << $stdout.string
  $stdout = original_console_log
  logs
end

This is a basic example and may need adjustments based on your specific setup and requirements. Let me know if you need further assistance or if you'd like to open a GitHub issue to track this task.

Comment on lines +432 to +457
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sanitize HTML tag name to prevent XSS.

The content_tag_options_html_tag is used directly in string interpolation which could be vulnerable to XSS if the tag name contains malicious content.

-content_tag_options_html_tag = render_options.html_options[:tag] || "div"
+content_tag_options_html_tag = render_options.html_options[:tag].presence&.to_s&.strip&.downcase || "div"
+unless content_tag_options_html_tag.match?(/\A[a-z0-9]+\z/)
+  raise ReactOnRails::Error, "Invalid HTML tag name: #{content_tag_options_html_tag}"
+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 build_react_component_result_for_server_streamed_content(
rendered_html_stream: required("rendered_html_stream"),
component_specification_tag: required("component_specification_tag"),
render_options: required("render_options")
)
content_tag_options_html_tag = render_options.html_options[:tag] || "div"
# The component_specification_tag is appended to the first chunk
# We need to pass it early with the first chunk because it's needed in hydration
# We need to make sure that client can hydrate the app early even before all components are streamed
is_first_chunk = true
rendered_html_stream = rendered_html_stream.transform do |chunk|
if is_first_chunk
is_first_chunk = false
html_content = <<-HTML
#{rails_context_if_not_already_rendered}
#{component_specification_tag}
<#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}>
HTML
next html_content.strip
end
chunk
end
rendered_html_stream.transform(&:html_safe)
# TODO: handle console logs
end
def build_react_component_result_for_server_streamed_content(
rendered_html_stream: required("rendered_html_stream"),
component_specification_tag: required("component_specification_tag"),
render_options: required("render_options")
)
content_tag_options_html_tag = render_options.html_options[:tag].presence&.to_s&.strip&.downcase || "div"
unless content_tag_options_html_tag.match?(/\A[a-z0-9]+\z/)
raise ReactOnRails::Error, "Invalid HTML tag name: #{content_tag_options_html_tag}"
end
# The component_specification_tag is appended to the first chunk
# We need to pass it early with the first chunk because it's needed in hydration
# We need to make sure that client can hydrate the app early even before all components are streamed
is_first_chunk = true
rendered_html_stream = rendered_html_stream.transform do |chunk|
if is_first_chunk
is_first_chunk = false
html_content = <<-HTML
#{rails_context_if_not_already_rendered}
#{component_specification_tag}
<#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}>
HTML
next html_content.strip
end
chunk
end
rendered_html_stream.transform(&:html_safe)
# TODO: handle console logs
end


def build_react_component_result_for_server_rendered_hash(
server_rendered_html: required("server_rendered_html"),
component_specification_tag: required("component_specification_tag"),
Expand Down Expand Up @@ -404,20 +499,22 @@ def compose_react_component_html_with_spec_and_console(component_specification_t
HTML
end

# prepend the rails_context if not yet applied
def prepend_render_rails_context(render_value)
return render_value if @rendered_rails_context
def rails_context_if_not_already_rendered
return "" if @rendered_rails_context

data = rails_context(server_side: false)

@rendered_rails_context = true

rails_context_content = content_tag(:script,
json_safe_and_pretty(data).html_safe,
type: "application/json",
id: "js-react-on-rails-context")
content_tag(:script,
json_safe_and_pretty(data).html_safe,
type: "application/json",
id: "js-react-on-rails-context")
end
Comment on lines +502 to +513
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

Memoize the Rails context script tag

In rails_context_if_not_already_rendered, you generate the Rails context script tag each time the method is called until @rendered_rails_context is set. To optimize performance, consider memoizing the generated script tag so it's only created once.

Apply this diff to memoize the script tag:

 def rails_context_if_not_already_rendered
   return @rails_context_script if @rendered_rails_context

   data = rails_context(server_side: false)
   @rendered_rails_context = true

-  content_tag(:script,
-              json_safe_and_pretty(data).html_safe,
-              type: "application/json",
-              id: "js-react-on-rails-context")
+  @rails_context_script = content_tag(:script,
+                                      json_safe_and_pretty(data).html_safe,
+                                      type: "application/json",
+                                      id: "js-react-on-rails-context")
+  @rails_context_script
 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 rails_context_if_not_already_rendered
return "" if @rendered_rails_context
data = rails_context(server_side: false)
@rendered_rails_context = true
rails_context_content = content_tag(:script,
json_safe_and_pretty(data).html_safe,
type: "application/json",
id: "js-react-on-rails-context")
content_tag(:script,
json_safe_and_pretty(data).html_safe,
type: "application/json",
id: "js-react-on-rails-context")
end
def rails_context_if_not_already_rendered
return @rails_context_script if @rendered_rails_context
data = rails_context(server_side: false)
@rendered_rails_context = true
@rails_context_script = content_tag(:script,
json_safe_and_pretty(data).html_safe,
type: "application/json",
id: "js-react-on-rails-context")
@rails_context_script
end


"#{rails_context_content}\n#{render_value}".html_safe
# prepend the rails_context if not yet applied
def prepend_render_rails_context(render_value)
"#{rails_context_if_not_already_rendered}\n#{render_value}".strip.html_safe
end

def internal_react_component(react_component_name, options = {})
Expand Down Expand Up @@ -520,6 +617,9 @@ def server_rendered_react_component(render_options)
js_code: js_code)
end

# TODO: handle errors for streams
return result if render_options.stream?
justin808 marked this conversation as resolved.
Show resolved Hide resolved

Comment on lines +620 to +622
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implement error handling for streaming components.

The TODO comment indicates that error handling for streaming components is missing. This could lead to silent failures or unclear error messages in production.

Consider implementing error handling similar to the non-streaming case. Would you like me to help design the error handling mechanism for streaming components?

if result["hasErrors"] && render_options.raise_on_prerender_error
# We caught this exception on our backtrace handler
raise ReactOnRails::PrerenderError.new(component_name: react_component_name,
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 @@ -107,6 +107,10 @@ def set_option(key, value)
options[key] = value
end

def stream?
options[:stream?]
end
Comment on lines +110 to +112
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

Consider using retrieve_configuration_value_for for consistency

The new stream? method is a good addition for checking if streaming is enabled. However, to maintain consistency with other option retrieval methods in this class, consider using the retrieve_configuration_value_for method. This would allow for a default value to be set in the configuration if not provided in the options.

Also, could you clarify why this method is public while some other option retrieval methods are private? If there's no specific reason for it to be public, consider making it private for encapsulation.

Here's a suggested implementation:

private

def stream?
  retrieve_configuration_value_for(:stream?)
end

This assumes you'll add a corresponding stream? option to the ReactOnRails.configuration object. If that's not the case, please disregard this suggestion.


private

attr_reader :options
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ def exec_server_render_js(js_code, render_options, js_evaluator = nil)
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/AbcSize, Metrics/PerceivedComplexity

# TODO: merge with exec_server_render_js
def exec_server_render_streaming_js(js_code, render_options, js_evaluator = nil)
js_evaluator ||= self
js_evaluator.eval_streaming_js(js_code, render_options)
end
Comment on lines +95 to +99
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

Consider refactoring to reduce duplication and improve error handling.

The new exec_server_render_streaming_js method introduces streaming capabilities, which aligns with the PR objectives. However, there are a few points to consider:

  1. As noted in the TODO comment and previous review, consider merging this method with exec_server_render_js to reduce duplication and maintain a single entry point for server rendering.

  2. Unlike exec_server_render_js, this method lacks error handling and tracing. Consider adding these features for consistency and robustness.

  3. The eval_streaming_js method is called on js_evaluator, but it's not clear where this method is defined. Ensure that all js_evaluators implement this method.

Consider refactoring the method as follows:

def exec_server_render_js(js_code, render_options, js_evaluator = nil, streaming: false)
  js_evaluator ||= self
  if render_options.trace
    trace_js_code_used("Evaluating code to server render#{streaming ? ' (streaming)' : ''}.", js_code)
  end
  begin
    result = streaming ? js_evaluator.eval_streaming_js(js_code, render_options) : js_evaluator.eval_js(js_code, render_options)
    process_result(result, render_options) unless streaming
  rescue StandardError => err
    handle_error(err)
  end
end

def exec_server_render_streaming_js(js_code, render_options, js_evaluator = nil)
  exec_server_render_js(js_code, render_options, js_evaluator, streaming: true)
end

This refactoring combines both methods, adds error handling and tracing for streaming, and maintains backwards compatibility.


def trace_js_code_used(msg, js_code, file_name = "tmp/server-generated.js", force: false)
return unless ReactOnRails.configuration.trace || force

Expand Down
11 changes: 10 additions & 1 deletion node_package/src/ReactOnRails.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import type { ReactElement } from 'react';
import type { Readable } from 'stream';

import * as ClientStartup from './clientStartup';
import handleError from './handleError';
import ComponentRegistry from './ComponentRegistry';
import StoreRegistry from './StoreRegistry';
import serverRenderReactComponent from './serverRenderReactComponent';
import serverRenderReactComponent, { streamServerRenderedReactComponent } from './serverRenderReactComponent';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix the recursion issue by renaming the imported function

The method streamServerRenderedReactComponent is recursively calling itself instead of the intended imported function due to a naming conflict. This will result in a stack overflow error.

To resolve this, you can rename the imported function to avoid the naming conflict.

Apply this diff to fix the problem:

 import serverRenderReactComponent, {
-  streamServerRenderedReactComponent
+  streamServerRenderedReactComponent as streamServerRenderedReactComponentFn
 } from './serverRenderReactComponent';

 ...

 streamServerRenderedReactComponent(options: RenderParams): Readable {
-  return streamServerRenderedReactComponent(options);
+  return streamServerRenderedReactComponentFn(options);
 },

Also applies to: 255-257

import buildConsoleReplay from './buildConsoleReplay';
import createReactOutput from './createReactOutput';
import Authenticity from './Authenticity';
Expand Down Expand Up @@ -247,6 +248,14 @@ ctx.ReactOnRails = {
return serverRenderReactComponent(options);
},

/**
* Used by server rendering by Rails
* @param options
*/
streamServerRenderedReactComponent(options: RenderParams): Readable {
return streamServerRenderedReactComponent(options);
},
Comment on lines +251 to +257
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential recursive call in streamServerRenderedReactComponent.

The implementation of streamServerRenderedReactComponent appears to be calling itself recursively, which could lead to a stack overflow error. This issue was previously flagged but hasn't been addressed.

To resolve this, you should call the imported function instead of the method itself:

streamServerRenderedReactComponent(options: RenderParams): Readable {
-  return streamServerRenderedReactComponent(options);
+  return streamServerRenderedReactComponent(options);
}

This change ensures that the imported function is called rather than recursively calling the method itself.

Committable suggestion was skipped due to low confidence.


/**
* Used by Rails to catch errors in rendering
* @param options
Expand Down
50 changes: 50 additions & 0 deletions node_package/src/serverRenderReactComponent.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import ReactDOMServer from 'react-dom/server';
import { PassThrough, Readable } from 'stream';
import type { ReactElement } from 'react';

import ComponentRegistry from './ComponentRegistry';
Expand Down Expand Up @@ -192,4 +193,53 @@ const serverRenderReactComponent: typeof serverRenderReactComponentInternal = (o
}
};

const stringToStream = (str: string): Readable => {
const stream = new PassThrough();
stream.push(str);
stream.push(null);
return stream;
};
Comment on lines +196 to +201
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

Consider using Readable.from for efficient stream creation

The stringToStream function can be simplified by using Readable.from instead of manually pushing data into a PassThrough stream.

Here's how you can refactor the function:

-const stringToStream = (str: string): Readable => {
-  const stream = new PassThrough();
-  stream.push(str);
-  stream.push(null);
-  return stream;
-};
+const stringToStream = (str: string): Readable => {
+  return Readable.from([str]);
+};

This approach is more concise and leverages the built-in method for creating a stream from an iterable.

📝 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
const stringToStream = (str: string): Readable => {
const stream = new PassThrough();
stream.push(str);
stream.push(null);
return stream;
};
const stringToStream = (str: string): Readable => {
return Readable.from([str]);
};


export const streamServerRenderedReactComponent = (options: RenderParams): Readable => {
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;

let renderResult: null | Readable = null;

try {
const componentObj = ComponentRegistry.get(componentName);
validateComponent(componentObj, componentName);

const reactRenderingResult = createReactOutput({
componentObj,
domNodeId,
trace,
props,
railsContext,
});

if (isServerRenderHash(reactRenderingResult) || isPromise(reactRenderingResult)) {
throw new Error('Server rendering of streams is not supported for server render hashes or promises.');
}

const renderStream = new PassThrough();
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(renderStream);
renderResult = renderStream;

Comment on lines +224 to +227
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle renderToPipeableStream events to ensure correct streaming behavior

Currently, ReactDOMServer.renderToPipeableStream is used without handling its lifecycle events. This might lead to issues where the stream starts piping before it's ready or errors are not properly managed.

Consider updating the implementation to handle events like onShellReady, onShellError, and onError:

 const renderStream = new PassThrough();
-ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(renderStream);
+const stream = ReactDOMServer.renderToPipeableStream(reactRenderingResult, {
+  onShellReady() {
+    stream.pipe(renderStream);
+  },
+  onShellError(error) {
+    renderStream.emit('error', error);
+  },
+  onError(error) {
+    // Optional: Handle individual chunk errors if needed
+    console.error(`Error during streaming: ${error.message}`);
+  },
+});

This ensures that the stream begins piping only when the content is ready and errors are properly propagated.

📝 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
const renderStream = new PassThrough();
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(renderStream);
renderResult = renderStream;
const renderStream = new PassThrough();
const stream = ReactDOMServer.renderToPipeableStream(reactRenderingResult, {
onShellReady() {
stream.pipe(renderStream);
},
onShellError(error) {
renderStream.emit('error', error);
},
onError(error) {
// Optional: Handle individual chunk errors if needed
console.error(`Error during streaming: ${error.message}`);
},
});
renderResult = renderStream;

// TODO: Add console replay script to the stream
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implement the console replay script in the stream

The TODO comment indicates that the console replay script is not yet added to the stream. Including the console replay script ensures that console messages are properly replayed on the client side.

Would you like assistance in implementing the console replay script within the streamServerRenderedReactComponent function?

} catch (e) {
if (throwJsErrors) {
throw e;
}

const error = e instanceof Error ? e : new Error(String(e));
renderResult = stringToStream(handleError({
e: error,
name: componentName,
serverSide: true,
}));
}

return renderResult;
Comment on lines +203 to +242
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance streaming implementation with event handling

The streamServerRenderedReactComponent function currently uses renderToPipeableStream without handling its lifecycle events. This might lead to premature piping or unhandled errors.

Consider updating the implementation to manage events like onShellReady, onShellError, and onError to ensure correct streaming behavior:

const renderStream = new PassThrough();
-ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(renderStream);
+const { pipe } = ReactDOMServer.renderToPipeableStream(reactRenderingResult, {
+  onShellReady() {
+    pipe(renderStream);
+  },
+  onShellError(error) {
+    renderStream.emit('error', error);
+  },
+  onError(error) {
+    console.error(`Error during streaming: ${error.message}`);
+  },
+});
renderResult = renderStream;

This ensures that piping begins only when the content is ready and that errors are properly propagated.

📝 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
export const streamServerRenderedReactComponent = (options: RenderParams): Readable => {
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;
let renderResult: null | Readable = null;
try {
const componentObj = ComponentRegistry.get(componentName);
validateComponent(componentObj, componentName);
const reactRenderingResult = createReactOutput({
componentObj,
domNodeId,
trace,
props,
railsContext,
});
if (isServerRenderHash(reactRenderingResult) || isPromise(reactRenderingResult)) {
throw new Error('Server rendering of streams is not supported for server render hashes or promises.');
}
const renderStream = new PassThrough();
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(renderStream);
renderResult = renderStream;
// TODO: Add console replay script to the stream
} catch (e) {
if (throwJsErrors) {
throw e;
}
const error = e instanceof Error ? e : new Error(String(e));
renderResult = stringToStream(handleError({
e: error,
name: componentName,
serverSide: true,
}));
}
return renderResult;
export const streamServerRenderedReactComponent = (options: RenderParams): Readable => {
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;
let renderResult: null | Readable = null;
try {
const componentObj = ComponentRegistry.get(componentName);
validateComponent(componentObj, componentName);
const reactRenderingResult = createReactOutput({
componentObj,
domNodeId,
trace,
props,
railsContext,
});
if (isServerRenderHash(reactRenderingResult) || isPromise(reactRenderingResult)) {
throw new Error('Server rendering of streams is not supported for server render hashes or promises.');
}
const renderStream = new PassThrough();
const { pipe } = ReactDOMServer.renderToPipeableStream(reactRenderingResult, {
onShellReady() {
pipe(renderStream);
},
onShellError(error) {
renderStream.emit('error', error);
},
onError(error) {
console.error(`Error during streaming: ${error.message}`);
},
});
renderResult = renderStream;
// TODO: Add console replay script to the stream
} catch (e) {
if (throwJsErrors) {
throw e;
}
const error = e instanceof Error ? e : new Error(String(e));
renderResult = stringToStream(handleError({
e: error,
name: componentName,
serverSide: true,
}));
}
return renderResult;

};

export default serverRenderReactComponent;
2 changes: 2 additions & 0 deletions node_package/src/types/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { ReactElement, ReactNode, Component, ComponentType } from 'react';
import type { Readable } from 'stream';

// Don't import redux just for the type definitions
// See https://github.com/shakacode/react_on_rails/issues/1321
Expand Down Expand Up @@ -168,6 +169,7 @@ export interface ReactOnRails {
): RenderReturnType;
getComponent(name: string): RegisteredComponent;
serverRenderReactComponent(options: RenderParams): null | string | Promise<RenderResult>;
streamServerRenderedReactComponent(options: RenderParams): Readable;
handleError(options: ErrorOptions): string | undefined;
buildConsoleReplay(): string;
registeredComponents(): Map<string, RegisteredComponent>;
Expand Down
13 changes: 9 additions & 4 deletions node_package/tests/ReactOnRails.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ describe('ReactOnRails', () => {
});
ReactOnRails.register({ R1 });

document.body.innerHTML = '<div id="root"></div>';
// eslint-disable-next-line no-underscore-dangle
const actual = ReactOnRails.render('R1', {}, 'root')._reactInternals.type;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we use a newer react version that supports the Root API, it returns the ReactDomRoot object, which doesn't have the _reactInternals property.

expect(actual).toEqual(R1);
const root = document.createElement('div');
root.id = 'root';
root.textContent = ' WORLD ';

document.body.innerHTML = '';
document.body.appendChild(root);
ReactOnRails.render('R1', {}, 'root');

expect(document.getElementById('root').textContent).toEqual(' WORLD ');
});

it('accepts traceTurbolinks as an option true', () => {
Expand Down
13 changes: 13 additions & 0 deletions node_package/tests/jest.setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// If jsdom environment is set and TextEncoder is not defined, then define TextEncoder and TextDecoder
// The current version of jsdom does not support TextEncoder and TextDecoder
// The following code will tell us when jsdom supports TextEncoder and TextDecoder
if (typeof window !== 'undefined' && typeof window.TextEncoder !== 'undefined') {
throw new Error('TextEncoder is already defined, remove the polyfill');
}

if (typeof window !== 'undefined') {
// eslint-disable-next-line global-require
const { TextEncoder, TextDecoder } = require('util');
global.TextEncoder = TextEncoder;
global.TextDecoder = TextDecoder;
}
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
"@babel/preset-react": "^7.18.6",
"@babel/types": "^7.20.7",
"@types/jest": "^29.0.0",
"@types/react": "^17.0.0",
"@types/react-dom": "^17.0.0",
"@types/react": "^18.2.0",
"@types/react-dom": "^18.2.0",
"@types/turbolinks": "^5.2.2",
"@types/webpack-env": "^1.18.4",
"@typescript-eslint/eslint-plugin": "^6.18.1",
Expand All @@ -39,8 +39,8 @@
"prettier": "^2.8.8",
"prettier-eslint-cli": "^5.0.0",
"prop-types": "^15.8.1",
"react": "^17.0.0",
"react-dom": "^17.0.0",
"react": "^18.2.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renderToReadableStream function is added in React 18

"react-dom": "^18.2.0",
"react-transform-hmr": "^1.0.4",
"redux": "^4.2.1",
"ts-jest": "^29.1.0",
Expand Down
1 change: 1 addition & 0 deletions spec/dummy/config/webpack/alias.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module.exports = {
resolve: {
alias: {
Assets: resolve(__dirname, '..', '..', 'client', 'app', 'assets'),
stream: 'stream-browserify',
},
},
};
1 change: 1 addition & 0 deletions spec/dummy/config/webpack/commonWebpackConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ baseClientWebpackConfig.plugins.push(
new webpack.ProvidePlugin({
$: 'jquery',
jQuery: 'jquery',
process: 'process/browser',
}),
);

Expand Down
1 change: 1 addition & 0 deletions spec/dummy/config/webpack/webpackConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const serverWebpackConfig = require('./serverWebpackConfig');
const webpackConfig = (envSpecific) => {
const clientConfig = clientWebpackConfig();
const serverConfig = serverWebpackConfig();
clientConfig.resolve.fallback = { stream: require.resolve('stream-browserify') };

if (envSpecific) {
envSpecific(clientConfig, serverConfig);
Expand Down
Loading
Loading