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
39 changes: 27 additions & 12 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,22 @@ 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"] &&
((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) ||
(render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"]))
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
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

Consider improving readability of error condition checks.

The boolean logic could be more readable by extracting conditions into well-named variables.

Consider this refactor:

-    def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
-      chunk_json_result["hasErrors"] &&
-        ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) ||
-         (render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"]))
-    end
+    def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
+      has_errors = chunk_json_result["hasErrors"]
+      is_shell_ready = chunk_json_result["isShellReady"]
+      
+      raise_non_shell_error = render_options.raise_on_prerender_error && !is_shell_ready
+      raise_shell_error = render_options.raise_non_shell_server_rendering_errors && is_shell_ready
+      
+      has_errors && (raise_non_shell_error || raise_shell_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"] &&
((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) ||
(render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"]))
end
def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
has_errors = chunk_json_result["hasErrors"]
is_shell_ready = chunk_json_result["isShellReady"]
raise_non_shell_error = render_options.raise_on_prerender_error && !is_shell_ready
raise_shell_error = render_options.raise_non_shell_server_rendering_errors && is_shell_ready
has_errors && (raise_non_shell_error || raise_shell_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 +633,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 to the streamed chunks and raise error if it has errors
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
chunk_json_result
end
elsif result["hasErrors"] && render_options.raise_on_prerender_error
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
54 changes: 43 additions & 11 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 { PassThrough, Readable } from 'stream';
import type { ReactElement } from 'react';

import ComponentRegistry from './ComponentRegistry';
Expand Down Expand Up @@ -195,16 +195,19 @@ 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();
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]);

return stream;
};

export const streamServerRenderedReactComponent = (options: RenderParams): Readable => {
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;

let renderResult: null | Readable = null;
let hasErrors = false;
let isShellReady = false;
let previouslyReplayedConsoleMessages: number = 0;
let consoleHistory: typeof console['history'] | undefined;

try {
const componentObj = ComponentRegistry.get(componentName);
Expand All @@ -222,8 +225,8 @@ 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({
consoleHistory = console.history;
const transformStream = new PassThrough({
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

Ensure 'console.history' is available

console.history may not be defined in all environments. To prevent potential runtime errors, consider providing a default value when it's undefined.

Apply this diff:

-        consoleHistory = console.history;
+        consoleHistory = console.history || [];
📝 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
consoleHistory = console.history;
const transformStream = new PassThrough({
consoleHistory = console.history || [];
const transformStream = new PassThrough({

transform(chunk, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
Expand All @@ -232,14 +235,38 @@ export const streamServerRenderedReactComponent = (options: RenderParams): Reada
const jsonChunk = JSON.stringify({
html: htmlChunk,
consoleReplayScript,
hasErrors,
isShellReady,
});

this.push(jsonChunk);
callback();
}
});

ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream);
const renderingStream = ReactDOMServer.renderToPipeableStream(reactRenderingResult, {
onShellError(e) {
// Can't through error here if throwJsErrors is true because the error will happen inside the stream
// And will not be handled by any catch clause
AbanoubGhadban marked this conversation as resolved.
Show resolved Hide resolved
hasErrors = true;
const error = e instanceof Error ? e : new Error(String(e));
transformStream.write(handleError({
e: error,
name: componentName,
serverSide: true,
}));
transformStream.end();
},
onShellReady() {
isShellReady = true;
renderingStream.pipe(transformStream);
},
onError() {
// Can't through error here if throwJsErrors is true because the error will happen inside the stream
// And will not be handled by any catch clause
hasErrors = true;
},
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

Handle errors in 'onError' callback consistently

In the onError callback, the error is currently not being handled or communicated to the client. To ensure consistent error handling, consider processing the error similarly to onShellError.

Apply this diff:

-          onError() {
+          onError(e) {
             // Can't throw an error here if throwJsErrors is true because the error will happen inside the stream
             // and will not be handled by any catch clause
             hasErrors = true;
+            const error = e instanceof Error ? e : new Error(String(e));
+            transformStream.write(handleError({
+              e: error,
+              name: componentName,
+              serverSide: true,
+            }));
+            transformStream.end();
          },

This ensures that errors encountered during rendering are properly relayed to the client.

Committable suggestion was skipped due to low confidence.

});

renderResult = transformStream;
} catch (e) {
Expand All @@ -248,10 +275,15 @@ export const streamServerRenderedReactComponent = (options: RenderParams): Reada
}

const error = e instanceof Error ? e : new Error(String(e));
renderResult = stringToStream(handleError({
e: error,
name: componentName,
serverSide: true,
renderResult = stringToStream(JSON.stringify({
html: handleError({
e: error,
name: componentName,
serverSide: true,
}),
consoleReplayScript: buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages),
hasErrors: true,
isShellReady,
}));
}

Expand Down
Loading