Skip to content

Commit

Permalink
[Turbopack] fix root and project path usages in a monorepo (#73552)
Browse files Browse the repository at this point in the history
### What?

Improved trace source maps to return relative paths and always relative to process.cwd.

`turbopack://` urls in SourceMaps are always relative to the project root.

`file://` urls do no longer contain duplicate path segments.

Add a test case

Also:
* improve error when sourcemap lookup throws
  • Loading branch information
sokra authored Dec 19, 2024
1 parent d3529ba commit 53bb90c
Show file tree
Hide file tree
Showing 35 changed files with 531 additions and 176 deletions.
66 changes: 46 additions & 20 deletions crates/napi/src/next_api/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ use turbo_tasks::{
get_effects, Completion, Effects, ReadRef, ResolvedVc, TransientInstance, UpdateInfo, Vc,
};
use turbo_tasks_fs::{
util::uri_from_file, DiskFileSystem, FileContent, FileSystem, FileSystemPath,
get_relative_path_to, util::uri_from_file, DiskFileSystem, FileContent, FileSystem,
FileSystemPath,
};
use turbopack_core::{
diagnostics::PlainDiagnostic,
Expand Down Expand Up @@ -1015,6 +1016,7 @@ pub fn project_update_info_subscribe(
pub struct StackFrame {
pub is_server: bool,
pub is_internal: Option<bool>,
pub original_file: Option<String>,
pub file: String,
// 1-indexed, unlike source map tokens
pub line: Option<u32>,
Expand Down Expand Up @@ -1084,6 +1086,7 @@ pub async fn get_source_map(
pub async fn project_trace_source(
#[napi(ts_arg_type = "{ __napiType: \"Project\" }")] project: External<ProjectInstance>,
frame: StackFrame,
current_directory_file_url: String,
) -> napi::Result<Option<StackFrame>> {
let turbo_tasks = project.turbo_tasks.clone();
let container = project.container;
Expand Down Expand Up @@ -1120,27 +1123,50 @@ pub async fn project_trace_source(
}
};

let project_path_uri =
uri_from_file(project.container.project().project_path(), None).await? + "/";
let (source_file, is_internal) =
if let Some(source_file) = original_file.strip_prefix(&project_path_uri) {
// Client code uses file://
(source_file, false)
} else if let Some(source_file) =
original_file.strip_prefix(&*SOURCE_MAP_PREFIX_PROJECT)
{
// Server code uses turbopack://[project]
// TODO should this also be file://?
(source_file, false)
} else if let Some(source_file) = original_file.strip_prefix(SOURCE_MAP_PREFIX) {
// All other code like turbopack://[turbopack] is internal code
(source_file, true)
} else {
bail!("Original file ({}) outside project", original_file)
};
let project_root_uri =
uri_from_file(project.container.project().project_root_path(), None).await? + "/";
let (file, original_file, is_internal) = if let Some(source_file) =
original_file.strip_prefix(&project_root_uri)
{
// Client code uses file://
(
get_relative_path_to(&current_directory_file_url, &original_file)
// TODO(sokra) remove this to include a ./ here to make it a relative path
.trim_start_matches("./")
.to_string(),
Some(source_file.to_string()),
false,
)
} else if let Some(source_file) =
original_file.strip_prefix(&*SOURCE_MAP_PREFIX_PROJECT)
{
// Server code uses turbopack://[project]
// TODO should this also be file://?
(
get_relative_path_to(
&current_directory_file_url,
&format!("{}{}", project_root_uri, source_file),
)
// TODO(sokra) remove this to include a ./ here to make it a relative path
.trim_start_matches("./")
.to_string(),
Some(source_file.to_string()),
false,
)
} else if let Some(source_file) = original_file.strip_prefix(SOURCE_MAP_PREFIX) {
// All other code like turbopack://[turbopack] is internal code
(source_file.to_string(), None, true)
} else {
bail!(
"Original file ({}) outside project ({})",
original_file,
project_root_uri
)
};

Ok(Some(StackFrame {
file: source_file.to_string(),
file,
original_file,
method_name: name.as_ref().map(ToString::to_string),
line,
column,
Expand Down
14 changes: 7 additions & 7 deletions crates/next-api/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ impl Project {
}

#[turbo_tasks::function]
fn project_root_path(self: Vc<Self>) -> Vc<FileSystemPath> {
pub fn project_root_path(self: Vc<Self>) -> Vc<FileSystemPath> {
self.project_fs().root()
}

Expand Down Expand Up @@ -693,7 +693,7 @@ impl Project {

let node_execution_chunking_context = Vc::upcast(
NodeJsChunkingContext::builder(
self.project_path().to_resolved().await?,
self.project_root_path().to_resolved().await?,
node_root,
node_root,
node_root.join("build/chunks".into()).to_resolved().await?,
Expand Down Expand Up @@ -820,7 +820,7 @@ impl Project {
#[turbo_tasks::function]
pub(super) fn client_chunking_context(self: Vc<Self>) -> Vc<Box<dyn ChunkingContext>> {
get_client_chunking_context(
self.project_path(),
self.project_root_path(),
self.client_relative_path(),
self.next_config().computed_asset_prefix(),
self.client_compile_time_info().environment(),
Expand All @@ -838,7 +838,7 @@ impl Project {
if client_assets {
get_server_chunking_context_with_client_assets(
self.next_mode(),
self.project_path(),
self.project_root_path(),
self.node_root(),
self.client_relative_path(),
self.next_config().computed_asset_prefix(),
Expand All @@ -849,7 +849,7 @@ impl Project {
} else {
get_server_chunking_context(
self.next_mode(),
self.project_path(),
self.project_root_path(),
self.node_root(),
self.server_compile_time_info().environment(),
self.module_id_strategy(),
Expand All @@ -866,7 +866,7 @@ impl Project {
if client_assets {
get_edge_chunking_context_with_client_assets(
self.next_mode(),
self.project_path(),
self.project_root_path(),
self.node_root(),
self.client_relative_path(),
self.next_config().computed_asset_prefix(),
Expand All @@ -877,7 +877,7 @@ impl Project {
} else {
get_edge_chunking_context(
self.next_mode(),
self.project_path(),
self.project_root_path(),
self.node_root(),
self.edge_compile_time_info().environment(),
self.module_id_strategy(),
Expand Down
8 changes: 4 additions & 4 deletions crates/next-core/src/next_server/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ pub fn get_server_runtime_entries(
#[turbo_tasks::function]
pub async fn get_server_chunking_context_with_client_assets(
mode: Vc<NextMode>,
project_path: ResolvedVc<FileSystemPath>,
root_path: ResolvedVc<FileSystemPath>,
node_root: ResolvedVc<FileSystemPath>,
client_root: ResolvedVc<FileSystemPath>,
asset_prefix: ResolvedVc<Option<RcStr>>,
Expand All @@ -987,7 +987,7 @@ pub async fn get_server_chunking_context_with_client_assets(
// different server chunking contexts. OR the build chunking context should
// support both production and development modes.
let mut builder = NodeJsChunkingContext::builder(
project_path,
root_path,
node_root,
client_root,
node_root
Expand Down Expand Up @@ -1019,7 +1019,7 @@ pub async fn get_server_chunking_context_with_client_assets(
#[turbo_tasks::function]
pub async fn get_server_chunking_context(
mode: Vc<NextMode>,
project_path: ResolvedVc<FileSystemPath>,
root_path: ResolvedVc<FileSystemPath>,
node_root: ResolvedVc<FileSystemPath>,
environment: ResolvedVc<Environment>,
module_id_strategy: ResolvedVc<Box<dyn ModuleIdStrategy>>,
Expand All @@ -1030,7 +1030,7 @@ pub async fn get_server_chunking_context(
// different server chunking contexts. OR the build chunking context should
// support both production and development modes.
let mut builder = NodeJsChunkingContext::builder(
project_path,
root_path,
node_root,
node_root,
node_root.join("server/chunks".into()).to_resolved().await?,
Expand Down
3 changes: 2 additions & 1 deletion packages/next/src/build/swc/generated-native.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ export interface StackFrame {
}
export declare function projectTraceSource(
project: { __napiType: 'Project' },
frame: StackFrame
frame: StackFrame,
currentDirectoryFileUrl: string
): Promise<StackFrame | null>
export declare function projectGetSourceForAsset(
project: { __napiType: 'Project' },
Expand Down
9 changes: 7 additions & 2 deletions packages/next/src/build/swc/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -746,9 +746,14 @@ function bindingToApi(
}

traceSource(
stackFrame: TurbopackStackFrame
stackFrame: TurbopackStackFrame,
currentDirectoryFileUrl: string
): Promise<TurbopackStackFrame | null> {
return binding.projectTraceSource(this._nativeProject, stackFrame)
return binding.projectTraceSource(
this._nativeProject,
stackFrame,
currentDirectoryFileUrl
)
}

getSourceForAsset(filePath: string): Promise<string | null> {
Expand Down
4 changes: 3 additions & 1 deletion packages/next/src/build/swc/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ export interface TurbopackStackFrame {
isServer: boolean
isInternal?: boolean
file: string
originalFile?: string
/** 1-indexed, unlike source map tokens */
line?: number
/** 1-indexed, unlike source map tokens */
Expand Down Expand Up @@ -207,7 +208,8 @@ export interface Project {
getSourceMapSync(filePath: string): string | null

traceSource(
stackFrame: TurbopackStackFrame
stackFrame: TurbopackStackFrame,
currentDirectoryFileUrl: string
): Promise<TurbopackStackFrame | null>

updateInfoSubscribe(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ 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, type SourceMapPayload } from 'node:module'
import { pathToFileURL } from 'node:url'

function shouldIgnorePath(modulePath: string): boolean {
return (
Expand All @@ -40,7 +41,9 @@ export async function batchedTraceSource(
: undefined
if (!file) return

const sourceFrame = await project.traceSource(frame)
const currentDirectoryFileUrl = pathToFileURL(process.cwd()).href

const sourceFrame = await project.traceSource(frame, currentDirectoryFileUrl)
if (!sourceFrame) {
return {
frame: {
Expand All @@ -56,20 +59,21 @@ export async function batchedTraceSource(
}

let source = null
const originalFile = sourceFrame.originalFile
// Don't look up source for node_modules or internals. These can often be large bundled files.
const ignored =
shouldIgnorePath(sourceFrame.file) ||
shouldIgnorePath(originalFile ?? sourceFrame.file) ||
// isInternal means resource starts with turbopack://[turbopack]
!!sourceFrame.isInternal
if (sourceFrame && sourceFrame.file && !ignored) {
let sourcePromise = currentSourcesByFile.get(sourceFrame.file)
if (originalFile && !ignored) {
let sourcePromise = currentSourcesByFile.get(originalFile)
if (!sourcePromise) {
sourcePromise = project.getSourceForAsset(sourceFrame.file)
currentSourcesByFile.set(sourceFrame.file, sourcePromise)
sourcePromise = project.getSourceForAsset(originalFile)
currentSourcesByFile.set(originalFile, sourcePromise)
setTimeout(() => {
// Cache file reads for 100ms, as frames will often reference the same
// files and can be large.
currentSourcesByFile.delete(sourceFrame.file!)
currentSourcesByFile.delete(originalFile!)
}, 100)
}
source = await sourcePromise
Expand Down Expand Up @@ -231,10 +235,7 @@ async function nativeTraceSource(
'<unknown>',
column: (originalPosition.column ?? 0) + 1,
file: originalPosition.source?.startsWith('file://')
? path.relative(
process.cwd(),
url.fileURLToPath(originalPosition.source)
)
? relativeToCwd(originalPosition.source)
: originalPosition.source,
lineNumber: originalPosition.line ?? 0,
// TODO: c&p from async createOriginalStackFrame but why not frame.arguments?
Expand All @@ -252,6 +253,12 @@ async function nativeTraceSource(
return undefined
}

function relativeToCwd(file: string): string {
const relPath = path.relative(process.cwd(), url.fileURLToPath(file))
// TODO(sokra) include a ./ here to make it a relative path
return relPath
}

async function createOriginalStackFrame(
project: Project,
frame: TurbopackStackFrame
Expand Down Expand Up @@ -288,7 +295,7 @@ export function getOverlayMiddleware(project: Project) {
try {
originalStackFrame = await createOriginalStackFrame(project, frame)
} catch (e: any) {
return internalServerError(res, e.message)
return internalServerError(res, e.stack)
}

if (!originalStackFrame) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { ReactNode } from 'react'
export default function Root({ children }: { children: ReactNode }) {
return (
<html>
<body>{children}</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { text } from 'my-package/typescript'

export default function Page() {
return <p>{text}</p>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use client'

import { text } from 'my-package/typescript'

export default function Page() {
return <p>{text}</p>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>hello world</p>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
throw new Error('Expected error')
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use client'

import { useEffect } from 'react'

export default function Page() {
useEffect(function effectCallback() {
innerFunction()
})
return <p>Hello Source Maps</p>
}

function innerFunction() {
innerArrowFunction()
}

const innerArrowFunction = () => {
require('../separate-file')
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export default async function Page({ searchParams }) {
// We don't want the build to fail in production
if (process.env.NODE_ENV === 'development') {
innerFunction()
}
return <p>Hello Source Maps</p>
}

function innerFunction() {
innerArrowFunction()
}

const innerArrowFunction = () => {
require('../separate-file')
}
Loading

0 comments on commit 53bb90c

Please sign in to comment.