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

Add support for replaying logs happen on async server operations #1649

Merged
Show file tree
Hide file tree
Changes from all 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
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));
}
24 changes: 12 additions & 12 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,20 +129,12 @@ 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
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
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));
}


Expand Down Expand Up @@ -188,6 +186,8 @@ const serverRenderReactComponent: typeof serverRenderReactComponentInternal = (o
} finally {
// Reset console history after each render.
// See `RubyEmbeddedJavaScript.console_polyfill` for initialization.
// This is necessary when ExecJS and old versions of node renderer are used.
// New versions of node renderer reset the console history automatically.
console.history = [];
}
};
Expand Down
Loading