-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
[Flight] Add findSourceMapURL option to get a URL to load Server source maps from #29708
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ babelRegister({ | |
], | ||
presets: ['@babel/preset-react'], | ||
plugins: ['@babel/transform-modules-commonjs'], | ||
sourceMaps: process.env.NODE_ENV === 'development' ? 'inline' : false, | ||
}); | ||
|
||
if (typeof fetch === 'undefined') { | ||
|
@@ -38,6 +39,8 @@ const app = express(); | |
const compress = require('compression'); | ||
const {Readable} = require('node:stream'); | ||
|
||
const nodeModule = require('node:module'); | ||
|
||
app.use(compress()); | ||
|
||
// Application | ||
|
@@ -176,6 +179,69 @@ app.get('/todos', function (req, res) { | |
]); | ||
}); | ||
|
||
if (process.env.NODE_ENV === 'development') { | ||
const rootDir = path.resolve(__dirname, '../'); | ||
|
||
app.get('/source-maps', async function (req, res, next) { | ||
try { | ||
res.set('Content-type', 'application/json'); | ||
let requestedFilePath = req.query.name; | ||
|
||
if (requestedFilePath.startsWith('file://')) { | ||
requestedFilePath = requestedFilePath.slice(7); | ||
} | ||
|
||
const relativePath = path.relative(rootDir, requestedFilePath); | ||
if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | ||
// This is outside the root directory of the app. Forbid it to be served. | ||
res.status = 403; | ||
res.write('{}'); | ||
res.end(); | ||
return; | ||
} | ||
|
||
const sourceMap = nodeModule.findSourceMap(requestedFilePath); | ||
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. | ||
const sourceContent = await readFile(requestedFilePath, 'utf8'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now this throws when we request source map for node internal frames e.g. Should we handle these on the server gracefully? Otherwise it might be a bit spammy. I guess the client can't handle it since it doesn't necessarily know what runtime internals are (assuming Bun uses other protocols). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally they're filtered before at the findSourceMapURL so that it can just return null instead. This doesn't happen with just this PR though because we filter those out of the stack. It's not an issue until the next PR where it allows those to show up. |
||
const lines = sourceContent.split('\n').length; | ||
map = { | ||
version: 3, | ||
sources: [requestedFilePath], | ||
sourcesContent: [sourceContent], | ||
// Note: This approach to mapping each line only lets you jump to each line | ||
// not jump to a column within a line. To do that, you need a proper source map | ||
// generated for each parsed segment or add a segment for each column. | ||
mappings: 'AAAA' + ';AACA'.repeat(lines - 1), | ||
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. | ||
map = sourceMap.payload; | ||
} | ||
res.write(JSON.stringify(map)); | ||
res.end(); | ||
} catch (x) { | ||
res.status = 500; | ||
res.write('{}'); | ||
res.end(); | ||
console.error(x); | ||
} | ||
}); | ||
} | ||
|
||
app.listen(3001, () => { | ||
console.log('Regional Flight Server listening on port 3001...'); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,6 +239,8 @@ Chunk.prototype.then = function <T>( | |
} | ||
}; | ||
|
||
export type FindSourceMapURLCallback = (fileName: string) => null | string; | ||
|
||
export type Response = { | ||
_bundlerConfig: SSRModuleMap, | ||
_moduleLoading: ModuleLoading, | ||
|
@@ -255,6 +257,7 @@ export type Response = { | |
_buffer: Array<Uint8Array>, // chunks received so far as part of this row | ||
_tempRefs: void | TemporaryReferenceSet, // the set temporary references can be resolved from | ||
_debugRootTask?: null | ConsoleTask, // DEV-only | ||
_debugFindSourceMapURL?: void | FindSourceMapURLCallback, // DEV-only | ||
}; | ||
|
||
function readChunk<T>(chunk: SomeChunk<T>): T { | ||
|
@@ -696,7 +699,7 @@ function createElement( | |
console, | ||
getTaskName(type), | ||
); | ||
const callStack = buildFakeCallStack(stack, createTaskFn); | ||
const callStack = buildFakeCallStack(response, stack, createTaskFn); | ||
// This owner should ideally have already been initialized to avoid getting | ||
// user stack frames on the stack. | ||
const ownerTask = | ||
|
@@ -1140,6 +1143,7 @@ export function createResponse( | |
encodeFormAction: void | EncodeFormActionCallback, | ||
nonce: void | string, | ||
temporaryReferences: void | TemporaryReferenceSet, | ||
findSourceMapURL: void | FindSourceMapURLCallback, | ||
): Response { | ||
const chunks: Map<number, SomeChunk<any>> = new Map(); | ||
const response: Response = { | ||
|
@@ -1166,6 +1170,9 @@ export function createResponse( | |
// TODO: Make this string configurable. | ||
response._debugRootTask = (console: any).createTask('"use server"'); | ||
} | ||
if (__DEV__) { | ||
response._debugFindSourceMapURL = findSourceMapURL; | ||
} | ||
// Don't inline this call because it causes closure to outline the call above. | ||
response._fromJSON = createFromJSONCallback(response); | ||
return response; | ||
|
@@ -1673,6 +1680,7 @@ const fakeFunctionCache: Map<string, FakeFunction<any>> = __DEV__ | |
function createFakeFunction<T>( | ||
name: string, | ||
filename: string, | ||
sourceMap: null | string, | ||
line: number, | ||
col: number, | ||
): FakeFunction<T> { | ||
|
@@ -1697,7 +1705,9 @@ function createFakeFunction<T>( | |
'_()\n'; | ||
} | ||
|
||
if (filename) { | ||
if (sourceMap) { | ||
code += '//# sourceMappingURL=' + sourceMap; | ||
} else if (filename) { | ||
code += '//# sourceURL=' + filename; | ||
} | ||
|
||
|
@@ -1720,10 +1730,18 @@ function createFakeFunction<T>( | |
return fn; | ||
} | ||
|
||
// This matches either of these V8 formats. | ||
// at name (filename:0:0) | ||
// at filename:0:0 | ||
// at async filename:0:0 | ||
const frameRegExp = | ||
/^ {3} at (?:(.+) \(([^\)]+):(\d+):(\d+)\)|([^\)]+):(\d+):(\d+))$/; | ||
/^ {3} at (?:(.+) \(([^\)]+):(\d+):(\d+)\)|(?:async )?([^\)]+):(\d+):(\d+))$/; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add one or two examples as comments here? Doesn't need to be super exhaustive. It's just a very intimidating regex 😬 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment but I'm still ambivalent about how much we should cover here. This doesn't really cover all possible. |
||
|
||
function buildFakeCallStack<T>(stack: string, innerCall: () => T): () => T { | ||
function buildFakeCallStack<T>( | ||
response: Response, | ||
stack: string, | ||
innerCall: () => T, | ||
): () => T { | ||
const frames = stack.split('\n'); | ||
let callStack = innerCall; | ||
for (let i = 0; i < frames.length; i++) { | ||
|
@@ -1739,7 +1757,13 @@ function buildFakeCallStack<T>(stack: string, innerCall: () => T): () => T { | |
const filename = parsed[2] || parsed[5] || ''; | ||
const line = +(parsed[3] || parsed[6]); | ||
const col = +(parsed[4] || parsed[7]); | ||
fn = createFakeFunction(name, filename, line, col); | ||
const sourceMap = response._debugFindSourceMapURL | ||
? response._debugFindSourceMapURL(filename) | ||
: null; | ||
fn = createFakeFunction(name, filename, sourceMap, line, col); | ||
// TODO: This cache should technically live on the response since the _debugFindSourceMapURL | ||
// function is an input and can vary by response. | ||
fakeFunctionCache.set(frame, fn); | ||
} | ||
callStack = fn.bind(null, callStack); | ||
} | ||
|
@@ -1770,7 +1794,7 @@ function initializeFakeTask( | |
console, | ||
getServerComponentTaskName(componentInfo), | ||
); | ||
const callStack = buildFakeCallStack(stack, createTaskFn); | ||
const callStack = buildFakeCallStack(response, stack, createTaskFn); | ||
|
||
if (ownerTask === null) { | ||
const rootTask = response._debugRootTask; | ||
|
@@ -1832,6 +1856,7 @@ function resolveConsoleEntry( | |
return; | ||
} | ||
const callStack = buildFakeCallStack( | ||
response, | ||
stackTrace, | ||
printToConsole.bind(null, methodName, args, env), | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a requirement for this feature to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's just for client source maps. They end up off by one and not mapping to the right columns so not testing the proper experience.