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

Handle errors happen during streaming components #1648

Merged
Merged
Show file tree
Hide file tree
Changes from 13 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 handling errors happening during server rendering of streamed React components. It handles errors that happen during the initial render and errors that happen inside suspense boundaries. [PR #1648](https://github.com/shakacode/react_on_rails/pull/1648) by [AbanoubGhadban](https://github.com/AbanoubGhadban).

### 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).

Expand Down
42 changes: 30 additions & 12 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
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

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

‼️ 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 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


# Returns object with values that are NOT html_safe!
def server_rendered_react_component(render_options)
return { "html" => "", "consoleReplayScript" => "" } unless render_options.prerender
Expand Down Expand Up @@ -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
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

Improve clarity of streaming error handling.

The current implementation uses a transform block for error checking, which might be misleading since no actual transformation occurs.

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:

  1. Uses a more appropriate method name (each_chunk instead of transform)
  2. Extracts validation logic into a dedicated method
  3. Makes the error checking purpose more explicit

raise_prerender_error(result, react_component_name, props, js_code)
end

result
end

Expand Down
12 changes: 12 additions & 0 deletions lib/react_on_rails/react_component/render_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
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

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:

  1. Adding documentation to clarify the behavior
  2. Adding error handling for missing Pro configuration keys
  3. Consider logging a warning when accessed in non-Pro installations
+  # 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

‼️ 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 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
# 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
unless ReactOnRails::Utils.react_on_rails_pro?
Rails.logger.debug("Attempted to access Pro configuration '#{key}' in non-Pro installation")
return nil
end
unless ReactOnRailsPro.configuration.respond_to?(key)
raise ReactOnRails::Error, "Missing configuration key '#{key}' in ReactOnRailsPro"
end
ReactOnRailsPro.configuration.public_send(key)
end
end

end
end
end
146 changes: 109 additions & 37 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, 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';
Expand All @@ -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`);
Expand Down Expand Up @@ -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,
};
}

Expand Down Expand Up @@ -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
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

Refactor 'stringToStream' using 'Readable.from'

You can simplify the stringToStream function by utilizing Readable.from, which is available in Node.js v12 and above. This approach makes the code more concise and leverages built-in stream utilities.

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

‼️ 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
stream.write(str);
stream.end();
const stringToStream = (str: string): Readable => Readable.from([str]);

};

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;
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

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

‼️ 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 streamRenderReactComponent = (reactRenderingResult: ReactElement, options: RenderParams) => {
export const streamRenderReactComponent = (reactRenderingResult: ReactElement, options: RenderParams) => {
🧰 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
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

Consider enhancing error handling in onError callback

The onError callback could be enhanced to handle errors more consistently with onShellError. Currently, it updates the state but doesn't write the error to the stream when throwJsErrors is false.

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);
+  }
 },

Committable suggestion was skipped due to low confidence.


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);
Expand All @@ -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;
1 change: 1 addition & 0 deletions node_package/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export interface RenderResult {
consoleReplayScript: string;
hasErrors: boolean;
renderingError?: RenderingError;
isShellReady?: boolean;
}

// from react-dom 18
Expand Down
Loading
Loading