-
-
Notifications
You must be signed in to change notification settings - Fork 629
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 all 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,25 @@ 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"] && | ||||||||||||||||||||||||||||||
(if chunk_json_result["isShellReady"] | ||||||||||||||||||||||||||||||
render_options.raise_non_shell_server_rendering_errors | ||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||
render_options.raise_on_prerender_error | ||||||||||||||||||||||||||||||
end) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
Comment on lines
+583
to
+590
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 Simplify the conditional logic for better readability. The current implementation uses a nested if statement that could be simplified using the ternary operator. - def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
- chunk_json_result["hasErrors"] &&
- (if chunk_json_result["isShellReady"]
- render_options.raise_non_shell_server_rendering_errors
- else
- render_options.raise_on_prerender_error
- end)
- 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 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# 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 +636,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,13 +15,22 @@ type RenderState = { | |||||||
error?: RenderingError; | ||||||||
}; | ||||||||
|
||||||||
type StreamRenderState = Omit<RenderState, 'result'> & { | ||||||||
result: null | Readable; | ||||||||
isShellReady: boolean; | ||||||||
}; | ||||||||
|
||||||||
type RenderOptions = { | ||||||||
componentName: string; | ||||||||
domNodeId?: string; | ||||||||
trace?: boolean; | ||||||||
renderingReturnsPromises: boolean; | ||||||||
}; | ||||||||
|
||||||||
function convertToError(e: unknown): Error { | ||||||||
return e instanceof Error ? e : new Error(String(e)); | ||||||||
} | ||||||||
|
||||||||
function validateComponent(componentObj: RegisteredComponent, componentName: string) { | ||||||||
if (componentObj.isRenderer) { | ||||||||
throw new Error(`Detected a renderer while server rendering component '${componentName}'. See https://github.com/shakacode/react_on_rails#renderer-functions`); | ||||||||
|
@@ -87,20 +96,21 @@ function handleRenderingError(e: unknown, options: { componentName: string, thro | |||||||
if (options.throwJsErrors) { | ||||||||
throw e; | ||||||||
} | ||||||||
const error = e instanceof Error ? e : new Error(String(e)); | ||||||||
const error = convertToError(e); | ||||||||
return { | ||||||||
hasErrors: true, | ||||||||
result: handleError({ e: error, name: options.componentName, serverSide: true }), | ||||||||
error, | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
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 +205,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, pipeToTransform, writeChunk, emitError, endStream }; | ||||||||
} | ||||||||
|
||||||||
const streamRenderReactComponent = (reactRenderingResult: ReactElement, options: RenderParams) => { | ||||||||
const { name: componentName, throwJsErrors } = options; | ||||||||
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. Export streamRenderReactComponent function. The function is used internally but not exported, causing an unused variable warning. Consider exporting it or marking it as internal. -const streamRenderReactComponent = (reactRenderingResult: ReactElement, options: RenderParams) => {
+export const streamRenderReactComponent = (reactRenderingResult: ReactElement, options: RenderParams) => { 📝 Committable suggestion
Suggested change
🧰 Tools🪛 eslint[error] 247-247: 'streamRenderReactComponent' is assigned a value but never used. (@typescript-eslint/no-unused-vars) |
||||||||
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 = convertToError(e); | ||||||||
renderState.hasErrors = true; | ||||||||
renderState.error = error; | ||||||||
|
||||||||
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 = convertToError(e); | ||||||||
if (throwJsErrors) { | ||||||||
emitError(error); | ||||||||
} | ||||||||
renderState.hasErrors = true; | ||||||||
renderState.error = error; | ||||||||
}, | ||||||||
}); | ||||||||
Comment on lines
+285
to
+295
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. Consider enhancing error handling in onError callback The Consider updating the implementation: onError(e) {
if (!renderState.isShellReady) {
return;
}
const error = convertToError(e);
if (throwJsErrors) {
emitError(error);
}
renderState.hasErrors = true;
renderState.error = error;
+ if (!throwJsErrors) {
+ const errorHtml = handleError({ e: error, name: componentName, serverSide: true });
+ writeChunk(errorHtml);
+ }
},
|
||||||||
|
||||||||
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 +317,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 streamRenderReactComponent(reactRenderingResult, options); | ||||||||
} catch (e) { | ||||||||
if (throwJsErrors) { | ||||||||
throw e; | ||||||||
} | ||||||||
|
||||||||
const error = e instanceof Error ? e : new Error(String(e)); | ||||||||
renderResult = stringToStream(handleError({ | ||||||||
e: error, | ||||||||
name: componentName, | ||||||||
serverSide: true, | ||||||||
})); | ||||||||
const error = convertToError(e); | ||||||||
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.
Fix formatting of the Added section header
The section header has incorrect formatting:
Apply this diff to fix the formatting:
📝 Committable suggestion
🧰 Tools
🪛 Markdownlint
21-21: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
21-21: null
Bare URL used
(MD034, no-bare-urls)