-
-
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
Handle errors happen during streaming components #1648
Changes from 8 commits
50bf32f
2ec583e
ad9b93c
2b111e3
cc05983
b2af578
63cc100
e7e3857
3b7728a
276ac40
880f10e
721fed8
f36d74d
1507a9a
8f2e0b3
902e581
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 |
---|---|---|
|
@@ -570,6 +570,21 @@ def props_string(props) | |
props.is_a?(String) ? props : props.to_json | ||
end | ||
|
||
def raise_prerender_error(json_result, react_component_name, props, js_code) | ||
raise ReactOnRails::PrerenderError.new( | ||
component_name: react_component_name, | ||
props: sanitized_props_string(props), | ||
err: nil, | ||
js_code: js_code, | ||
console_messages: json_result["consoleReplayScript"] | ||
) | ||
end | ||
|
||
def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | ||
chunk_json_result["hasErrors"] && | ||
(chunk_json_result["isShellReady"] ? render_options.raise_non_shell_server_rendering_errors : render_options.raise_on_prerender_error) | ||
end | ||
|
||
# Returns object with values that are NOT html_safe! | ||
def server_rendered_react_component(render_options) | ||
return { "html" => "", "consoleReplayScript" => "" } unless render_options.prerender | ||
|
@@ -617,19 +632,18 @@ def server_rendered_react_component(render_options) | |
js_code: js_code) | ||
end | ||
|
||
# TODO: handle errors for streams | ||
return result if render_options.stream? | ||
|
||
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, | ||
# Sanitize as this might be browser logged | ||
props: sanitized_props_string(props), | ||
err: nil, | ||
js_code: js_code, | ||
console_messages: result["consoleReplayScript"]) | ||
|
||
if render_options.stream? | ||
result.transform do |chunk_json_result| | ||
if should_raise_streaming_prerender_error?(chunk_json_result, render_options) | ||
raise_prerender_error(chunk_json_result, react_component_name, props, js_code) | ||
end | ||
# It doesn't make any transformation, it listens and raises error if a chunk has errors | ||
chunk_json_result | ||
end | ||
elsif result["hasErrors"] && render_options.raise_on_prerender_error | ||
Comment on lines
+639
to
+647
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve clarity of streaming error handling. The current implementation uses a Consider this refactor to make the intent clearer: if render_options.stream?
- result.transform do |chunk_json_result|
- if should_raise_streaming_prerender_error?(chunk_json_result, render_options)
- raise_prerender_error(chunk_json_result, react_component_name, props, js_code)
- end
- # It doesn't make any transformation, it listens and raises error if a chunk has errors
- chunk_json_result
- end
+ result.each_chunk do |chunk_json_result|
+ validate_streaming_chunk(
+ chunk_json_result,
+ render_options,
+ react_component_name,
+ props,
+ js_code
+ )
+ chunk_json_result
+ end Add this private method: def validate_streaming_chunk(chunk_json_result, render_options, react_component_name, props, js_code)
if should_raise_streaming_prerender_error?(chunk_json_result, render_options)
raise_prerender_error(chunk_json_result, react_component_name, props, js_code)
end
end This refactor:
|
||
raise_prerender_error(result, react_component_name, props, js_code) | ||
end | ||
|
||
result | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -87,6 +87,10 @@ def raise_on_prerender_error | |||||||||||||||||||||||||||||||||||||||||||||||||||
retrieve_configuration_value_for(:raise_on_prerender_error) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
def raise_non_shell_server_rendering_errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||
retrieve_react_on_rails_pro_config_value_for(:raise_non_shell_server_rendering_errors) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
def logging_on_server | ||||||||||||||||||||||||||||||||||||||||||||||||||||
retrieve_configuration_value_for(:logging_on_server) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -128,6 +132,14 @@ def retrieve_configuration_value_for(key) | |||||||||||||||||||||||||||||||||||||||||||||||||||
ReactOnRails.configuration.public_send(key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
def retrieve_react_on_rails_pro_config_value_for(key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
options.fetch(key) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil unless ReactOnRails::Utils.react_on_rails_pro? | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
ReactOnRailsPro.configuration.public_send(key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+136
to
+142
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 Enhance error handling and add documentation for Pro configuration retrieval. The method silently returns nil for non-Pro installations and might not handle missing configuration keys in ReactOnRailsPro gracefully. Consider:
+ # Retrieves a configuration value from ReactOnRailsPro if available
+ # @param key [Symbol] the configuration key to retrieve
+ # @return [Object, nil] the configuration value or nil if Pro is not enabled
+ # @raise [ReactOnRails::Error] if the configuration key doesn't exist in Pro config
def retrieve_react_on_rails_pro_config_value_for(key)
options.fetch(key) do
- return nil unless ReactOnRails::Utils.react_on_rails_pro?
+ unless ReactOnRails::Utils.react_on_rails_pro?
+ Rails.logger.debug("Attempted to access Pro configuration '#{key}' in non-Pro installation")
+ return nil
+ end
- ReactOnRailsPro.configuration.public_send(key)
+ unless ReactOnRailsPro.configuration.respond_to?(key)
+ raise ReactOnRails::Error, "Missing configuration key '#{key}' in ReactOnRailsPro"
+ end
+
+ ReactOnRailsPro.configuration.public_send(key)
end
end 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
end |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||||
import ReactDOMServer from 'react-dom/server'; | ||||||||
import { PassThrough, Readable, Transform } from 'stream'; | ||||||||
import ReactDOMServer, { type PipeableStream } from 'react-dom/server'; | ||||||||
import { PassThrough, Readable } from 'stream'; | ||||||||
import type { ReactElement } from 'react'; | ||||||||
|
||||||||
import ComponentRegistry from './ComponentRegistry'; | ||||||||
|
@@ -15,6 +15,11 @@ type RenderState = { | |||||||
error?: RenderingError; | ||||||||
}; | ||||||||
|
||||||||
type StreamRenderState = Omit<RenderState, 'result'> & { | ||||||||
result: null | Readable; | ||||||||
isShellReady: boolean; | ||||||||
}; | ||||||||
|
||||||||
type RenderOptions = { | ||||||||
componentName: string; | ||||||||
domNodeId?: string; | ||||||||
|
@@ -95,12 +100,13 @@ function handleRenderingError(e: unknown, options: { componentName: string, thro | |||||||
}; | ||||||||
} | ||||||||
|
||||||||
function createResultObject(html: string | null, consoleReplayScript: string, renderState: RenderState): RenderResult { | ||||||||
function createResultObject(html: string | null, consoleReplayScript: string, renderState: RenderState | StreamRenderState): RenderResult { | ||||||||
return { | ||||||||
html, | ||||||||
consoleReplayScript, | ||||||||
hasErrors: renderState.hasErrors, | ||||||||
renderingError: renderState.error && { message: renderState.error.message, stack: renderState.error.stack }, | ||||||||
isShellReady: 'isShellReady' in renderState ? renderState.isShellReady : undefined, | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
|
@@ -195,17 +201,102 @@ const serverRenderReactComponent: typeof serverRenderReactComponentInternal = (o | |||||||
|
||||||||
const stringToStream = (str: string): Readable => { | ||||||||
const stream = new PassThrough(); | ||||||||
stream.push(str); | ||||||||
stream.push(null); | ||||||||
stream.write(str); | ||||||||
stream.end(); | ||||||||
return stream; | ||||||||
Comment on lines
+208
to
+209
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 Refactor 'stringToStream' using 'Readable.from' You can simplify the Apply this diff: -const stringToStream = (str: string): Readable => {
- const stream = new PassThrough();
- stream.write(str);
- stream.end();
- return stream;
-};
+const stringToStream = (str: string): Readable => Readable.from([str]); 📝 Committable suggestion
Suggested change
|
||||||||
}; | ||||||||
|
||||||||
const transformRenderStreamChunksToResultObject = (renderState: StreamRenderState) => { | ||||||||
const consoleHistory = console.history; | ||||||||
let previouslyReplayedConsoleMessages = 0; | ||||||||
|
||||||||
const transformStream = new PassThrough({ | ||||||||
transform(chunk, _, callback) { | ||||||||
const htmlChunk = chunk.toString(); | ||||||||
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages); | ||||||||
previouslyReplayedConsoleMessages = consoleHistory?.length || 0; | ||||||||
|
||||||||
const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState)); | ||||||||
|
||||||||
this.push(`${jsonChunk}\n`); | ||||||||
callback(); | ||||||||
} | ||||||||
}); | ||||||||
|
||||||||
let pipedStream: PipeableStream | null = null; | ||||||||
const pipeToTransform = (pipeableStream: PipeableStream) => { | ||||||||
pipeableStream.pipe(transformStream); | ||||||||
pipedStream = pipeableStream; | ||||||||
}; | ||||||||
// We need to wrap the transformStream in a Readable stream to properly handle errors: | ||||||||
// 1. If we returned transformStream directly, we couldn't emit errors into it externally | ||||||||
// 2. If an error is emitted into the transformStream, it would cause the render to fail | ||||||||
// 3. By wrapping in Readable.from(), we can explicitly emit errors into the readableStream without affecting the transformStream | ||||||||
// Note: Readable.from can merge multiple chunks into a single chunk, so we need to ensure that we can separate them later | ||||||||
const readableStream = Readable.from(transformStream); | ||||||||
|
||||||||
const writeChunk = (chunk: string) => transformStream.write(chunk); | ||||||||
const emitError = (error: unknown) => readableStream.emit('error', error); | ||||||||
const endStream = () => { | ||||||||
transformStream.end(); | ||||||||
pipedStream?.abort(); | ||||||||
} | ||||||||
return { readableStream: readableStream as Readable, pipeToTransform, writeChunk, emitError, endStream }; | ||||||||
} | ||||||||
AbanoubGhadban marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
const streamRenderReactComponen = (reactRenderingResult: ReactElement, options: RenderParams) => { | ||||||||
const { name: componentName, throwJsErrors } = options; | ||||||||
AbanoubGhadban marked this conversation as resolved.
Show resolved
Hide resolved
AbanoubGhadban marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
const renderState: StreamRenderState = { | ||||||||
result: null, | ||||||||
hasErrors: false, | ||||||||
isShellReady: false | ||||||||
}; | ||||||||
|
||||||||
const { | ||||||||
readableStream, | ||||||||
pipeToTransform, | ||||||||
writeChunk, | ||||||||
emitError, | ||||||||
endStream | ||||||||
} = transformRenderStreamChunksToResultObject(renderState); | ||||||||
|
||||||||
const renderingStream = ReactDOMServer.renderToPipeableStream(reactRenderingResult, { | ||||||||
onShellError(e) { | ||||||||
const error = e instanceof Error ? e : new Error(String(e)); | ||||||||
renderState.hasErrors = true; | ||||||||
renderState.error = error; | ||||||||
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 extracting error conversion logic. The error conversion logic is duplicated. Consider extracting it into a helper function for better maintainability. +const convertToError = (e: unknown): Error =>
+ e instanceof Error ? e : new Error(String(e));
-const error = e instanceof Error ? e : new Error(String(e));
+const error = convertToError(e); Also applies to: 285-286 |
||||||||
|
||||||||
if (throwJsErrors) { | ||||||||
emitError(error); | ||||||||
} | ||||||||
|
||||||||
const errorHtml = handleError({ e: error, name: componentName, serverSide: true }); | ||||||||
writeChunk(errorHtml); | ||||||||
endStream(); | ||||||||
}, | ||||||||
onShellReady() { | ||||||||
renderState.isShellReady = true; | ||||||||
pipeToTransform(renderingStream); | ||||||||
}, | ||||||||
onError(e) { | ||||||||
if (!renderState.isShellReady) { | ||||||||
return; | ||||||||
} | ||||||||
const error = e instanceof Error ? e : new Error(String(e)); | ||||||||
if (throwJsErrors) { | ||||||||
AbanoubGhadban marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
emitError(error); | ||||||||
} | ||||||||
renderState.hasErrors = true; | ||||||||
renderState.error = error; | ||||||||
}, | ||||||||
}); | ||||||||
|
||||||||
return readableStream; | ||||||||
} | ||||||||
|
||||||||
export const streamServerRenderedReactComponent = (options: RenderParams): Readable => { | ||||||||
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options; | ||||||||
|
||||||||
let renderResult: null | Readable = null; | ||||||||
let previouslyReplayedConsoleMessages: number = 0; | ||||||||
|
||||||||
try { | ||||||||
const componentObj = ComponentRegistry.get(componentName); | ||||||||
validateComponent(componentObj, componentName); | ||||||||
|
@@ -222,40 +313,17 @@ export const streamServerRenderedReactComponent = (options: RenderParams): Reada | |||||||
throw new Error('Server rendering of streams is not supported for server render hashes or promises.'); | ||||||||
} | ||||||||
|
||||||||
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, | ||||||||
}); | ||||||||
|
||||||||
this.push(jsonChunk); | ||||||||
callback(); | ||||||||
} | ||||||||
}); | ||||||||
|
||||||||
ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream); | ||||||||
|
||||||||
renderResult = transformStream; | ||||||||
return streamRenderReactComponen(reactRenderingResult, options); | ||||||||
} catch (e) { | ||||||||
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. Update function call to match corrected function name After correcting the function name, update the function call accordingly to maintain consistency. Apply this diff: - return streamRenderReactComponen(reactRenderingResult, options);
+ return streamRenderReactComponent(reactRenderingResult, options); 📝 Committable suggestion
Suggested change
|
||||||||
if (throwJsErrors) { | ||||||||
throw e; | ||||||||
} | ||||||||
|
||||||||
const error = e instanceof Error ? e : new Error(String(e)); | ||||||||
renderResult = stringToStream(handleError({ | ||||||||
e: error, | ||||||||
name: componentName, | ||||||||
serverSide: true, | ||||||||
})); | ||||||||
const htmlResult = handleError({ e: error, name: componentName, serverSide: true }); | ||||||||
const jsonResult = JSON.stringify(createResultObject(htmlResult, buildConsoleReplay(), { hasErrors: true, error, result: null })); | ||||||||
return stringToStream(jsonResult); | ||||||||
} | ||||||||
|
||||||||
return renderResult; | ||||||||
}; | ||||||||
|
||||||||
export default serverRenderReactComponent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve readability of error condition check.
The current implementation, while functional, could be more readable by breaking down the complex conditional logic.
Consider this refactor for better readability:
📝 Committable suggestion
🧰 Tools
🪛 rubocop
[convention] 585-585: Line is too long. [142/120]
(Layout/LineLength)