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 console replay while streaming component #1647

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
### [Unreleased]
Changes since the last non-beta release.

### Added
- Added support for replaying console logs that occur during server rendering of streamed React components. This enables debugging of server-side rendering issues by capturing and displaying console output on the client and on the server output. [PR #1647](https://github.com/shakacode/react_on_rails/pull/1647) by [AbanoubGhadban](https://github.com/AbanoubGhadban).

### Added
- Added streaming server rendering support:
- New `stream_react_component` helper for adding streamed components to views
Expand Down
38 changes: 19 additions & 19 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -430,30 +430,29 @@ def build_react_component_result_for_server_rendered_string(
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")
rendered_html_stream:,
component_specification_tag:,
render_options:
Comment on lines +433 to +435
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

Ensure required keyword is used for parameter validation and consistency

In the build_react_component_result_for_server_streamed_content method, the parameters are declared without the required method. To ensure that these parameters are not nil and to maintain consistency with other methods in the file, consider using the required helper to validate them.

Apply this diff to enforce parameter validation:

 def build_react_component_result_for_server_streamed_content(
-          rendered_html_stream:,
-          component_specification_tag:,
-          render_options:
+          rendered_html_stream: required("rendered_html_stream"),
+          component_specification_tag: required("component_specification_tag"),
+          render_options: required("render_options")
        )
📝 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
rendered_html_stream:,
component_specification_tag:,
render_options:
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|
rendered_html_stream.transform do |chunk_json_result|
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
build_react_component_result_for_server_rendered_string(
server_rendered_html: chunk_json_result["html"],
component_specification_tag: component_specification_tag,
console_script: chunk_json_result["consoleReplayScript"],
render_options: render_options
)
else
result_console_script = render_options.replay_console ? chunk_json_result["consoleReplayScript"] : ""
# No need to prepend component_specification_tag or add rails context again
# as they're already included in the first chunk
compose_react_component_html_with_spec_and_console(
"", chunk_json_result["html"], result_console_script
)
end
chunk
end
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved

rendered_html_stream.transform(&:html_safe)
# TODO: handle console logs
end

def build_react_component_result_for_server_rendered_hash(
Expand Down Expand Up @@ -492,11 +491,12 @@ def build_react_component_result_for_server_rendered_hash(

def compose_react_component_html_with_spec_and_console(component_specification_tag, rendered_output, console_script)
# IMPORTANT: Ensure that we mark string as html_safe to avoid escaping.
<<~HTML.html_safe
html_content = <<~HTML
#{rendered_output}
#{component_specification_tag}
#{console_script}
HTML
html_content.strip.html_safe
Comment on lines +494 to +499
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

Adjust indentation in HEREDOC to prevent unintended whitespace

The inconsistent indentation inside the HEREDOC may introduce unwanted whitespace in the generated HTML output. To ensure the rendered HTML is formatted correctly, align the content within the HEREDOC.

Apply this diff to correct the indentation:

           html_content = <<~HTML
-            #{rendered_output}
-                  #{component_specification_tag}
-                  #{console_script}
+#{rendered_output}
+#{component_specification_tag}
+#{console_script}
           HTML
📝 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
html_content = <<~HTML
#{rendered_output}
#{component_specification_tag}
#{console_script}
HTML
html_content.strip.html_safe
html_content = <<~HTML
#{rendered_output}
#{component_specification_tag}
#{console_script}
HTML
html_content.strip.html_safe

end

def rails_context_if_not_already_rendered
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, Metrics/AbcSize, Metrics/PerceivedComplexity
# rubocop:disable Metrics/CyclomaticComplexity
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 cyclomatic complexity

Disabling RuboCop's Metrics/CyclomaticComplexity for the exec_server_render_js method suggests that it may have become too complex. Refactoring this method could improve maintainability and readability, allowing you to re-enable the RuboCop check.

def exec_server_render_js(js_code, render_options, js_evaluator = nil)
js_evaluator ||= self
if render_options.trace
Expand All @@ -56,7 +56,11 @@ def exec_server_render_js(js_code, render_options, js_evaluator = nil)
@file_index += 1
end
begin
json_string = js_evaluator.eval_js(js_code, render_options)
result = if render_options.stream?
js_evaluator.eval_streaming_js(js_code, render_options)
else
js_evaluator.eval_js(js_code, render_options)
end
rescue StandardError => err
msg = <<~MSG
Error evaluating server bundle. Check your webpack configuration.
Expand All @@ -71,32 +75,14 @@ def exec_server_render_js(js_code, render_options, js_evaluator = nil)
end
raise ReactOnRails::Error, msg, err.backtrace
end
result = nil
begin
result = JSON.parse(json_string)
rescue JSON::ParserError => e
raise ReactOnRails::JsonParseError.new(parse_error: e, json: json_string)
end

if render_options.logging_on_server
console_script = result["consoleReplayScript"]
console_script_lines = console_script.split("\n")
console_script_lines = console_script_lines[2..-2]
re = /console\.(?:log|error)\.apply\(console, \["\[SERVER\] (?<msg>.*)"\]\);/
console_script_lines&.each do |line|
match = re.match(line)
Rails.logger.info { "[react_on_rails] #{match[:msg]}" } if match
end
end
result
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/AbcSize, Metrics/PerceivedComplexity
return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream?

# 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)
# 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) }
Comment on lines +79 to +83
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

Verify that result responds to transform method

In the streaming case, the code calls result.transform { |chunk| ... }. Ensure that result is an object that responds to the transform method. If transform is not a valid method for result, consider using map or another appropriate enumerable method to process each chunk.

Apply this diff to use map instead of transform:

- result.transform { |chunk| parse_result_and_replay_console_messages(chunk, render_options) }
+ result.map { |chunk| parse_result_and_replay_console_messages(chunk, render_options) }
📝 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
return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream?
# 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)
# 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) }
return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream?
# Streamed component is returned as stream of strings.
# We need to parse each chunk and replay the console messages.
result.map { |chunk| parse_result_and_replay_console_messages(chunk, render_options) }

end
# rubocop:enable Metrics/CyclomaticComplexity

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 @@ -239,6 +225,28 @@ def file_url_to_string(url)
msg = "file_url_to_string #{url} failed\nError is: #{e}"
raise ReactOnRails::Error, msg
end

def parse_result_and_replay_console_messages(result_string, render_options)
result = nil
begin
result = JSON.parse(result_string)
rescue JSON::ParserError => e
raise ReactOnRails::JsonParseError.new(parse_error: e, json: result_string)
end

if render_options.logging_on_server
console_script = result["consoleReplayScript"]
console_script_lines = console_script.split("\n")
# Regular expression to match console.log or console.error calls with SERVER prefix
re = /console\.(?:log|error)\.apply\(console, \["\[SERVER\] (?<msg>.*)"\]\);/
console_script_lines&.each do |line|
match = re.match(line)
# Log matched messages to Rails logger with react_on_rails prefix
Rails.logger.info { "[react_on_rails] #{match[:msg]}" } if match
end
end
result
end
Comment on lines +229 to +249
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

Add nil checks for console_script to prevent potential errors

In the parse_result_and_replay_console_messages method, if result["consoleReplayScript"] is nil, calling split on it will raise a NoMethodError. It's advisable to check if console_script is present before proceeding. Similarly, ensure that console_script_lines is not nil before iterating.

Apply this diff to add the necessary nil checks:

def parse_result_and_replay_console_messages(result_string, render_options)
  result = nil
  begin
    result = JSON.parse(result_string)
  rescue JSON::ParserError => e
    raise ReactOnRails::JsonParseError.new(parse_error: e, json: result_string)
  end

  if render_options.logging_on_server
    console_script = result["consoleReplayScript"]
+   return result unless console_script
    console_script_lines = console_script.split("\n")
+   return result unless console_script_lines
    # Regular expression to match console.log or console.error calls with SERVER prefix
    re = /console\.(?:log|error)\.apply\(console, \["\[SERVER\] (?<msg>.*)"\]\);/
    console_script_lines.each do |line|
      match = re.match(line)
      # Log matched messages to Rails logger with react_on_rails prefix
      Rails.logger.info { "[react_on_rails] #{match[:msg]}" } if match
    end
  end
  result
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 parse_result_and_replay_console_messages(result_string, render_options)
result = nil
begin
result = JSON.parse(result_string)
rescue JSON::ParserError => e
raise ReactOnRails::JsonParseError.new(parse_error: e, json: result_string)
end
if render_options.logging_on_server
console_script = result["consoleReplayScript"]
console_script_lines = console_script.split("\n")
# Regular expression to match console.log or console.error calls with SERVER prefix
re = /console\.(?:log|error)\.apply\(console, \["\[SERVER\] (?<msg>.*)"\]\);/
console_script_lines&.each do |line|
match = re.match(line)
# Log matched messages to Rails logger with react_on_rails prefix
Rails.logger.info { "[react_on_rails] #{match[:msg]}" } if match
end
end
result
end
def parse_result_and_replay_console_messages(result_string, render_options)
result = nil
begin
result = JSON.parse(result_string)
rescue JSON::ParserError => e
raise ReactOnRails::JsonParseError.new(parse_error: e, json: result_string)
end
if render_options.logging_on_server
console_script = result["consoleReplayScript"]
return result unless console_script
console_script_lines = console_script.split("\n")
return result unless console_script_lines
# Regular expression to match console.log or console.error calls with SERVER prefix
re = /console\.(?:log|error)\.apply\(console, \["\[SERVER\] (?<msg>.*)"\]\);/
console_script_lines.each do |line|
match = re.match(line)
# Log matched messages to Rails logger with react_on_rails prefix
Rails.logger.info { "[react_on_rails] #{match[:msg]}" } if match
end
end
result
end

end
# rubocop:enable Metrics/ClassLength
end
Expand Down
8 changes: 4 additions & 4 deletions node_package/src/buildConsoleReplay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ declare global {
}
}

export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined): string {
export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined, numberOfMessagesToSkip: number = 0): string {
// console.history is a global polyfill used in server rendering.
const consoleHistory = customConsoleHistory ?? console.history;

if (!(Array.isArray(consoleHistory))) {
return '';
}

const lines = consoleHistory.map(msg => {
const lines = consoleHistory.slice(numberOfMessagesToSkip).map(msg => {
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

Add validation for numberOfMessagesToSkip parameter.

The slice operation could behave unexpectedly with negative numbers or values larger than the array length. Consider adding validation to ensure robust behavior.

Here's a suggested improvement:

+  if (numberOfMessagesToSkip < 0) {
+    throw new Error('numberOfMessagesToSkip must be non-negative');
+  }
+  if (numberOfMessagesToSkip > consoleHistory.length) {
+    return '';  // or throw error depending on desired behavior
+  }
   const lines = consoleHistory.slice(numberOfMessagesToSkip).map(msg => {
📝 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 lines = consoleHistory.slice(numberOfMessagesToSkip).map(msg => {
if (numberOfMessagesToSkip < 0) {
throw new Error('numberOfMessagesToSkip must be non-negative');
}
if (numberOfMessagesToSkip > consoleHistory.length) {
return ''; // or throw error depending on desired behavior
}
const lines = consoleHistory.slice(numberOfMessagesToSkip).map(msg => {

const stringifiedList = msg.arguments.map(arg => {
let val: string;
try {
Expand All @@ -44,6 +44,6 @@ export function consoleReplay(customConsoleHistory: typeof console['history'] |
return lines.join('\n');
}

export default function buildConsoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined): string {
return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay(customConsoleHistory));
export default function buildConsoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined, numberOfMessagesToSkip: number = 0): string {
return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay(customConsoleHistory, numberOfMessagesToSkip));
}
26 changes: 21 additions & 5 deletions node_package/src/serverRenderReactComponent.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import ReactDOMServer from 'react-dom/server';
import { PassThrough, Readable } from 'stream';
import { PassThrough, Readable, Transform } from 'stream';
import type { ReactElement } from 'react';

import ComponentRegistry from './ComponentRegistry';
Expand Down Expand Up @@ -204,6 +204,7 @@ export const streamServerRenderedReactComponent = (options: RenderParams): Reada
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;

let renderResult: null | Readable = null;
let previouslyReplayedConsoleMessages: number = 0;

try {
const componentObj = ComponentRegistry.get(componentName);
Expand All @@ -221,11 +222,26 @@ export const streamServerRenderedReactComponent = (options: RenderParams): Reada
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;
const consoleHistory = console.history;
const transformStream = new Transform({
transform(chunk, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;

const jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript,
});
Comment on lines +232 to +235
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 security and performance of JSON serialization.

The current implementation might be vulnerable to XSS attacks through console messages and could have performance issues with large chunks.

Consider these improvements:

-const jsonChunk = JSON.stringify({
-  html: htmlChunk,
-  consoleReplayScript,
-});
+const jsonChunk = JSON.stringify({
+  html: htmlChunk,
+  consoleReplayScript: consoleReplayScript.replace(/<|>/g, match => 
+    match === '<' ? '\\u003c' : '\\u003e'
+  ),
+}, null, 0);
📝 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 jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript,
});
const jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript: consoleReplayScript.replace(/<|>/g, match =>
match === '<' ? '\\u003c' : '\\u003e'
),
}, null, 0);


this.push(jsonChunk);
callback();
}
Comment on lines +226 to +239
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

Add error handling and backpressure management to transform stream.

The transform function lacks error handling and proper backpressure management, which could lead to memory issues with large streams or when errors occur during transformation.

Consider this improved implementation:

 const transformStream = new Transform({
-  transform(chunk, _, callback) {
+  async transform(chunk, encoding, callback) {
     try {
-      const htmlChunk = chunk.toString();
+      const htmlChunk = chunk.toString(encoding);
       const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
       previouslyReplayedConsoleMessages = consoleHistory?.length || 0;

-      const jsonChunk = JSON.stringify({
+      const jsonChunk = JSON.stringify({
         html: htmlChunk,
         consoleReplayScript,
-      });
+      }) + '\n';

       this.push(jsonChunk);
       callback();
+    } catch (error) {
+      callback(error);
+    }
   }
 });
📝 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 transformStream = new Transform({
transform(chunk, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
const jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript,
});
this.push(jsonChunk);
callback();
}
const transformStream = new Transform({
async transform(chunk, encoding, callback) {
try {
const htmlChunk = chunk.toString(encoding);
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
const jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript,
}) + '\n';
this.push(jsonChunk);
callback();
} catch (error) {
callback(error);
}
}

});

ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream);

// TODO: Add console replay script to the stream
renderResult = transformStream;
Comment on lines +242 to +244
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

Add error boundary and cleanup for streaming pipeline.

The streaming pipeline should handle errors gracefully and clean up resources properly.

Consider adding error handling:

-ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream);
-renderResult = transformStream;
+const stream = ReactDOMServer.renderToPipeableStream(reactRenderingResult);
+stream.on('error', (error) => {
+  transformStream.destroy(error);
+});
+transformStream.on('error', (error) => {
+  console.error('Error during streaming:', error);
+});
+stream.pipe(transformStream);
+renderResult = transformStream;
📝 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
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream);
// TODO: Add console replay script to the stream
renderResult = transformStream;
const stream = ReactDOMServer.renderToPipeableStream(reactRenderingResult);
stream.on('error', (error) => {
transformStream.destroy(error);
});
transformStream.on('error', (error) => {
console.error('Error during streaming:', error);
});
stream.pipe(transformStream);
renderResult = transformStream;

} catch (e) {
if (throwJsErrors) {
throw e;
Expand Down
Loading