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

-    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
+    def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
+      return false unless 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
📝 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"] &&
(chunk_json_result["isShellReady"] ? render_options.raise_non_shell_server_rendering_errors : render_options.raise_on_prerender_error)
end
def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
return false unless 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
🧰 Tools
🪛 rubocop

[convention] 585-585: Line is too long. [142/120]

(Layout/LineLength)


# 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 +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
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
138 changes: 103 additions & 35 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,6 +15,11 @@ type RenderState = {
error?: RenderingError;
};

type StreamRenderState = Omit<RenderState, 'result'> & {
result: null | Readable;
isShellReady: boolean;
};

type RenderOptions = {
componentName: string;
domNodeId?: string;
Expand Down Expand Up @@ -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,
};
}

Expand Down Expand Up @@ -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
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: 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider 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);
Expand All @@ -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) {
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

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return streamRenderReactComponen(reactRenderingResult, options);
return streamRenderReactComponent(reactRenderingResult, options);

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