From 5117cc6ea990a6ddda5585a5fe2bf6a1914285c8 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Thu, 4 Jan 2024 13:06:45 +0100 Subject: [PATCH] Improve consistency of issues and diagnostics for napi calls (#60198) ### What? get value, issues and diagnostics in a single strongly consistent call ### Why? Before issues and diagnostics where not received strongly consistent, which could result in stale data. ### How? Closes PACK-2194 --- .../crates/napi/src/next_api/endpoint.rs | 49 ++++-- .../crates/napi/src/next_api/project.rs | 139 ++++++++++++++---- .../crates/napi/src/next_api/utils.rs | 20 +-- packages/next-swc/crates/next-api/src/lib.rs | 2 +- .../crates/next-api/src/server_actions.rs | 5 +- 5 files changed, 165 insertions(+), 50 deletions(-) diff --git a/packages/next-swc/crates/napi/src/next_api/endpoint.rs b/packages/next-swc/crates/napi/src/next_api/endpoint.rs index 6e0b4dfceb..2575e6a810 100644 --- a/packages/next-swc/crates/napi/src/next_api/endpoint.rs +++ b/packages/next-swc/crates/napi/src/next_api/endpoint.rs @@ -1,13 +1,16 @@ -use std::ops::Deref; +use std::{ops::Deref, sync::Arc}; +use anyhow::Result; use napi::{bindgen_prelude::External, JsFunction}; use next_api::{ route::{Endpoint, WrittenEndpoint}, server_paths::ServerPath, }; use tracing::Instrument; -use turbo_tasks::Vc; -use turbopack_binding::turbopack::core::error::PrettyPrintError; +use turbo_tasks::{ReadRef, Vc}; +use turbopack_binding::turbopack::core::{ + diagnostics::PlainDiagnostic, error::PrettyPrintError, issue::PlainIssue, +}; use super::utils::{ get_diagnostics, get_issues, subscribe, NapiDiagnostic, NapiIssue, RootTask, TurbopackResult, @@ -80,7 +83,31 @@ impl Deref for ExternalEndpoint { } } +#[turbo_tasks::value(serialization = "none")] +struct WrittenEndpointWithIssues { + written: ReadRef, + issues: Arc>>, + diagnostics: Arc>>, +} + +#[turbo_tasks::function] +async fn get_written_endpoint_with_issues( + endpoint: Vc>, +) -> Result> { + let write_to_disk = endpoint.write_to_disk(); + let written = write_to_disk.strongly_consistent().await?; + let issues = get_issues(write_to_disk).await?; + let diagnostics = get_diagnostics(write_to_disk).await?; + Ok(WrittenEndpointWithIssues { + written, + issues, + diagnostics, + } + .cell()) +} + #[napi] +#[tracing::instrument(skip_all)] pub async fn endpoint_write_to_disk( #[napi(ts_arg_type = "{ __napiType: \"Endpoint\" }")] endpoint: External, ) -> napi::Result> { @@ -88,15 +115,17 @@ pub async fn endpoint_write_to_disk( let endpoint = ***endpoint; let (written, issues, diags) = turbo_tasks .run_once(async move { - let write_to_disk = endpoint.write_to_disk(); - let written = write_to_disk.strongly_consistent().await?; - let issues = get_issues(write_to_disk).await?; - let diags = get_diagnostics(write_to_disk).await?; - Ok((written, issues, diags)) + let WrittenEndpointWithIssues { + written, + issues, + diagnostics, + } = &*get_written_endpoint_with_issues(endpoint) + .strongly_consistent() + .await?; + Ok((written.clone(), issues.clone(), diagnostics.clone())) }) .await .map_err(|e| napi::Error::from_reason(PrettyPrintError(&e).to_string()))?; - // TODO diagnostics Ok(TurbopackResult { result: NapiWrittenEndpoint::from(&*written), issues: issues.iter().map(|i| NapiIssue::from(&**i)).collect(), @@ -124,7 +153,7 @@ pub fn endpoint_server_changed_subscribe( let diags = get_diagnostics(changed).await?; Ok((issues, diags)) } else { - Ok((vec![], vec![])) + Ok((Arc::new(vec![]), Arc::new(vec![]))) } } .instrument(tracing::info_span!("server changes subscription")) diff --git a/packages/next-swc/crates/napi/src/next_api/project.rs b/packages/next-swc/crates/napi/src/next_api/project.rs index e9bce8f3f8..bbfc761270 100644 --- a/packages/next-swc/crates/napi/src/next_api/project.rs +++ b/packages/next-swc/crates/napi/src/next_api/project.rs @@ -7,8 +7,9 @@ use napi::{ JsFunction, Status, }; use next_api::{ + entrypoints::Entrypoints, project::{ - DefineEnv, Instrumentation, Middleware, PartialProjectOptions, ProjectContainer, + DefineEnv, Instrumentation, Middleware, PartialProjectOptions, Project, ProjectContainer, ProjectOptions, }, route::{Endpoint, Route}, @@ -21,7 +22,7 @@ use tracing::Instrument; use tracing_subscriber::{ prelude::__tracing_subscriber_SubscriberExt, util::SubscriberInitExt, EnvFilter, Registry, }; -use turbo_tasks::{TransientInstance, TurboTasks, UpdateInfo, Vc}; +use turbo_tasks::{ReadRef, TransientInstance, TurboTasks, UpdateInfo, Vc}; use turbopack_binding::{ turbo::{ tasks_fs::{FileContent, FileSystem}, @@ -29,9 +30,11 @@ use turbopack_binding::{ }, turbopack::{ core::{ + diagnostics::PlainDiagnostic, error::PrettyPrintError, + issue::PlainIssue, source_map::{GenerateSourceMap, Token}, - version::{PartialUpdate, TotalUpdate, Update}, + version::{PartialUpdate, TotalUpdate, Update, VersionState}, }, ecmascript_hmr_protocol::{ClientUpdateInstruction, ResourceIdentifier}, trace_utils::{ @@ -424,6 +427,29 @@ struct NapiEntrypoints { pub pages_error_endpoint: External, } +#[turbo_tasks::value(serialization = "none")] +struct EntrypointsWithIssues { + entrypoints: ReadRef, + issues: Arc>>, + diagnostics: Arc>>, +} + +#[turbo_tasks::function] +async fn get_entrypoints_with_issues( + container: Vc, +) -> Result> { + let entrypoints_operation = container.entrypoints(); + let entrypoints = entrypoints_operation.strongly_consistent().await?; + let issues = get_issues(entrypoints_operation).await?; + let diagnostics = get_diagnostics(entrypoints_operation).await?; + Ok(EntrypointsWithIssues { + entrypoints, + issues, + diagnostics, + } + .cell()) +} + #[napi(ts_return_type = "{ __napiType: \"RootTask\" }")] pub fn project_entrypoints_subscribe( #[napi(ts_arg_type = "{ __napiType: \"Project\" }")] project: External, @@ -436,13 +462,14 @@ pub fn project_entrypoints_subscribe( func, move || { async move { - let entrypoints_operation = container.entrypoints(); - let entrypoints = entrypoints_operation.strongly_consistent().await?; - - let issues = get_issues(entrypoints_operation).await?; - let diags = get_diagnostics(entrypoints_operation).await?; - - Ok((entrypoints, issues, diags)) + let EntrypointsWithIssues { + entrypoints, + issues, + diagnostics, + } = &*get_entrypoints_with_issues(container) + .strongly_consistent() + .await?; + Ok((entrypoints.clone(), issues.clone(), diagnostics.clone())) } .instrument(tracing::info_span!("entrypoints subscription")) }, @@ -491,6 +518,31 @@ pub fn project_entrypoints_subscribe( ) } +#[turbo_tasks::value(serialization = "none")] +struct HmrUpdateWithIssues { + update: ReadRef, + issues: Arc>>, + diagnostics: Arc>>, +} + +#[turbo_tasks::function] +async fn hmr_update( + project: Vc, + identifier: String, + state: Vc, +) -> Result> { + let update_operation = project.hmr_update(identifier, state); + let update = update_operation.strongly_consistent().await?; + let issues = get_issues(update_operation).await?; + let diagnostics = get_diagnostics(update_operation).await?; + Ok(HmrUpdateWithIssues { + update, + issues, + diagnostics, + } + .cell()) +} + #[napi(ts_return_type = "{ __napiType: \"RootTask\" }")] pub fn project_hmr_events( #[napi(ts_arg_type = "{ __napiType: \"Project\" }")] project: External, @@ -510,14 +562,17 @@ pub fn project_hmr_events( let identifier = outer_identifier.clone(); let session = session.clone(); async move { - let state = project - .project() - .hmr_version_state(identifier.clone(), session); - let update_operation = project.project().hmr_update(identifier, state); - let update = update_operation.strongly_consistent().await?; - let issues = get_issues(update_operation).await?; - let diags = get_diagnostics(update_operation).await?; - match &*update { + let project = project.project().resolve().await?; + let state = project.hmr_version_state(identifier.clone(), session); + let update = hmr_update(project, identifier, state) + .strongly_consistent() + .await?; + let HmrUpdateWithIssues { + update, + issues, + diagnostics, + } = &*update; + match &**update { Update::None => {} Update::Total(TotalUpdate { to }) => { state.set(to.clone()).await?; @@ -526,7 +581,7 @@ pub fn project_hmr_events( state.set(to.clone()).await?; } } - Ok((update, issues, diags)) + Ok((update.clone(), issues.clone(), diagnostics.clone())) } .instrument(tracing::info_span!( "HMR subscription", @@ -574,6 +629,29 @@ struct HmrIdentifiers { pub identifiers: Vec, } +#[turbo_tasks::value(serialization = "none")] +struct HmrIdentifiersWithIssues { + identifiers: ReadRef>, + issues: Arc>>, + diagnostics: Arc>>, +} + +#[turbo_tasks::function] +async fn get_hmr_identifiers_with_issues( + container: Vc, +) -> Result> { + let hmr_identifiers_operation = container.hmr_identifiers(); + let hmr_identifiers = hmr_identifiers_operation.strongly_consistent().await?; + let issues = get_issues(hmr_identifiers_operation).await?; + let diagnostics = get_diagnostics(hmr_identifiers_operation).await?; + Ok(HmrIdentifiersWithIssues { + identifiers: hmr_identifiers, + issues, + diagnostics, + } + .cell()) +} + #[napi(ts_return_type = "{ __napiType: \"RootTask\" }")] pub fn project_hmr_identifiers_subscribe( #[napi(ts_arg_type = "{ __napiType: \"Project\" }")] project: External, @@ -585,20 +663,22 @@ pub fn project_hmr_identifiers_subscribe( turbo_tasks.clone(), func, move || async move { - let hmr_identifiers_operation = container.hmr_identifiers(); - let hmr_identifiers = hmr_identifiers_operation.strongly_consistent().await?; + let HmrIdentifiersWithIssues { + identifiers, + issues, + diagnostics, + } = &*get_hmr_identifiers_with_issues(container) + .strongly_consistent() + .await?; - let issues = get_issues(hmr_identifiers_operation).await?; - let diags = get_diagnostics(hmr_identifiers_operation).await?; - - Ok((hmr_identifiers, issues, diags)) + Ok((identifiers.clone(), issues.clone(), diagnostics.clone())) }, move |ctx| { - let (hmr_identifiers, issues, diags) = ctx.value; + let (identifiers, issues, diagnostics) = ctx.value; Ok(vec![TurbopackResult { result: HmrIdentifiers { - identifiers: hmr_identifiers + identifiers: identifiers .iter() .map(|ident| ident.to_string()) .collect::>(), @@ -607,7 +687,10 @@ pub fn project_hmr_identifiers_subscribe( .iter() .map(|issue| NapiIssue::from(&**issue)) .collect(), - diagnostics: diags.iter().map(|d| NapiDiagnostic::from(d)).collect(), + diagnostics: diagnostics + .iter() + .map(|d| NapiDiagnostic::from(d)) + .collect(), }]) }, ) diff --git a/packages/next-swc/crates/napi/src/next_api/utils.rs b/packages/next-swc/crates/napi/src/next_api/utils.rs index 13ab019977..fe3a4ce15e 100644 --- a/packages/next-swc/crates/napi/src/next_api/utils.rs +++ b/packages/next-swc/crates/napi/src/next_api/utils.rs @@ -77,22 +77,24 @@ pub fn root_task_dispose( Ok(()) } -pub async fn get_issues(source: Vc) -> Result>> { +pub async fn get_issues(source: Vc) -> Result>>> { let issues = source.peek_issues_with_path().await?; - issues.get_plain_issues().await + Ok(Arc::new(issues.get_plain_issues().await?)) } /// Collect [turbopack::core::diagnostics::Diagnostic] from given source, /// returns [turbopack::core::diagnostics::PlainDiagnostic] -pub async fn get_diagnostics(source: Vc) -> Result>> { +pub async fn get_diagnostics(source: Vc) -> Result>>> { let captured_diags = source.peek_diagnostics().await?; - captured_diags - .diagnostics - .iter() - .map(|d| d.into_plain()) - .try_join() - .await + Ok(Arc::new( + captured_diags + .diagnostics + .iter() + .map(|d| d.into_plain()) + .try_join() + .await?, + )) } #[napi(object)] diff --git a/packages/next-swc/crates/next-api/src/lib.rs b/packages/next-swc/crates/next-api/src/lib.rs index 0a1402928e..3a5633f892 100644 --- a/packages/next-swc/crates/next-api/src/lib.rs +++ b/packages/next-swc/crates/next-api/src/lib.rs @@ -3,7 +3,7 @@ mod app; mod dynamic_imports; -mod entrypoints; +pub mod entrypoints; mod instrumentation; mod middleware; mod pages; diff --git a/packages/next-swc/crates/next-api/src/server_actions.rs b/packages/next-swc/crates/next-api/src/server_actions.rs index c2cb352135..cb3defacf4 100644 --- a/packages/next-swc/crates/next-api/src/server_actions.rs +++ b/packages/next-swc/crates/next-api/src/server_actions.rs @@ -56,7 +56,7 @@ pub(crate) async fn create_server_actions_manifest( ) -> Result<(Vc>, Vc>)> { let actions = get_actions(rsc_entry, server_reference_modules, asset_context); let loader = - build_server_actions_loader(project_path, page_name, actions, asset_context).await?; + build_server_actions_loader(project_path, page_name.to_string(), actions, asset_context); let evaluable = Vc::try_resolve_sidecast::>(loader) .await? .context("loader module must be evaluatable")?; @@ -76,9 +76,10 @@ pub(crate) async fn create_server_actions_manifest( /// The actions are reexported under a hashed name (comprised of the exporting /// file's name and the action name). This hash matches the id sent to the /// client and present inside the paired manifest. +#[turbo_tasks::function] async fn build_server_actions_loader( project_path: Vc, - page_name: &str, + page_name: String, actions: Vc, asset_context: Vc>, ) -> Result>> {