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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
add support for streaming rendered component using renderToPipeableSt…
…ream
AbanoubGhadban committed Oct 20, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 4fc054cde8d857723266987755686dbf11ebc9e4
62 changes: 54 additions & 8 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
@@ -91,6 +91,16 @@ def react_component(component_name, options = {})
end
end

def stream_react_component(component_name, options = {})
options = options.merge(stream?: true)
result = internal_react_component(component_name, options)
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 result is of type StreamRequest

build_react_component_result_for_server_streamed_content(
rendered_html_stream: result[:result],
component_specification_tag: result[:tag],
render_options: result[:render_options]
)
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:
@@ -334,6 +344,10 @@ def generated_components_pack_path(component_name)
"#{ReactOnRails::PackerUtils.packer_source_entry_path}/generated/#{component_name}.js"
end

def get_content_tag_options_html_tag(render_options)

end

def build_react_component_result_for_server_rendered_string(
server_rendered_html: required("server_rendered_html"),
component_specification_tag: required("component_specification_tag"),
@@ -361,6 +375,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.prepend { rails_context_if_not_already_rendered }
.prepend { "<#{content_tag_options_html_tag} id=\"#{render_options.dom_id}\">" }
.transform(&:html_safe)

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

is_first_chunk = false
if is_first_chunk
return "#{chunk}\n#{component_specification_tag}"
end
chunk
end
.append { "</#{content_tag_options_html_tag}>" }
.append { component_specification_tag }
# 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.


def build_react_component_result_for_server_rendered_hash(
server_rendered_html: required("server_rendered_html"),
component_specification_tag: required("component_specification_tag"),
@@ -404,20 +445,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}".html_safe
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
end

def internal_react_component(react_component_name, options = {})
@@ -520,6 +563,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,
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
@@ -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
Original file line number Diff line number Diff line change
@@ -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