Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions lib/react_on_rails/server_rendering_js_code.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def render(props_string, rails_context, redux_stores, react_component_name, rend
var railsContext = #{rails_context};
#{redux_stores}
var props = #{props_string};
console.history = [];
return ReactOnRails.serverRenderReactComponent({
name: '#{react_component_name}',
domNodeId: '#{render_options.dom_id}',
Expand Down
12 changes: 7 additions & 5 deletions node_package/src/buildConsoleReplay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ declare global {
}
}

export function consoleReplay(): string {
export function consoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined): string {
// console.history is a global polyfill used in server rendering.
if (!(console.history instanceof Array)) {
const consoleHistory = customConsoleHistory ?? console.history;

if (!(Array.isArray(consoleHistory))) {
return '';
}

const lines = console.history.map(msg => {
const lines = consoleHistory.map(msg => {
const stringifiedList = msg.arguments.map(arg => {
let val: string;
try {
Expand All @@ -42,6 +44,6 @@ export function consoleReplay(): string {
return lines.join('\n');
}

export default function buildConsoleReplay(): string {
return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay());
export default function buildConsoleReplay(customConsoleHistory: typeof console['history'] | undefined = undefined): string {
return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay(customConsoleHistory));
}
34 changes: 11 additions & 23 deletions node_package/src/serverRenderReactComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,22 @@ function createResultObject(html: string | null, consoleReplayScript: string, re

async function createPromiseResult(
renderState: RenderState & { result: Promise<string> },
consoleReplayScript: string,
componentName: string,
throwJsErrors: boolean
): Promise<RenderResult> {
// Capture console history before awaiting the promise
// Node renderer will reset the global console.history after executing the synchronous part of the request.
// It resets it only if replayServerAsyncOperationLogs renderer config is set to false.
// In both cases, we need to keep a reference to console.history to avoid losing console logs in case of reset.
const consoleHistory = console.history;
try {
const html = await renderState.result;
const consoleReplayScript = buildConsoleReplay(consoleHistory);
return createResultObject(html, consoleReplayScript, renderState);
} catch (e: unknown) {
const errorRenderState = handleRenderingError(e, { componentName, throwJsErrors });
return createResultObject(errorRenderState.result, consoleReplayScript, errorRenderState);
const consoleReplayScript = buildConsoleReplay(consoleHistory);
return createResultObject(errorRenderState.result, consoleReplayScript, renderState);
}
}

Expand All @@ -123,24 +129,16 @@ function createFinalResult(
componentName: string,
throwJsErrors: boolean
): null | string | Promise<RenderResult> {
// Console history is stored globally in `console.history`.
// If node renderer is handling a render request that returns a promise,
// It can handle another request while awaiting the promise.
// To prevent cross-request console logs leakage between these requests,
// we build the consoleReplayScript before awaiting any promises.
// The console history is reset after the synchronous part of each request.
// This causes console logs happening during async operations to not be captured.
const consoleReplayScript = buildConsoleReplay();

const { result } = renderState;
if (isPromise(result)) {
return createPromiseResult({ ...renderState, result }, consoleReplayScript, componentName, throwJsErrors);
return createPromiseResult({ ...renderState, result }, componentName, throwJsErrors);
}

const consoleReplayScript = buildConsoleReplay();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent console log handling in non-promise scenarios.

The buildConsoleReplay call here doesn't use the captured console history, unlike in the createPromiseResult function. This inconsistency may lead to loss of console logs in non-promise scenarios.

Consider updating this line to use the captured console history:

- const consoleReplayScript = buildConsoleReplay();
+ const consoleReplayScript = buildConsoleReplay(console.history);

This change would ensure consistent handling of console logs across all scenarios.

📝 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 consoleReplayScript = buildConsoleReplay();
const consoleReplayScript = buildConsoleReplay(console.history);

return JSON.stringify(createResultObject(result, consoleReplayScript, renderState));
}
Comment on lines +134 to 139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent console history handling in createFinalResult

The use of the spread operator in the createPromiseResult call is a good practice for maintaining immutability. However, there's an inconsistency in how the console history is handled for non-promise results.

For consistency with the promise case and to ensure all console logs are captured, consider modifying the buildConsoleReplay call for non-promise results:

-  const consoleReplayScript = buildConsoleReplay();
+  const consoleReplayScript = buildConsoleReplay(console.history);

This change ensures that all console logs are included in the replay script, regardless of whether the result is a promise or not.

📝 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 createPromiseResult({ ...renderState, result }, componentName, throwJsErrors);
}
const consoleReplayScript = buildConsoleReplay();
return JSON.stringify(createResultObject(result, consoleReplayScript, renderState));
}
return createPromiseResult({ ...renderState, result }, componentName, throwJsErrors);
}
const consoleReplayScript = buildConsoleReplay(console.history);
return JSON.stringify(createResultObject(result, consoleReplayScript, renderState));
}


function serverRenderReactComponentInternal(options: RenderParams): null | string | Promise<RenderResult> {
function serverRenderReactComponent(options: RenderParams): null | string | Promise<RenderResult> {
const { name: componentName, domNodeId, trace, props, railsContext, renderingReturnsPromises, throwJsErrors } = options;

let renderState: RenderState = {
Expand Down Expand Up @@ -182,14 +180,4 @@ function serverRenderReactComponentInternal(options: RenderParams): null | strin
return createFinalResult(renderState, componentName, throwJsErrors);
}

const serverRenderReactComponent: typeof serverRenderReactComponentInternal = (options) => {
try {
return serverRenderReactComponentInternal(options);
} finally {
// Reset console history after each render.
// See `RubyEmbeddedJavaScript.console_polyfill` for initialization.
console.history = [];
}
};

export default serverRenderReactComponent;