-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Changes from all commits
2e4f94c
05138f2
9366c80
c8b8699
b0f0360
49547bb
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 | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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: | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
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( | ||||||||||||||||||||||||||
|
@@ -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
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. 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
Suggested change
|
||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
def rails_context_if_not_already_rendered | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 Consider refactoring to reduce cyclomatic complexity Disabling RuboCop's |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def exec_server_render_js(js_code, render_options, js_evaluator = nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
js_evaluator ||= self | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if render_options.trace | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
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. Verify that In the streaming case, the code calls Apply this diff to use - 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
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. Add nil checks for In the 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# rubocop:enable Metrics/ClassLength | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 => { | ||||||||||||||||||
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. Add validation for numberOfMessagesToSkip parameter. The 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
Suggested change
|
||||||||||||||||||
const stringifiedList = msg.arguments.map(arg => { | ||||||||||||||||||
let val: string; | ||||||||||||||||||
try { | ||||||||||||||||||
|
@@ -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)); | ||||||||||||||||||
} |
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'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
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. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.push(jsonChunk); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
callback(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+226
to
+239
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. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// TODO: Add console replay script to the stream | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
renderResult = transformStream; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+242
to
+244
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. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} catch (e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (throwJsErrors) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw e; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
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 therequired
method. To ensure that these parameters are notnil
and to maintain consistency with other methods in the file, consider using therequired
helper to validate them.Apply this diff to enforce parameter validation:
📝 Committable suggestion