Skip to content

Commit

Permalink
Ensure Issue Overlay sourcemaps externals in Turbopack
Browse files Browse the repository at this point in the history 
Was noticeable by a bunch of 500s from the middleware
when externals were sourcemapped.

We special case these differently in other places
by checking for `webpack://` frames.
Special casing `webpack://` only fixes externals
that are bundled via Webpack e.g. `next-server` runtimes.
  • Loading branch information
@eps1lon
eps1lon committed Dec 3, 2024
1 parent e5336c2 commit 356055b
Show file tree
Hide file tree
Showing 2 changed files with 172 additions and 21 deletions.
151 changes: 148 additions & 3 deletions packages/next/src/client/components/react-dev-overlay/server/middleware-turbopack.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ import path from 'path'
import url from 'url'
import { launchEditor } from '../internal/helpers/launchEditor'
import type { StackFrame } from 'next/dist/compiled/stacktrace-parser'
import { SourceMapConsumer } from 'next/dist/compiled/source-map08'
import type { Project, TurbopackStackFrame } from '../../../../build/swc/types'
import { getSourceMapFromFile } from '../internal/helpers/get-source-map-from-file'
import { findSourceMap } from 'node:module'
import { findSourceMap, type SourceMapPayload } from 'node:module'

function shouldIgnorePath(modulePath: string): boolean {
return (
Expand All @@ -33,7 +34,10 @@ export async function batchedTraceSource(
project: Project,
frame: TurbopackStackFrame
): Promise<{ frame: IgnorableStackFrame; source: string | null } | undefined> {
const file = frame.file ? decodeURIComponent(frame.file) : undefined
const file = frame.file
? // TODO(veil): Why are the frames sent encoded?
decodeURIComponent(frame.file)
: undefined
if (!file) return

const sourceFrame = await project.traceSource(frame)
Expand Down Expand Up @@ -108,11 +112,150 @@ function createStackFrame(searchParams: URLSearchParams) {
} satisfies TurbopackStackFrame
}

/**
* https://tc39.es/source-map/#index-map
*/
interface IndexSourceMapSection {
offset: {
line: number
column: number
}
map: ModernRawSourceMap
}

// TODO(veil): Upstream types
interface IndexSourceMap {
version: number
file: string
sections: IndexSourceMapSection[]
}

interface ModernRawSourceMap extends SourceMapPayload {
ignoreList?: number[]
}

type ModernSourceMapPayload = ModernRawSourceMap | IndexSourceMap

/**
* Finds the sourcemap payload applicable to a given frame.
* Equal to the input unless an Index Source Map is used.
*/
function findApplicableSourceMapPayload(
frame: TurbopackStackFrame,
payload: ModernSourceMapPayload
): ModernRawSourceMap | undefined {
if ('sections' in payload) {
const frameLine = frame.line ?? 0
const frameColumn = frame.column ?? 0
// Sections must not overlap and must be sorted: https://tc39.es/source-map/#section-object
// Therefore the last section that has an offset less than or equal to the frame is the applicable one.
// TODO(veil): Binary search
let section: IndexSourceMapSection | undefined = payload.sections[0]
for (
let i = 0;
i < payload.sections.length &&
payload.sections[i].offset.line <= frameLine &&
payload.sections[i].offset.column <= frameColumn;
i++
) {
section = payload.sections[i]
}

return section === undefined ? undefined : section.map
} else {
return payload
}
}

async function nativeTraceSource(
frame: TurbopackStackFrame
): Promise<{ frame: IgnorableStackFrame; source: string | null } | undefined> {
const sourceMap = findSourceMap(
// TODO(veil): Why are the frames sent encoded?
decodeURIComponent(frame.file)
)
if (sourceMap !== undefined) {
const traced = await SourceMapConsumer.with(
sourceMap.payload,
null,
async (consumer) => {
const originalPosition = consumer.originalPositionFor({
line: frame.line ?? 1,
column: frame.column ?? 1,
})

if (originalPosition.source === null) {
return null
}

const sourceContent: string | null =
consumer.sourceContentFor(
originalPosition.source,
/* returnNullOnMissing */ true
) ?? null

return { originalPosition, sourceContent }
}
)

if (traced !== null) {
const { originalPosition, sourceContent } = traced
const applicableSourceMap = findApplicableSourceMapPayload(
frame,
sourceMap.payload
)

// TODO(veil): Upstream a method to sourcemap consumer that immediately says if a frame is ignored or not.
let ignored = false
if (applicableSourceMap === undefined) {
console.error(
'No applicable source map found in sections for frame',
frame
)
} else {
// TODO: O(n^2). Consider moving `ignoreList` into a Set
const sourceIndex = applicableSourceMap.sources.indexOf(
originalPosition.source!
)
ignored = applicableSourceMap.ignoreList?.includes(sourceIndex) ?? false
}

const originalStackFrame: IgnorableStackFrame = {
methodName:
originalPosition.name ||
// default is not a valid identifier in JS so webpack uses a custom variable when it's an unnamed default export
// Resolve it back to `default` for the method name if the source position didn't have the method.
frame.methodName
?.replace('__WEBPACK_DEFAULT_EXPORT__', 'default')
?.replace('__webpack_exports__.', '') ||
'<unknown>',
column: originalPosition.column ?? 0,
file: originalPosition.source,
lineNumber: originalPosition.line ?? 0,
// TODO: c&p from async createOriginalStackFrame but why not frame.arguments?
arguments: [],
ignored,
}

return {
frame: originalStackFrame,
source: sourceContent,
}
}
}

return undefined
}

export async function createOriginalStackFrame(
project: Project,
frame: TurbopackStackFrame
): Promise<OriginalStackFrameResponse | null> {
const traced = await batchedTraceSource(project, frame)
const traced =
(await nativeTraceSource(frame)) ??
// TODO(veil): When would the bundler know more than native?
// If it's faster, try the bundler first and fall back to native later.
(await batchedTraceSource(project, frame))
if (!traced) {
return null
}
Expand Down Expand Up @@ -193,6 +336,8 @@ export function getSourceMapMiddleware(project: Project) {
return badRequest(res)
}

// TODO(veil): Always try the native version first.
// Externals could also be files that aren't bundled via Webpack.
if (
filename.startsWith('webpack://') ||
filename.startsWith('webpack-internal:///')
Expand Down
42 changes: 24 additions & 18 deletions test/integration/server-side-dev-errors/test/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,10 @@ describe('server-side dev errors', () => {

const stderrOutput = stripAnsi(stderr.slice(stderrIdx))
if (isTurbopack) {
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toStartWith(
' ⨯ test/integration/server-side-dev-errors/pages/gsp.js (6:3) @ getStaticProps' +
'\n ⨯ test/integration/server-side-dev-errors/pages/gsp.js (6:3) @ getStaticProps'
' ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/gsp.js (6:3) @ getStaticProps' +
'\n ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/gsp.js (6:3) @ getStaticProps'
)
expect(stderrOutput).toContain(
' ⨯ ReferenceError: missingVar is not defined' +
Expand Down Expand Up @@ -119,9 +120,10 @@ describe('server-side dev errors', () => {

const stderrOutput = stripAnsi(stderr.slice(stderrIdx))
if (isTurbopack) {
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toStartWith(
' ⨯ test/integration/server-side-dev-errors/pages/gssp.js (6:3) @ getServerSideProps' +
'\n ⨯ test/integration/server-side-dev-errors/pages/gssp.js (6:3) @ getServerSideProps'
' ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/gssp.js (6:3) @ getServerSideProps' +
'\n ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/gssp.js (6:3) @ getServerSideProps'
)
expect(stderrOutput).toContain(
' ⨯ ReferenceError: missingVar is not defined' +
Expand Down Expand Up @@ -172,9 +174,10 @@ describe('server-side dev errors', () => {

const stderrOutput = stripAnsi(stderr.slice(stderrIdx))
if (isTurbopack) {
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toStartWith(
' ⨯ test/integration/server-side-dev-errors/pages/blog/[slug].js (6:3) @ getServerSideProps' +
'\n ⨯ test/integration/server-side-dev-errors/pages/blog/[slug].js (6:3) @ getServerSideProps'
' ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/blog/[slug].js (6:3) @ getServerSideProps' +
'\n ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/blog/[slug].js (6:3) @ getServerSideProps'
)
expect(stderrOutput).toContain(
' ⨯ ReferenceError: missingVar is not defined' +
Expand Down Expand Up @@ -225,9 +228,10 @@ describe('server-side dev errors', () => {

const stderrOutput = stripAnsi(stderr.slice(stderrIdx))
if (isTurbopack) {
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toStartWith(
' ⨯ test/integration/server-side-dev-errors/pages/api/hello.js (2:3) @ handler' +
'\n ⨯ test/integration/server-side-dev-errors/pages/api/hello.js (2:3) @ handler'
' ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/api/hello.js (2:3) @ handler' +
'\n ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/api/hello.js (2:3) @ handler'
)
expect(stderrOutput).toContain(
' ⨯ ReferenceError: missingVar is not defined' +
Expand Down Expand Up @@ -280,9 +284,10 @@ describe('server-side dev errors', () => {
// FIXME(veil): error repeated
// FIXME(veil): codeframe repeated after " ⨯ unhandledRejection: Error: catch this rejection"
if (isTurbopack) {
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toStartWith(
' ⨯ test/integration/server-side-dev-errors/pages/api/blog/[slug].js (2:3) @ handler' +
'\n ⨯ test/integration/server-side-dev-errors/pages/api/blog/[slug].js (2:3) @ handler'
' ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/api/blog/[slug].js (2:3) @ handler' +
'\n ⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/api/blog/[slug].js (2:3) @ handler'
)
expect(stderrOutput).toContain(
'\n ⨯ Error: missingVar is not defined' +
Expand Down Expand Up @@ -328,6 +333,7 @@ describe('server-side dev errors', () => {
// FIXME(veil): codeframe repeated after " ⨯ unhandledRejection: Error: catch this rejection"
if (isTurbopack) {
// TODO(veil): digest: undefined should be omitted?
// FIXME(veil): Paths include root twice. Bug in generated Turbopack sourcemaps.
expect(stderrOutput).toMatchInlineSnapshot(`
"Error: catch this rejection
at Timeout._onTimeout (../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-rejection.js:7:19)
Expand All @@ -338,8 +344,8 @@ describe('server-side dev errors', () => {
8 | }, 10)
9 | return {
10 | props: {},
⨯ test/integration/server-side-dev-errors/pages/uncaught-rejection.js (7:20) @ Timeout._onTimeout
⨯ test/integration/server-side-dev-errors/pages/uncaught-rejection.js (7:20) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-rejection.js (7:20) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-rejection.js (7:20) @ Timeout._onTimeout
⨯ unhandledRejection: Error: catch this rejection
at Timeout._onTimeout (./test/integration/server-side-dev-errors/pages/uncaught-rejection.js:7:20) {
digest: undefined
Expand Down Expand Up @@ -438,8 +444,8 @@ describe('server-side dev errors', () => {
8 | }, 10)
9 | return {
10 | props: {},
⨯ test/integration/server-side-dev-errors/pages/uncaught-empty-rejection.js (7:20) @ Timeout._onTimeout
⨯ test/integration/server-side-dev-errors/pages/uncaught-empty-rejection.js (7:20) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-empty-rejection.js (7:20) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-empty-rejection.js (7:20) @ Timeout._onTimeout
⨯ unhandledRejection: Error:
at Timeout._onTimeout (./test/integration/server-side-dev-errors/pages/uncaught-empty-rejection.js:7:20) {
digest: undefined
Expand Down Expand Up @@ -537,8 +543,8 @@ describe('server-side dev errors', () => {
8 | }, 10)
9 | return {
10 | props: {},
⨯ test/integration/server-side-dev-errors/pages/uncaught-exception.js (7:11) @ Timeout._onTimeout
⨯ test/integration/server-side-dev-errors/pages/uncaught-exception.js (7:11) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-exception.js (7:11) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-exception.js (7:11) @ Timeout._onTimeout
⨯ uncaughtException: Error: catch this exception
at Timeout._onTimeout (./test/integration/server-side-dev-errors/pages/uncaught-exception.js:7:11) {
digest: undefined
Expand Down Expand Up @@ -636,8 +642,8 @@ describe('server-side dev errors', () => {
8 | }, 10)
9 | return {
10 | props: {},
⨯ test/integration/server-side-dev-errors/pages/uncaught-empty-exception.js (7:11) @ Timeout._onTimeout
⨯ test/integration/server-side-dev-errors/pages/uncaught-empty-exception.js (7:11) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-empty-exception.js (7:11) @ Timeout._onTimeout
⨯ ../../test/integration/server-side-dev-errors/test/integration/server-side-dev-errors/pages/uncaught-empty-exception.js (7:11) @ Timeout._onTimeout
⨯ uncaughtException: Error:
at Timeout._onTimeout (./test/integration/server-side-dev-errors/pages/uncaught-empty-exception.js:7:11) {
digest: undefined
Expand Down

0 comments on commit 356055b

Please sign in to comment.