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

Turbopack: handle removed routes #77890

Merged
merged 2 commits into from
Apr 8, 2025
Merged
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
22 changes: 8 additions & 14 deletions crates/napi/src/next_api/endpoint.rs
Original file line number Diff line number Diff line change
@@ -3,10 +3,11 @@ use std::{ops::Deref, sync::Arc};
use anyhow::Result;
use napi::{bindgen_prelude::External, JsFunction};
use next_api::{
operation::OptionEndpoint,
paths::ServerPath,
route::{
endpoint_server_changed_operation, endpoint_write_to_disk_operation, Endpoint,
EndpointOutputPaths,
endpoint_client_changed_operation, endpoint_server_changed_operation,
endpoint_write_to_disk_operation, EndpointOutputPaths,
},
};
use tracing::Instrument;
@@ -71,7 +72,7 @@ impl From<Option<EndpointOutputPaths>> for NapiWrittenEndpoint {
server_paths: server_paths.into_iter().map(From::from).collect(),
..Default::default()
},
None => Self {
Some(EndpointOutputPaths::NotFound) | None => Self {
r#type: "none".to_string(),
..Default::default()
},
@@ -85,10 +86,10 @@ impl From<Option<EndpointOutputPaths>> for NapiWrittenEndpoint {
// some async functions (in this case `endpoint_write_to_disk`) can cause
// higher-ranked lifetime errors. See https://github.com/rust-lang/rust/issues/102211
// 2. the type_complexity clippy lint.
pub struct ExternalEndpoint(pub VcArc<Box<dyn Endpoint>>);
pub struct ExternalEndpoint(pub VcArc<OptionEndpoint>);

impl Deref for ExternalEndpoint {
type Target = VcArc<Box<dyn Endpoint>>;
type Target = VcArc<OptionEndpoint>;

fn deref(&self) -> &Self::Target {
&self.0
@@ -105,7 +106,7 @@ struct WrittenEndpointWithIssues {

#[turbo_tasks::function(operation)]
async fn get_written_endpoint_with_issues_operation(
endpoint_op: OperationVc<Box<dyn Endpoint>>,
endpoint_op: OperationVc<OptionEndpoint>,
) -> Result<Vc<WrittenEndpointWithIssues>> {
let write_to_disk_op = endpoint_write_to_disk_operation(endpoint_op);
let (written, issues, diagnostics, effects) =
@@ -214,7 +215,7 @@ impl Eq for EndpointIssuesAndDiags {}

#[turbo_tasks::function(operation)]
async fn subscribe_issues_and_diags_operation(
endpoint_op: OperationVc<Box<dyn Endpoint>>,
endpoint_op: OperationVc<OptionEndpoint>,
should_include_issues: bool,
) -> Result<Vc<EndpointIssuesAndDiags>> {
let changed_op = endpoint_server_changed_operation(endpoint_op);
@@ -241,13 +242,6 @@ async fn subscribe_issues_and_diags_operation(
}
}

#[turbo_tasks::function(operation)]
fn endpoint_client_changed_operation(
endpoint_op: OperationVc<Box<dyn Endpoint>>,
) -> Vc<Completion> {
endpoint_op.connect().client_changed()
}

#[napi(ts_return_type = "{ __napiType: \"RootTask\" }")]
pub fn endpoint_client_changed_subscribe(
#[napi(ts_arg_type = "{ __napiType: \"Endpoint\" }")] endpoint: External<ExternalEndpoint>,
5 changes: 3 additions & 2 deletions crates/napi/src/next_api/project.rs
Original file line number Diff line number Diff line change
@@ -9,7 +9,8 @@ use napi::{
use next_api::{
entrypoints::Entrypoints,
operation::{
EntrypointsOperation, InstrumentationOperation, MiddlewareOperation, RouteOperation,
EntrypointsOperation, InstrumentationOperation, MiddlewareOperation, OptionEndpoint,
RouteOperation,
},
project::{
DefineEnv, DraftModeOptions, PartialProjectOptions, Project, ProjectContainer,
@@ -580,7 +581,7 @@ pub struct NapiRoute {

impl NapiRoute {
fn from_route(pathname: String, value: RouteOperation, turbo_tasks: &NextTurboTasks) -> Self {
let convert_endpoint = |endpoint: OperationVc<Box<dyn Endpoint>>| {
let convert_endpoint = |endpoint: OperationVc<OptionEndpoint>| {
Some(External::new(ExternalEndpoint(VcArc::new(
turbo_tasks.clone(),
endpoint,
179 changes: 132 additions & 47 deletions crates/next-api/src/operation.rs
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
use turbo_rcstr::RcStr;
use turbo_tasks::{
debug::ValueDebugFormat, get_effects, trace::TraceRawVcs, CollectiblesSource, FxIndexMap,
NonLocalValue, OperationVc, ResolvedVc, Vc,
NonLocalValue, OperationValue, OperationVc, ResolvedVc, TaskInput, Vc,
};
use turbopack_core::{diagnostics::Diagnostic, issue::IssueDescriptionExt};

@@ -25,9 +25,9 @@ pub struct EntrypointsOperation {
pub routes: FxIndexMap<RcStr, RouteOperation>,
pub middleware: Option<MiddlewareOperation>,
pub instrumentation: Option<InstrumentationOperation>,
pub pages_document_endpoint: OperationVc<Box<dyn Endpoint>>,
pub pages_app_endpoint: OperationVc<Box<dyn Endpoint>>,
pub pages_error_endpoint: OperationVc<Box<dyn Endpoint>>,
pub pages_document_endpoint: OperationVc<OptionEndpoint>,
pub pages_app_endpoint: OperationVc<OptionEndpoint>,
pub pages_error_endpoint: OperationVc<OptionEndpoint>,
}

/// Removes diagnostics, issues, and effects from the top-level `entrypoints` operation so that
@@ -53,96 +53,181 @@ impl EntrypointsOperation {
routes: e
.routes
.iter()
.map(|(k, v)| (k.clone(), wrap_route(v, entrypoints)))
.map(|(k, v)| (k.clone(), pick_route(entrypoints, k.clone(), v)))
.collect(),
middleware: e.middleware.as_ref().map(|m| MiddlewareOperation {
endpoint: wrap(m.endpoint, entrypoints),
middleware: e.middleware.as_ref().map(|_| MiddlewareOperation {
endpoint: pick_endpoint(entrypoints, EndpointSelector::Middleware),
}),
instrumentation: e
.instrumentation
.as_ref()
.map(|i| InstrumentationOperation {
node_js: wrap(i.node_js, entrypoints),
edge: wrap(i.edge, entrypoints),
.map(|_| InstrumentationOperation {
node_js: pick_endpoint(entrypoints, EndpointSelector::InstrumentationNodeJs),
edge: pick_endpoint(entrypoints, EndpointSelector::InstrumentationEdge),
}),
pages_document_endpoint: wrap(e.pages_document_endpoint, entrypoints),
pages_app_endpoint: wrap(e.pages_app_endpoint, entrypoints),
pages_error_endpoint: wrap(e.pages_error_endpoint, entrypoints),
pages_document_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesDocument),
pages_app_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesApp),
pages_error_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesError),
}
.cell())
}
}

fn wrap_route(route: &Route, entrypoints: OperationVc<Entrypoints>) -> RouteOperation {
fn pick_route(entrypoints: OperationVc<Entrypoints>, key: RcStr, route: &Route) -> RouteOperation {
match route {
Route::Page {
html_endpoint,
data_endpoint,
} => RouteOperation::Page {
html_endpoint: wrap(*html_endpoint, entrypoints),
data_endpoint: wrap(*data_endpoint, entrypoints),
Route::Page { .. } => RouteOperation::Page {
html_endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageHtml(key.clone())),
data_endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageData(key)),
},
Route::PageApi { endpoint } => RouteOperation::PageApi {
endpoint: wrap(*endpoint, entrypoints),
Route::PageApi { .. } => RouteOperation::PageApi {
endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageApi(key)),
},
Route::AppPage(pages) => RouteOperation::AppPage(
pages
.iter()
.map(|p| AppPageRouteOperation {
.enumerate()
.map(|(i, p)| AppPageRouteOperation {
original_name: p.original_name.clone(),
html_endpoint: wrap(p.html_endpoint, entrypoints),
rsc_endpoint: wrap(p.rsc_endpoint, entrypoints),
html_endpoint: pick_endpoint(
entrypoints,
EndpointSelector::RouteAppPageHtml(key.clone(), i),
),
rsc_endpoint: pick_endpoint(
entrypoints,
EndpointSelector::RouteAppPageRsc(key.clone(), i),
),
})
.collect(),
),
Route::AppRoute {
original_name,
endpoint,
} => RouteOperation::AppRoute {
Route::AppRoute { original_name, .. } => RouteOperation::AppRoute {
original_name: original_name.clone(),
endpoint: wrap(*endpoint, entrypoints),
endpoint: pick_endpoint(entrypoints, EndpointSelector::RouteAppRoute(key)),
},
Route::Conflict => RouteOperation::Conflict,
}
}

/// Given a resolved `Endpoint` and the `Entrypoints` operation that it comes from, connect the
/// operation and return a `OperationVc` of the `Entrypoint`. This `Endpoint` operation will keep
/// the entire `Entrypoints` operation alive.
#[derive(
Debug,
Clone,
TaskInput,
Serialize,
Deserialize,
TraceRawVcs,
PartialEq,
Eq,
Hash,
ValueDebugFormat,
NonLocalValue,
OperationValue,
)]
enum EndpointSelector {
RoutePageHtml(RcStr),
RoutePageData(RcStr),
RoutePageApi(RcStr),
RouteAppPageHtml(RcStr, usize),
RouteAppPageRsc(RcStr, usize),
RouteAppRoute(RcStr),
InstrumentationNodeJs,
InstrumentationEdge,
Middleware,
PagesDocument,
PagesApp,
PagesError,
}

#[turbo_tasks::value(transparent)]
pub struct OptionEndpoint(Option<ResolvedVc<Box<dyn Endpoint>>>);

/// Given a selector and the `Entrypoints` operation that it comes from, connect the operation and
/// return an `OperationVc` containing the selected value. The returned operation will keep the
/// entire `Entrypoints` operation alive.
#[turbo_tasks::function(operation)]
fn wrap(
endpoint: ResolvedVc<Box<dyn Endpoint>>,
async fn pick_endpoint(
op: OperationVc<Entrypoints>,
) -> Vc<Box<dyn Endpoint>> {
Comment on lines -113 to -116
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgw fn wrap(endpoint: ResolvedVc<Box<dyn Endpoint>>, op: OperationVc<Entrypoints>) is a problem when op not consistently produces endpoint. Only op no longer produces endpoint we end up with a dangling Vc.

let _ = op.connect();
*endpoint
selector: EndpointSelector,
) -> Result<Vc<OptionEndpoint>> {
let endpoints = op.connect().strongly_consistent().await?;
let endpoint = match selector {
EndpointSelector::InstrumentationNodeJs => {
endpoints.instrumentation.as_ref().map(|i| i.node_js)
}
EndpointSelector::InstrumentationEdge => endpoints.instrumentation.as_ref().map(|i| i.edge),
EndpointSelector::Middleware => endpoints.middleware.as_ref().map(|m| m.endpoint),
EndpointSelector::PagesDocument => Some(endpoints.pages_document_endpoint),
EndpointSelector::PagesApp => Some(endpoints.pages_app_endpoint),
EndpointSelector::PagesError => Some(endpoints.pages_error_endpoint),
EndpointSelector::RoutePageHtml(name) => {
if let Some(Route::Page { html_endpoint, .. }) = endpoints.routes.get(&name) {
Some(*html_endpoint)
} else {
None
}
}
EndpointSelector::RoutePageData(name) => {
if let Some(Route::Page { data_endpoint, .. }) = endpoints.routes.get(&name) {
Some(*data_endpoint)
} else {
None
}
}
EndpointSelector::RoutePageApi(name) => {
if let Some(Route::PageApi { endpoint }) = endpoints.routes.get(&name) {
Some(*endpoint)
} else {
None
}
}
EndpointSelector::RouteAppPageHtml(name, i) => {
if let Some(Route::AppPage(pages)) = endpoints.routes.get(&name) {
pages.get(i).as_ref().map(|p| p.html_endpoint)
} else {
None
}
}
EndpointSelector::RouteAppPageRsc(name, i) => {
if let Some(Route::AppPage(pages)) = endpoints.routes.get(&name) {
pages.get(i).as_ref().map(|p| p.rsc_endpoint)
} else {
None
}
}
EndpointSelector::RouteAppRoute(name) => {
if let Some(Route::AppRoute { endpoint, .. }) = endpoints.routes.get(&name) {
Some(*endpoint)
} else {
None
}
}
};
Ok(Vc::cell(endpoint))
}

#[derive(Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, ValueDebugFormat, NonLocalValue)]
pub struct InstrumentationOperation {
pub node_js: OperationVc<Box<dyn Endpoint>>,
pub edge: OperationVc<Box<dyn Endpoint>>,
pub node_js: OperationVc<OptionEndpoint>,
pub edge: OperationVc<OptionEndpoint>,
}

#[derive(Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, ValueDebugFormat, NonLocalValue)]
pub struct MiddlewareOperation {
pub endpoint: OperationVc<Box<dyn Endpoint>>,
pub endpoint: OperationVc<OptionEndpoint>,
}

#[turbo_tasks::value(shared)]
#[derive(Clone, Debug)]
pub enum RouteOperation {
Page {
html_endpoint: OperationVc<Box<dyn Endpoint>>,
data_endpoint: OperationVc<Box<dyn Endpoint>>,
html_endpoint: OperationVc<OptionEndpoint>,
data_endpoint: OperationVc<OptionEndpoint>,
},
PageApi {
endpoint: OperationVc<Box<dyn Endpoint>>,
endpoint: OperationVc<OptionEndpoint>,
},
AppPage(Vec<AppPageRouteOperation>),
AppRoute {
original_name: RcStr,
endpoint: OperationVc<Box<dyn Endpoint>>,
endpoint: OperationVc<OptionEndpoint>,
},
Conflict,
}
@@ -160,6 +245,6 @@ pub enum RouteOperation {
)]
pub struct AppPageRouteOperation {
pub original_name: RcStr,
pub html_endpoint: OperationVc<Box<dyn Endpoint>>,
pub rsc_endpoint: OperationVc<Box<dyn Endpoint>>,
pub html_endpoint: OperationVc<OptionEndpoint>,
pub rsc_endpoint: OperationVc<OptionEndpoint>,
}
38 changes: 29 additions & 9 deletions crates/next-api/src/route.rs
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@ use turbopack_core::{
output::OutputAssets,
};

use crate::{paths::ServerPath, project::Project};
use crate::{operation::OptionEndpoint, paths::ServerPath, project::Project};

#[derive(
TraceRawVcs,
@@ -97,17 +97,36 @@ async fn endpoint_output_assets_operation(
}

#[turbo_tasks::function(operation)]
pub fn endpoint_write_to_disk_operation(
endpoint: OperationVc<Box<dyn Endpoint>>,
) -> Vc<EndpointOutputPaths> {
endpoint_write_to_disk(endpoint.connect())
pub async fn endpoint_write_to_disk_operation(
endpoint: OperationVc<OptionEndpoint>,
) -> Result<Vc<EndpointOutputPaths>> {
Ok(if let Some(endpoint) = *endpoint.connect().await? {
endpoint_write_to_disk(*endpoint)
} else {
EndpointOutputPaths::NotFound.cell()
})
}

#[turbo_tasks::function(operation)]
pub async fn endpoint_server_changed_operation(
endpoint: OperationVc<OptionEndpoint>,
) -> Result<Vc<Completion>> {
Ok(if let Some(endpoint) = *endpoint.connect().await? {
endpoint.server_changed()
} else {
Completion::new()
})
}

#[turbo_tasks::function(operation)]
pub fn endpoint_server_changed_operation(
endpoint: OperationVc<Box<dyn Endpoint>>,
) -> Vc<Completion> {
endpoint.connect().server_changed()
pub async fn endpoint_client_changed_operation(
endpoint: OperationVc<OptionEndpoint>,
) -> Result<Vc<Completion>> {
Ok(if let Some(endpoint) = *endpoint.connect().await? {
endpoint.client_changed()
} else {
Completion::new()
})
}

#[turbo_tasks::value(shared)]
@@ -131,6 +150,7 @@ pub enum EndpointOutputPaths {
server_paths: Vec<ServerPath>,
client_paths: Vec<RcStr>,
},
NotFound,
}

/// The routes as map from pathname to route. (pathname includes the leading
15 changes: 9 additions & 6 deletions test/development/app-hmr/hmr.test.ts
Original file line number Diff line number Diff line change
@@ -50,6 +50,8 @@ describe(`app-dir-hmr`, () => {
// The new page should be rendered
const newHTML = await next.render('/folder-renamed')
expect(newHTML).toContain('Hello')

expect(next.cliOutput).not.toContain('FATAL')
} finally {
// Rename it back
await next.renameFolder('app/folder-renamed', 'app/folder')
@@ -89,7 +91,6 @@ describe(`app-dir-hmr`, () => {
// details are unimportant.
expect(fastRefreshLogs).toEqual(
expect.arrayContaining([
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
@@ -102,6 +103,8 @@ describe(`app-dir-hmr`, () => {
await retry(async () => {
expect(await browser.elementByCss('p').text()).toBe('mac')
})

expect(next.cliOutput).not.toContain('FATAL')
})

it.each(['node', 'node-module-var', 'edge', 'edge-module-var'])(
@@ -118,7 +121,7 @@ describe(`app-dir-hmr`, () => {
expect(logs).toEqual(
expect.arrayContaining([
expect.objectContaining({
message: '[Fast Refresh] rebuilding',
message: expect.stringContaining('[Fast Refresh] done'),
source: 'log',
}),
])
@@ -143,6 +146,8 @@ describe(`app-dir-hmr`, () => {
await retry(async () => {
expect(await browser.elementByCss('p').text()).toBe('mac')
})

expect(next.cliOutput).not.toContain('FATAL')
}
)

@@ -168,10 +173,6 @@ describe(`app-dir-hmr`, () => {
// TODO: Should assert on all logs but these are cluttered with logs from our test utils (e.g. playwright tracing or webdriver)
expect(logs).toEqual(
expect.arrayContaining([
{
message: '[Fast Refresh] rebuilding',
source: 'log',
},
{
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
@@ -187,6 +188,8 @@ describe(`app-dir-hmr`, () => {
)
// No MPA navigation triggered
expect(await browser.eval('window.__TEST_NO_RELOAD')).toEqual(true)

expect(next.cliOutput).not.toContain('FATAL')
})
})
})