Skip to content

Commit

Permalink
[Flight] Override prepareStackTrace when reading stacks (#29740)
Browse files Browse the repository at this point in the history
This lets us ensure that we use the original V8 format and it lets us
skip source mapping. Source mapping every call can be expensive since we
do it eagerly for server components even if an error doesn't happen.

In the case of an error being thrown we don't actually always do this in
practice because if a try/catch before us touches it or if something in
onError touches it (which the default console.error does), it has
already been initialized. So we have to be resilient to thrown errors
having other formats.

These are not as perf sensitive since something actually threw but if
you want better perf in these cases, you can simply do something like
`onError(error) { console.error(error.message) }` instead.

The server has to be aware whether it's looking up original or compiled
output. I currently use the file:// check to determine if it's referring
to a source mapped file or compiled file in the fixture. A bundled app
can more easily check if it's a bundle or not.
  • Loading branch information
sebmarkbage authored Jun 5, 2024
1 parent d2767c9 commit 1df34bd
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 20 deletions.
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ module.exports = {
$ReadOnlyArray: 'readonly',
$ArrayBufferView: 'readonly',
$Shape: 'readonly',
CallSite: 'readonly',
ConsoleTask: 'readonly', // TOOD: Figure out what the official name of this will be.
ReturnType: 'readonly',
AnimationFrameID: 'readonly',
Expand Down
26 changes: 14 additions & 12 deletions fixtures/flight/server/region.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,11 @@ if (process.env.NODE_ENV === 'development') {
res.set('Content-type', 'application/json');
let requestedFilePath = req.query.name;

let isCompiledOutput = false;
if (requestedFilePath.startsWith('file://')) {
// We assume that if it was prefixed with file:// it's referring to the compiled output
// and if it's a direct file path we assume it's source mapped back to original format.
isCompiledOutput = true;
requestedFilePath = requestedFilePath.slice(7);
}

Expand All @@ -204,11 +208,11 @@ if (process.env.NODE_ENV === 'development') {
let map;
// There are two ways to return a source map depending on what we observe in error.stack.
// A real app will have a similar choice to make for which strategy to pick.
if (!sourceMap || Error.prepareStackTrace === undefined) {
// When --enable-source-maps is enabled, the error.stack that we use to track
// stacks will have had the source map already applied so it's pointing to the
// original source. We return a blank source map that just maps everything to
// the original source in this case.
if (!sourceMap || !isCompiledOutput) {
// If a file doesn't have a source map, such as this file, then we generate a blank
// source map that just contains the original content and segments pointing to the
// original lines.
// Similarly
const sourceContent = await readFile(requestedFilePath, 'utf8');
const lines = sourceContent.split('\n').length;
map = {
Expand All @@ -222,13 +226,11 @@ if (process.env.NODE_ENV === 'development') {
sourceRoot: '',
};
} else {
// If something has overridden prepareStackTrace it is likely not getting the
// natively applied source mapping to error.stack and so the line will point to
// the compiled output similar to how a browser works.
// E.g. ironically this can happen with the source-map-support library that is
// auto-invoked by @babel/register if external source maps are generated.
// In this case we just use the source map that the native source mapping would
// have used.
// We always set prepareStackTrace before reading the stack so that we get the stack
// without source maps applied. Therefore we have to use the original source map.
// If something read .stack before we did, we might observe the line/column after
// source mapping back to the original file. We use the isCompiledOutput check above
// in that case.
map = sourceMap.payload;
}
res.write(JSON.stringify(map));
Expand Down
45 changes: 37 additions & 8 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,41 @@ function isNotExternal(stackFrame: string): boolean {
return !externalRegExp.test(stackFrame);
}

function prepareStackTrace(
error: Error,
structuredStackTrace: CallSite[],
): string {
const name = error.name || 'Error';
const message = error.message || '';
let stack = name + ': ' + message;
for (let i = 0; i < structuredStackTrace.length; i++) {
stack += '\n at ' + structuredStackTrace[i].toString();
}
return stack;
}

function getStack(error: Error): string {
// We override Error.prepareStackTrace with our own version that normalizes
// the stack to V8 formatting even if the server uses other formatting.
// It also ensures that source maps are NOT applied to this since that can
// be slow we're better off doing that lazily from the client instead of
// eagerly on the server. If the stack has already been read, then we might
// not get a normalized stack and it might still have been source mapped.
// So the client still needs to be resilient to this.
const previousPrepare = Error.prepareStackTrace;
Error.prepareStackTrace = prepareStackTrace;
try {
// eslint-disable-next-line react-internal/safe-string-coercion
return String(error.stack);
} finally {
Error.prepareStackTrace = previousPrepare;
}
}

function initCallComponentFrame(): string {
// Extract the stack frame of the callComponentInDEV function.
const error = callComponentInDEV(Error, 'react-stack-top-frame', {});
const stack = error.stack;
const stack = getStack(error);
const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0;
const endIdx = stack.indexOf('\n', startIdx);
if (endIdx === -1) {
Expand All @@ -155,7 +186,7 @@ function initCallIteratorFrame(): string {
(callIteratorInDEV: any)({next: null});
return '';
} catch (error) {
const stack = error.stack;
const stack = getStack(error);
const startIdx = stack.startsWith('TypeError: ')
? stack.indexOf('\n') + 1
: 0;
Expand All @@ -174,7 +205,7 @@ function initCallLazyInitFrame(): string {
_init: Error,
_payload: 'react-stack-top-frame',
});
const stack = error.stack;
const stack = getStack(error);
const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0;
const endIdx = stack.indexOf('\n', startIdx);
if (endIdx === -1) {
Expand All @@ -188,7 +219,7 @@ function filterDebugStack(error: Error): string {
// to save bandwidth even in DEV. We'll also replay these stacks on the client so by
// stripping them early we avoid that overhead. Otherwise we'd normally just rely on
// the DevTools or framework's ignore lists to filter them out.
let stack = error.stack;
let stack = getStack(error);
if (stack.startsWith('Error: react-stack-top-frame\n')) {
// V8's default formatting prefixes with the error message which we
// don't want/need.
Expand Down Expand Up @@ -2601,8 +2632,7 @@ function emitPostponeChunk(
try {
// eslint-disable-next-line react-internal/safe-string-coercion
reason = String(postponeInstance.message);
// eslint-disable-next-line react-internal/safe-string-coercion
stack = String(postponeInstance.stack);
stack = getStack(postponeInstance);
} catch (x) {}
row = serializeRowHeader('P', id) + stringify({reason, stack}) + '\n';
} else {
Expand All @@ -2627,8 +2657,7 @@ function emitErrorChunk(
if (error instanceof Error) {
// eslint-disable-next-line react-internal/safe-string-coercion
message = String(error.message);
// eslint-disable-next-line react-internal/safe-string-coercion
stack = String(error.stack);
stack = getStack(error);
} else if (typeof error === 'object' && error !== null) {
message = describeObjectForErrorMessage(error);
} else {
Expand Down

0 comments on commit 1df34bd

Please sign in to comment.