From baad064fd1c509fa0c7ff2dfdaec5412ded20766 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Sat, 14 Mar 2026 11:17:21 +0100 Subject: [PATCH 1/4] Turboopack: put import attributes behind a pointer, 23% faster analyze (#91347) The JS analyze microbenchmark got 23% faster: Only a tiny fraction of imports have annotations, be it `import ... with {...}` or `require(/*webpackIgnore: true*/...)`, so a `Option>` mkes sense here. ``` canary: references/jsonwebtoken.js/full time: [63.250 ms 63.918 ms 64.752 ms] change: [-5.2380% -3.0074% -0.8753%] (p = 0.01 < 0.05) Change within noise threshold. Found 12 outliers among 100 measurements (12.00%) 5 (5.00%) high mild 7 (7.00%) high severe now: references/jsonwebtoken.js/full time: [49.070 ms 49.166 ms 49.273 ms] change: [-24.097% -23.079% -22.237%] (p = 0.00 < 0.05) Performance has improved. Found 6 outliers among 100 measurements (6.00%) 3 (3.00%) high mild 3 (3.00%) high severe ```` This came up in https://github.com/vercel/next.js/pull/91278#issuecomment-4058275943, which added another (tiny) field to ImportAttributes. But it caused a 6% regression in the microbenchmark anyway. --- .../src/analyzer/imports.rs | 73 +++++++++++-------- .../turbopack-ecmascript/src/analyzer/mod.rs | 30 ++++++-- .../src/analyzer/well_known.rs | 9 +-- .../src/references/esm/base.rs | 35 ++++++--- .../src/references/mod.rs | 15 ++-- .../graph/class_super/graph-effects.snapshot | 7 +- .../createRequire/graph-effects.snapshot | 14 +--- .../graph/createRequire/graph.snapshot | 7 +- .../graph/default-args/graph.snapshot | 14 +--- .../analyzer/graph/imports/graph.snapshot | 21 +----- .../graph/issue-75938/graph-effects.snapshot | 28 +------ .../analyzer/graph/issue-75938/graph.snapshot | 14 +--- .../graph/issue-77083/graph-effects.snapshot | 21 +----- .../analyzer/graph/pack-2236/graph.snapshot | 7 +- .../graph/path-join/graph-effects.snapshot | 7 +- .../analyzer/graph/path-join/graph.snapshot | 14 +--- .../unreachable-break/graph-effects.snapshot | 42 ++--------- 17 files changed, 129 insertions(+), 229 deletions(-) diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs index 60e4332c26b05..c811a44983681 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, collections::BTreeMap, fmt::Display}; +use std::{borrow::Cow, collections::BTreeMap, fmt::Display, sync::Arc}; use once_cell::sync::Lazy; use rustc_hash::{FxHashMap, FxHashSet}; @@ -60,10 +60,8 @@ static ANNOTATION_CHUNKING_TYPE: Lazy = static ATTRIBUTE_MODULE_TYPE: Lazy = Lazy::new(|| atom!("type").into()); impl ImportAnnotations { - pub fn parse(with: Option<&ObjectLit>) -> ImportAnnotations { - let Some(with) = with else { - return ImportAnnotations::default(); - }; + pub fn parse(with: Option<&ObjectLit>) -> Option { + let with = with?; let mut map = BTreeMap::new(); let mut turbopack_loader_name: Option = None; @@ -148,11 +146,19 @@ impl ImportAnnotations { options: turbopack_loader_options, }); - ImportAnnotations { - map, - turbopack_loader, - turbopack_rename_as, - turbopack_module_type, + if !map.is_empty() + || turbopack_loader.is_some() + || turbopack_rename_as.is_some() + || turbopack_module_type.is_some() + { + Some(ImportAnnotations { + map, + turbopack_loader, + turbopack_rename_as, + turbopack_module_type, + }) + } else { + None } } @@ -181,12 +187,16 @@ impl ImportAnnotations { ); } - Some(ImportAnnotations { - map, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }) + if !map.is_empty() { + Some(ImportAnnotations { + map, + turbopack_loader: None, + turbopack_rename_as: None, + turbopack_module_type: None, + }) + } else { + None + } } /// Returns the content on the transition annotation @@ -395,7 +405,7 @@ pub(crate) enum ImportedSymbol { pub(crate) struct ImportMapReference { pub module_path: Wtf8Atom, pub imported_symbol: ImportedSymbol, - pub annotations: ImportAnnotations, + pub annotations: Option>, pub issue_source: Option, } @@ -581,7 +591,7 @@ impl Analyzer<'_> { span: Span, module_path: Wtf8Atom, imported_symbol: ImportedSymbol, - annotations: ImportAnnotations, + annotations: Option, ) -> Option { let issue_source = self .source @@ -591,7 +601,7 @@ impl Analyzer<'_> { module_path, imported_symbol, issue_source, - annotations, + annotations: annotations.map(Arc::new), }; if let Some(i) = self.data.references.get_index_of(&r) { Some(i) @@ -897,11 +907,11 @@ impl Visit for Analyzer<'_> { _ => None, }; - let attributes = parse_directives(comments, n.args.first()); - - if let Some((callee_span, attributes)) = callee_span.zip(attributes) { + if let Some(callee_span) = callee_span + && let Some(attributes) = parse_directives(comments, n.args.first()) + { self.data.attributes.insert(callee_span.lo, attributes); - }; + } } n.visit_children_with(self); @@ -915,11 +925,11 @@ impl Visit for Analyzer<'_> { _ => None, }; - let attributes = parse_directives(comments, n.args.iter().flatten().next()); - - if let Some((callee_span, attributes)) = callee_span.zip(attributes) { + if let Some(callee_span) = callee_span + && let Some(attributes) = parse_directives(comments, n.args.iter().flatten().next()) + { self.data.attributes.insert(callee_span.lo, attributes); - }; + } } n.visit_children_with(self); @@ -1069,7 +1079,7 @@ mod tests { props: vec![kv_prop(ident_key("turbopackLoader"), str_lit("raw-loader"))], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(annotations.has_turbopack_loader()); let loader = annotations.turbopack_loader().unwrap(); @@ -1091,7 +1101,7 @@ mod tests { ], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(annotations.has_turbopack_loader()); let loader = annotations.turbopack_loader().unwrap(); @@ -1107,7 +1117,7 @@ mod tests { props: vec![kv_prop(ident_key("type"), str_lit("json"))], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(!annotations.has_turbopack_loader()); assert!(annotations.module_type().is_some()); } @@ -1115,7 +1125,6 @@ mod tests { #[test] fn test_parse_empty_with() { let annotations = ImportAnnotations::parse(None); - assert!(!annotations.has_turbopack_loader()); - assert!(annotations.module_type().is_none()); + assert!(annotations.is_none()); } } diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs index 93b22ba2ce351..b5c7f8162b748 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs @@ -9,6 +9,7 @@ use std::{ }; use anyhow::{Result, bail}; +use either::Either; use num_bigint::BigInt; use num_traits::identities::Zero; use once_cell::sync::Lazy; @@ -268,7 +269,7 @@ impl Display for ConstantValue { #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct ModuleValue { pub module: Wtf8Atom, - pub annotations: ImportAnnotations, + pub annotations: Option>, } #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] @@ -799,7 +800,16 @@ impl Display for JsValue { module: name, annotations, }) => { - write!(f, "Module({}, {annotations})", name.to_string_lossy()) + write!( + f, + "Module({}, {})", + name.to_string_lossy(), + if let Some(annotations) = annotations { + Either::Left(annotations) + } else { + Either::Right("{}") + } + ) } JsValue::Unknown { .. } => write!(f, "???"), JsValue::WellKnownObject(obj) => write!(f, "WellKnownObject({obj:?})"), @@ -1618,7 +1628,15 @@ impl JsValue { module: name, annotations, }) => { - format!("module<{}, {annotations}>", name.to_string_lossy()) + format!( + "module<{}, {}>", + name.to_string_lossy(), + if let Some(annotations) = annotations { + Either::Left(annotations) + } else { + Either::Right("{}") + } + ) } JsValue::Unknown { original_value: inner, @@ -3527,9 +3545,7 @@ pub mod test_utils { }; use crate::{ analyzer::{ - RequireContextValue, - builtin::replace_builtin, - imports::{ImportAnnotations, ImportAttributes}, + RequireContextValue, builtin::replace_builtin, imports::ImportAttributes, parse_require_context, }, utils::module_value_to_well_known_object, @@ -3557,7 +3573,7 @@ pub mod test_utils { JsValue::Constant(ConstantValue::Str(v)) => { JsValue::promise(JsValue::Module(ModuleValue { module: v.as_atom().into_owned().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } _ => v.into_unknown(true, "import() non constant"), diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs index 0fd6550de9887..53311f2966a4d 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs @@ -7,8 +7,7 @@ use turbopack_core::compile_time_info::CompileTimeInfo; use url::Url; use super::{ - ConstantValue, JsValue, JsValueUrlKind, ModuleValue, WellKnownFunctionKind, - WellKnownObjectKind, imports::ImportAnnotations, + ConstantValue, JsValue, JsValueUrlKind, ModuleValue, WellKnownFunctionKind, WellKnownObjectKind, }; use crate::analyzer::RequireContextValue; @@ -359,7 +358,7 @@ pub fn import(args: Vec) -> JsValue { [JsValue::Constant(ConstantValue::Str(v))] => { JsValue::promise(JsValue::Module(ModuleValue { module: v.as_atom().into_owned().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } _ => JsValue::unknown( @@ -380,7 +379,7 @@ fn require(args: Vec) -> JsValue { if let Some(s) = args[0].as_str() { JsValue::Module(ModuleValue { module: s.into(), - annotations: ImportAnnotations::default(), + annotations: None, }) } else { JsValue::unknown( @@ -448,7 +447,7 @@ fn require_context_require(val: RequireContextValue, args: Vec) -> Resu Ok(JsValue::Module(ModuleValue { module: m.to_string().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } diff --git a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs index 99088420619d9..ac3648234200b 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs @@ -322,13 +322,13 @@ impl EsmAssetReferences { #[turbo_tasks::value(shared)] #[derive(Hash, Debug, ValueToString)] -#[value_to_string("import {request} with {annotations}")] +#[value_to_string("import {request}")] pub struct EsmAssetReference { pub module: ResolvedVc, pub origin: ResolvedVc>, // Request is a string to avoid eagerly parsing into a `Request` VC pub request: RcStr, - pub annotations: ImportAnnotations, + pub annotations: Option, pub issue_source: IssueSource, pub export_name: Option, pub import_usage: ImportUsage, @@ -339,7 +339,7 @@ pub struct EsmAssetReference { impl EsmAssetReference { fn get_origin(&self) -> Vc> { - if let Some(transition) = self.annotations.transition() { + if let Some(transition) = self.annotations.as_ref().and_then(|a| a.transition()) { self.origin.with_transition(transition.into()) } else { *self.origin @@ -353,7 +353,7 @@ impl EsmAssetReference { origin: ResolvedVc>, request: RcStr, issue_source: IssueSource, - annotations: ImportAnnotations, + annotations: Option, export_name: Option, import_usage: ImportUsage, import_externals: bool, @@ -378,7 +378,7 @@ impl EsmAssetReference { origin: ResolvedVc>, request: RcStr, issue_source: IssueSource, - annotations: ImportAnnotations, + annotations: Option, export_name: Option, import_usage: ImportUsage, import_externals: bool, @@ -406,7 +406,8 @@ impl EsmAssetReference { impl ModuleReference for EsmAssetReference { #[turbo_tasks::function] async fn resolve_reference(&self) -> Result> { - let ty = if let Some(loader) = self.annotations.turbopack_loader() { + let ty = if let Some(loader) = self.annotations.as_ref().and_then(|a| a.turbopack_loader()) + { // Resolve the loader path relative to the importing file let origin = self.get_origin(); let origin_path = origin.origin_path().await?; @@ -428,10 +429,18 @@ impl ModuleReference for EsmAssetReference { loader: loader_fs_path, options: loader.options.clone(), }, - rename_as: self.annotations.turbopack_rename_as().cloned(), - module_type: self.annotations.turbopack_module_type().cloned(), + rename_as: self + .annotations + .as_ref() + .and_then(|a| a.turbopack_rename_as()) + .cloned(), + module_type: self + .annotations + .as_ref() + .and_then(|a| a.turbopack_module_type()) + .cloned(), } - } else if let Some(module_type) = self.annotations.module_type() { + } else if let Some(module_type) = self.annotations.as_ref().and_then(|a| a.module_type()) { EcmaScriptModulesReferenceSubType::ImportWithType(RcStr::from( &*module_type.to_string_lossy(), )) @@ -498,7 +507,8 @@ impl ModuleReference for EsmAssetReference { fn chunking_type(&self) -> Option { self.annotations - .chunking_type() + .as_ref() + .and_then(|a| a.chunking_type()) .unwrap_or(Some(ChunkingType::Parallel { inherit_async: true, hoisted: true, @@ -534,7 +544,10 @@ impl EsmAssetReference { } // only chunked references can be imported - if !matches!(this.annotations.chunking_type(), Some(None)) { + if !matches!( + this.annotations.as_ref().and_then(|a| a.chunking_type()), + Some(None) + ) { let import_externals = this.import_externals; let referenced_asset = self.get_referenced_asset().await?; diff --git a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs index 42be2c3673942..bb3c052b69d96 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs @@ -820,7 +820,7 @@ async fn analyze_ecmascript_module_internal( RcStr::from(&*r.module_path.to_string_lossy()), r.issue_source .unwrap_or_else(|| IssueSource::from_source_only(source)), - r.annotations.clone(), + r.annotations.as_ref().map(|a| (**a).clone()), match &r.imported_symbol { ImportedSymbol::ModuleEvaluation => { should_add_evaluation = true; @@ -1510,12 +1510,15 @@ async fn analyze_ecmascript_module_internal( }; if let Some("__turbopack_module_id__") = export.as_deref() { - let chunking_type = r.await?.annotations.chunking_type().unwrap_or(Some( - ChunkingType::Parallel { + let chunking_type = r + .await? + .annotations + .as_ref() + .and_then(|a| a.chunking_type()) + .unwrap_or(Some(ChunkingType::Parallel { inherit_async: true, hoisted: true, - }, - )); + })); analysis.add_reference_code_gen( EsmModuleIdAssetReference::new(*r, chunking_type), ast_path.into(), @@ -4264,7 +4267,7 @@ pub static TURBOPACK_HELPER_WTF8: Lazy = pub fn is_turbopack_helper_import(import: &ImportDecl) -> bool { let annotations = ImportAnnotations::parse(import.with.as_deref()); - annotations.get(&TURBOPACK_HELPER_WTF8).is_some() + annotations.is_some_and(|a| a.get(&TURBOPACK_HELPER_WTF8).is_some()) } pub fn is_swc_helper_import(import: &ImportDecl) -> bool { diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot index 02f38b127d8e0..8721bbe73f6df 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot @@ -121,12 +121,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot index 188f0333e1814..cd7a05e6a9552 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot @@ -95,12 +95,7 @@ obj: Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), prop: Constant( @@ -308,12 +303,7 @@ obj: Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), prop: Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot index 6c464d7c38acc..539d46005ab8a 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot @@ -58,12 +58,7 @@ Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot index 3d782c399656e..dd8e5cd9164c6 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot @@ -58,12 +58,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -102,12 +97,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot index 09bcb27506c02..1eb1a4ccae3c9 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot @@ -6,12 +6,7 @@ Module( ModuleValue { module: "x2", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -30,12 +25,7 @@ Module( ModuleValue { module: "x1", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -52,12 +42,7 @@ Module( ModuleValue { module: "x3", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), ), diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot index 3389868d842d0..f3dbef6ada397 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot @@ -71,12 +71,7 @@ Module( ModuleValue { module: "react", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -579,12 +574,7 @@ Module( ModuleValue { module: "https://esm.sh/preact/jsx-runtime", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -958,12 +948,7 @@ Module( ModuleValue { module: "./external", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -1084,12 +1069,7 @@ Module( ModuleValue { module: "./external", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot index a8a5aeb100d93..71b6fe1bcf90e 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot @@ -70,12 +70,7 @@ Module( ModuleValue { module: "https://esm.sh/preact/jsx-runtime", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -151,12 +146,7 @@ Module( ModuleValue { module: "react", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot index 7237ed46a8bca..9efd3e24a036c 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot @@ -83,12 +83,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -374,12 +369,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -526,12 +516,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot index 66b2cc8466917..5b94fdfb1d31a 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot @@ -40,12 +40,7 @@ Module( ModuleValue { module: "@upstash/redis", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot index c532678fde867..131e508c7a8dd 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot @@ -1028,12 +1028,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot index e796df057ccad..233a7b4fedf93 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot @@ -81,12 +81,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -113,12 +108,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot index 06b05832ce8ca..0c376c4d41db2 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot @@ -60,12 +60,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -230,12 +225,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -365,12 +355,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -549,12 +534,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -684,12 +664,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -868,12 +843,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( From 855f0a3f5251fc46796bbeeb7dd30b6f264d1f6b Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Sat, 14 Mar 2026 13:05:21 +0100 Subject: [PATCH 2/4] Turbopack: `require(/* turbopackChunkingType: parallel */` (#91278) 1. Add the CJS version of `import ... with {"turbopack-chunking-type: "parallel"` 2. Allow `shared` as a chunking type 3. Improve error message for invalid values: Bildschirmfoto 2026-03-13 um 15 07 06 --- .../src/analyzer/imports.rs | 93 ++++++++----------- .../turbopack-ecmascript/src/annotations.rs | 8 -- .../crates/turbopack-ecmascript/src/errors.rs | 1 + .../src/references/cjs.rs | 49 ++++++---- .../src/references/esm/base.rs | 25 +++-- .../src/references/mod.rs | 15 ++- .../src/references/util.rs | 64 ++++++++++++- 7 files changed, 163 insertions(+), 92 deletions(-) diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs index c811a44983681..43121028ca389 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs @@ -5,12 +5,7 @@ use rustc_hash::{FxHashMap, FxHashSet}; use smallvec::SmallVec; use swc_core::{ atoms::Wtf8Atom, - common::{ - BytePos, Span, Spanned, SyntaxContext, - comments::Comments, - errors::{DiagnosticId, HANDLER}, - source_map::SmallPos, - }, + common::{BytePos, Span, Spanned, SyntaxContext, comments::Comments, source_map::SmallPos}, ecma::{ ast::*, atoms::{Atom, atom}, @@ -20,15 +15,14 @@ use swc_core::{ }; use turbo_rcstr::{RcStr, rcstr}; use turbo_tasks::{FxIndexMap, FxIndexSet, ResolvedVc}; -use turbopack_core::{ - chunk::ChunkingType, issue::IssueSource, loader::WebpackLoaderItem, source::Source, -}; +use turbopack_core::{issue::IssueSource, loader::WebpackLoaderItem, source::Source}; use super::{JsValue, ModuleValue, top_level_await::has_top_level_await}; use crate::{ SpecifiedModuleType, analyzer::{ConstantValue, ObjectPart}, magic_identifier, + references::util::{SpecifiedChunkingType, parse_chunking_type_annotation}, tree_shake::{PartId, find_turbopack_part_id_in_asserts}, }; @@ -46,16 +40,13 @@ pub struct ImportAnnotations { turbopack_loader: Option, turbopack_rename_as: Option, turbopack_module_type: Option, + chunking_type: Option, } /// Enables a specified transition for the annotated import static ANNOTATION_TRANSITION: Lazy = Lazy::new(|| crate::annotations::ANNOTATION_TRANSITION.into()); -/// Changes the chunking type for the annotated import -static ANNOTATION_CHUNKING_TYPE: Lazy = - Lazy::new(|| crate::annotations::ANNOTATION_CHUNKING_TYPE.into()); - /// Changes the type of the resolved module (only "json" is supported currently) static ATTRIBUTE_MODULE_TYPE: Lazy = Lazy::new(|| atom!("type").into()); @@ -69,6 +60,7 @@ impl ImportAnnotations { serde_json::Map::new(); let mut turbopack_rename_as: Option = None; let mut turbopack_module_type: Option = None; + let mut chunking_type: Option = None; for prop in &with.props { let Some(kv) = prop.as_prop().and_then(|p| p.as_key_value()) else { @@ -76,13 +68,13 @@ impl ImportAnnotations { }; let key_str = match &kv.key { - PropName::Ident(ident) => ident.sym.to_string(), - PropName::Str(str) => str.value.to_string_lossy().into_owned(), + PropName::Ident(ident) => Cow::Borrowed(ident.sym.as_str()), + PropName::Str(str) => str.value.to_string_lossy(), _ => continue, }; // All turbopack* keys are extracted as string values (per TC39 import attributes spec) - match key_str.as_str() { + match &*key_str { "turbopackLoader" => { if let Some(Lit::Str(s)) = kv.value.as_lit() { turbopack_loader_name = @@ -110,6 +102,14 @@ impl ImportAnnotations { Some(RcStr::from(s.value.to_string_lossy().into_owned())); } } + "turbopack-chunking-type" => { + if let Some(Lit::Str(s)) = kv.value.as_lit() { + chunking_type = parse_chunking_type_annotation( + kv.value.span(), + &s.value.to_string_lossy(), + ); + } + } _ => { // For all other keys, only accept string values (per spec) if let Some(Lit::Str(str)) = kv.value.as_lit() { @@ -118,23 +118,6 @@ impl ImportAnnotations { PropName::Str(s) => s.value.clone(), _ => continue, }; - // Validate known annotation values - if key == *ANNOTATION_CHUNKING_TYPE { - let value = str.value.to_string_lossy(); - if value != "parallel" && value != "none" { - HANDLER.with(|handler| { - handler.span_warn_with_code( - kv.value.span(), - &format!( - "unknown turbopack-chunking-type: \"{value}\", \ - expected \"parallel\" or \"none\"" - ), - DiagnosticId::Error("turbopack-chunking-type".into()), - ); - }); - continue; - } - } map.insert(key, str.value.clone()); } } @@ -150,12 +133,14 @@ impl ImportAnnotations { || turbopack_loader.is_some() || turbopack_rename_as.is_some() || turbopack_module_type.is_some() + || chunking_type.is_some() { Some(ImportAnnotations { map, turbopack_loader, turbopack_rename_as, turbopack_module_type, + chunking_type, }) } else { None @@ -193,6 +178,7 @@ impl ImportAnnotations { turbopack_loader: None, turbopack_rename_as: None, turbopack_module_type: None, + chunking_type: None, }) } else { None @@ -205,23 +191,9 @@ impl ImportAnnotations { .map(|v| v.to_string_lossy()) } - /// Returns the chunking type override from the `turbopack-chunking-type` annotation. - /// - /// - `None` — no annotation present - /// - `Some(None)` — annotation is `"none"` (opt out of chunking) - /// - `Some(Some(..))` — explicit chunking type (e.g. `"parallel"`) - /// - /// Unknown values are rejected during [`ImportAnnotations::parse`] and omitted. - pub fn chunking_type(&self) -> Option> { - let chunking_type = self.get(&ANNOTATION_CHUNKING_TYPE)?; - if chunking_type == "none" { - Some(None) - } else { - Some(Some(ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - })) - } + /// Returns the content on the chunking-type annotation + pub fn chunking_type(&self) -> Option { + self.chunking_type } /// Returns the content on the type attribute @@ -362,6 +334,15 @@ pub struct ImportAttributes { /// const { b } = await import(/* turbopackExports: "b" */ "module"); /// ``` pub export_names: Option>, + /// Whether to use a specific chunking type for this import. + // + /// This is set by using a or `turbopackChunkingType` comment. + /// + /// Example: + /// ```js + /// const a = require(/* turbopackChunkingType: parallel */ "a"); + /// ``` + pub chunking_type: Option, } impl ImportAttributes { @@ -370,6 +351,7 @@ impl ImportAttributes { ignore: false, optional: false, export_names: None, + chunking_type: None, } } @@ -942,12 +924,13 @@ fn parse_directives( comments: &dyn Comments, value: Option<&ExprOrSpread>, ) -> Option { - let comment_pos = value.map(|arg| arg.span_lo())?; - let leading_comments = comments.get_leading(comment_pos)?; + let value = value?; + let leading_comments = comments.get_leading(value.span_lo())?; let mut ignore = None; let mut optional = None; let mut export_names = None; + let mut chunking_type = None; // Process all comments, last one wins for each directive type for comment in leading_comments.iter() { @@ -967,17 +950,21 @@ fn parse_directives( "webpackExports" | "turbopackExports" => { export_names = Some(parse_export_names(val)); } + "turbopackChunkingType" => { + chunking_type = parse_chunking_type_annotation(value.span(), val); + } _ => {} // ignore anything else } } } // Return Some only if at least one directive was found - if ignore.is_some() || optional.is_some() || export_names.is_some() { + if ignore.is_some() || optional.is_some() || export_names.is_some() || chunking_type.is_some() { Some(ImportAttributes { ignore: ignore.unwrap_or(false), optional: optional.unwrap_or(false), export_names, + chunking_type, }) } else { None diff --git a/turbopack/crates/turbopack-ecmascript/src/annotations.rs b/turbopack/crates/turbopack-ecmascript/src/annotations.rs index d6c2ba6847ea5..4f5c3536903d3 100644 --- a/turbopack/crates/turbopack-ecmascript/src/annotations.rs +++ b/turbopack/crates/turbopack-ecmascript/src/annotations.rs @@ -9,14 +9,6 @@ pub const ANNOTATION_CHUNKING_TYPE: &str = "turbopack-chunking-type"; /// Enables a specified transition for the annotated import pub const ANNOTATION_TRANSITION: &str = "turbopack-transition"; -pub fn with_chunking_type(chunking_type: &str) -> Box { - with_clause(&[(ANNOTATION_CHUNKING_TYPE, chunking_type)]) -} - -pub fn with_transition(transition_name: &str) -> Box { - with_clause(&[(ANNOTATION_TRANSITION, transition_name)]) -} - pub fn with_clause<'a>( entries: impl IntoIterator, ) -> Box { diff --git a/turbopack/crates/turbopack-ecmascript/src/errors.rs b/turbopack/crates/turbopack-ecmascript/src/errors.rs index c762cabe46f04..29312eebefd3e 100644 --- a/turbopack/crates/turbopack-ecmascript/src/errors.rs +++ b/turbopack/crates/turbopack-ecmascript/src/errors.rs @@ -19,5 +19,6 @@ pub mod failed_to_analyze { pub const NEW_WORKER: &str = "TP1203"; pub const MODULE_HOT_ACCEPT: &str = "TP1204"; pub const MODULE_HOT_DECLINE: &str = "TP1205"; + pub const CHUNKING_TYPE: &str = "TP1206"; } } diff --git a/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs b/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs index 0f6e97278ecc5..a0ad52a68c1d1 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs @@ -23,6 +23,7 @@ use crate::{ references::{ AstPath, pattern_mapping::{PatternMapping, ResolveType}, + util::SpecifiedChunkingType, }, runtime_functions::TURBOPACK_CACHE, }; @@ -80,10 +81,11 @@ impl ModuleReference for CjsAssetReference { #[derive(Hash, Debug, ValueToString)] #[value_to_string("require {request}")] pub struct CjsRequireAssetReference { - pub origin: ResolvedVc>, - pub request: ResolvedVc, - pub issue_source: IssueSource, - pub error_mode: ResolveErrorMode, + origin: ResolvedVc>, + request: ResolvedVc, + issue_source: IssueSource, + error_mode: ResolveErrorMode, + chunking_type_attribute: Option, } impl CjsRequireAssetReference { @@ -92,12 +94,14 @@ impl CjsRequireAssetReference { request: ResolvedVc, issue_source: IssueSource, error_mode: ResolveErrorMode, + chunking_type_attribute: Option, ) -> Self { CjsRequireAssetReference { origin, request, issue_source, error_mode, + chunking_type_attribute, } } } @@ -116,10 +120,15 @@ impl ModuleReference for CjsRequireAssetReference { } fn chunking_type(&self) -> Option { - Some(ChunkingType::Parallel { - inherit_async: false, - hoisted: false, - }) + self.chunking_type_attribute.map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: false, + hoisted: false, + }) + }, + |c| c.as_chunking_type(false, false), + ) } } @@ -202,10 +211,11 @@ impl CjsRequireAssetReferenceCodeGen { #[derive(Hash, Debug, ValueToString)] #[value_to_string("require.resolve {request}")] pub struct CjsRequireResolveAssetReference { - pub origin: ResolvedVc>, - pub request: ResolvedVc, - pub issue_source: IssueSource, - pub error_mode: ResolveErrorMode, + origin: ResolvedVc>, + request: ResolvedVc, + issue_source: IssueSource, + error_mode: ResolveErrorMode, + chunking_type_attribute: Option, } impl CjsRequireResolveAssetReference { @@ -214,12 +224,14 @@ impl CjsRequireResolveAssetReference { request: ResolvedVc, issue_source: IssueSource, error_mode: ResolveErrorMode, + chunking_type_attribute: Option, ) -> Self { CjsRequireResolveAssetReference { origin, request, issue_source, error_mode, + chunking_type_attribute, } } } @@ -238,10 +250,15 @@ impl ModuleReference for CjsRequireResolveAssetReference { } fn chunking_type(&self) -> Option { - Some(ChunkingType::Parallel { - inherit_async: false, - hoisted: false, - }) + self.chunking_type_attribute.map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: false, + hoisted: false, + }) + }, + |c| c.as_chunking_type(false, false), + ) } } diff --git a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs index ac3648234200b..ace40d3146a2a 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs @@ -46,7 +46,7 @@ use crate::{ EsmExport, export::{all_known_export_names, is_export_missing}, }, - util::throw_module_not_found_expr, + util::{SpecifiedChunkingType, throw_module_not_found_expr}, }, runtime_functions::{TURBOPACK_EXTERNAL_IMPORT, TURBOPACK_EXTERNAL_REQUIRE, TURBOPACK_IMPORT}, tree_shake::{TURBOPACK_PART_IMPORT_SOURCE, part::module::EcmascriptModulePartAsset}, @@ -509,10 +509,15 @@ impl ModuleReference for EsmAssetReference { self.annotations .as_ref() .and_then(|a| a.chunking_type()) - .unwrap_or(Some(ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - })) + .map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: true, + hoisted: true, + }) + }, + |c| c.as_chunking_type(true, true), + ) } fn binding_usage(&self) -> BindingUsage { @@ -544,10 +549,12 @@ impl EsmAssetReference { } // only chunked references can be imported - if !matches!( - this.annotations.as_ref().and_then(|a| a.chunking_type()), - Some(None) - ) { + if this + .annotations + .as_ref() + .and_then(|a| a.chunking_type()) + .is_none_or(|v| v != SpecifiedChunkingType::None) + { let import_externals = this.import_externals; let referenced_asset = self.get_referenced_asset().await?; diff --git a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs index bb3c052b69d96..6da5f4f39a2d6 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs @@ -1515,10 +1515,15 @@ async fn analyze_ecmascript_module_internal( .annotations .as_ref() .and_then(|a| a.chunking_type()) - .unwrap_or(Some(ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - })); + .map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: true, + hoisted: true, + }) + }, + |c| c.as_chunking_type(true, true), + ); analysis.add_reference_code_gen( EsmModuleIdAssetReference::new(*r, chunking_type), ast_path.into(), @@ -2262,6 +2267,7 @@ where Request::parse(pat).to_resolved().await?, issue_source(source, span), error_mode, + attributes.chunking_type, ), ast_path.to_vec().into(), ); @@ -2315,6 +2321,7 @@ where Request::parse(pat).to_resolved().await?, issue_source(source, span), error_mode, + attributes.chunking_type, ), ast_path.to_vec().into(), ); diff --git a/turbopack/crates/turbopack-ecmascript/src/references/util.rs b/turbopack/crates/turbopack-ecmascript/src/references/util.rs index 1a263fc36b73b..ae269d6926b45 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/util.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/util.rs @@ -1,10 +1,19 @@ use anyhow::Result; -use swc_core::{ecma::ast::Expr, quote}; +use bincode::{Decode, Encode}; +use swc_core::{ + common::{ + Span, + errors::{DiagnosticId, HANDLER}, + }, + ecma::ast::Expr, + quote, +}; use turbo_rcstr::{RcStr, rcstr}; -use turbo_tasks::{ResolvedVc, Vc, turbofmt}; +use turbo_tasks::{NonLocalValue, ResolvedVc, Vc, trace::TraceRawVcs, turbofmt}; use turbo_tasks_fs::FileSystemPath; use turbopack_core::{ self, + chunk::ChunkingType, issue::{ Issue, IssueExt, IssueSeverity, IssueSource, IssueStage, OptionIssueSource, OptionStyledString, StyledString, @@ -12,6 +21,8 @@ use turbopack_core::{ resolve::{ModuleResolveResult, parse::Request, pattern::Pattern}, }; +use crate::errors; + /// Creates a IIFE expression that throws a "Cannot find module" error for the /// given request string pub fn throw_module_not_found_expr(request: &str) -> Expr { @@ -132,3 +143,52 @@ impl Issue for TooManyMatchesWarning { Vc::cell(Some(self.source)) } } + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Encode, Decode, TraceRawVcs, NonLocalValue)] +pub enum SpecifiedChunkingType { + Parallel, + Shared, + None, +} + +impl SpecifiedChunkingType { + pub fn as_chunking_type(&self, inherit_async: bool, hoisted: bool) -> Option { + match self { + SpecifiedChunkingType::Parallel => Some(ChunkingType::Parallel { + inherit_async, + hoisted, + }), + SpecifiedChunkingType::Shared => Some(ChunkingType::Shared { + inherit_async, + merge_tag: None, + }), + SpecifiedChunkingType::None => None, + } + } +} + +pub fn parse_chunking_type_annotation( + span: Span, + chunking_type_annotation: &str, +) -> Option { + match chunking_type_annotation { + "parallel" => Some(SpecifiedChunkingType::Parallel), + "shared" => Some(SpecifiedChunkingType::Shared), + "none" => Some(SpecifiedChunkingType::None), + _ => { + HANDLER.with(|handler| { + handler.span_err_with_code( + span, + &format!( + "Unknown specified chunking-type: \"{chunking_type_annotation}\", \ + expected \"parallel\", \"shared\" or \"none\"" + ), + DiagnosticId::Error( + errors::failed_to_analyze::ecmascript::CHUNKING_TYPE.into(), + ), + ); + }); + None + } + } +} From 83e47acd77eeae03aa251ce5ab2fe5323a4b8c42 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 14 Mar 2026 08:38:36 -0400 Subject: [PATCH 3/4] Simplify scroll restoration with shared ScrollRef on CacheNode (#91348) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the segment-path-matching scroll system with a simpler model based on a shared mutable ScrollRef on CacheNode. The old system accumulated segment paths during navigation and matched them in layout-router to decide which segments should scroll. This was necessary when CacheNodes were created lazily during render. Now that we construct the entire CacheNode tree immediately upon navigation, we can assign a shared ScrollRef directly to each new leaf node. When any segment scrolls, it flips the ref to false, preventing other segments from also scrolling. This removes all the segment path accumulation and matching logic. Fixes a regression where calling `refresh()` from a server action scrolled the page to the top. The old system had a semantic gap between `null` (no segments) and `[]` (scroll everything) — a server action refresh with no new segments fell through to a path that scrolled unconditionally. The new model avoids this: refresh creates no new CacheNodes, so no ScrollRef is assigned, and nothing scrolls. Repro: https://github.com/stipsan/nextjs-refresh-regression-repro There is extensive existing test coverage for scroll restoration behavior. This adds one additional test for the server action refresh bug. --- packages/next/src/client/app-dir/link.tsx | 3 +- .../client/components/app-router-instance.ts | 20 +- .../src/client/components/layout-router.tsx | 324 ++++++++---------- .../create-initial-router-state.ts | 4 +- .../router-reducer/ppr-navigations.ts | 139 ++++---- .../reducers/navigate-reducer.ts | 4 +- .../reducers/refresh-reducer.ts | 5 +- .../reducers/restore-reducer.ts | 2 +- .../reducers/server-action-reducer.ts | 7 +- .../reducers/server-patch-reducer.ts | 5 +- .../router-reducer/router-reducer-types.ts | 39 ++- .../components/segment-cache/navigation.ts | 130 ++++--- .../next/src/shared/lib/app-router-types.ts | 17 + .../browser-logs/browser-logs.test.ts | 4 +- .../app/server-action-refresh/actions.ts | 7 + .../app/server-action-refresh/page.tsx | 21 ++ .../router-autoscroll.test.ts | 27 ++ 17 files changed, 445 insertions(+), 313 deletions(-) create mode 100644 test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts create mode 100644 test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx diff --git a/packages/next/src/client/app-dir/link.tsx b/packages/next/src/client/app-dir/link.tsx index 11646bc5cb0fe..93e0e73c10ed0 100644 --- a/packages/next/src/client/app-dir/link.tsx +++ b/packages/next/src/client/app-dir/link.tsx @@ -8,6 +8,7 @@ import { useMergedRef } from '../use-merged-ref' import { isAbsoluteUrl } from '../../shared/lib/utils' import { addBasePath } from '../add-base-path' import { warnOnce } from '../../shared/lib/utils/warn-once' +import { ScrollBehavior } from '../components/router-reducer/router-reducer-types' import type { PENDING_LINK_STATUS } from '../components/links' import { IDLE_LINK_STATUS, @@ -307,7 +308,7 @@ function linkClicked( dispatchNavigateAction( href, replace ? 'replace' : 'push', - scroll ?? true, + scroll === false ? ScrollBehavior.NoScroll : ScrollBehavior.Default, linkInstanceRef.current, transitionTypes ) diff --git a/packages/next/src/client/components/app-router-instance.ts b/packages/next/src/client/components/app-router-instance.ts index 606aed344eeec..c62f20d84de7a 100644 --- a/packages/next/src/client/components/app-router-instance.ts +++ b/packages/next/src/client/components/app-router-instance.ts @@ -9,6 +9,7 @@ import { type NavigateAction, ACTION_HMR_REFRESH, PrefetchKind, + ScrollBehavior, type AppHistoryState, } from './router-reducer/router-reducer-types' import { reducer } from './router-reducer/router-reducer' @@ -277,7 +278,7 @@ function getProfilingHookForOnNavigationStart() { export function dispatchNavigateAction( href: string, navigateType: NavigateAction['navigateType'], - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, linkInstanceRef: LinkInstance | null, transitionTypes: string[] | undefined ): void { @@ -307,7 +308,7 @@ export function dispatchNavigateAction( url, isExternalUrl: isExternalURL(url), locationSearch: location.search, - shouldScroll, + scrollBehavior, navigateType, }) } @@ -356,7 +357,10 @@ function gesturePush(href: string, options?: NavigateOptions): void { // Fork the router state for the duration of the gesture transition. const currentUrl = new URL(state.canonicalUrl, location.href) - const shouldScroll = options?.scroll ?? true + const scrollBehavior = + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default // This is a special freshness policy that prevents dynamic requests from // being spawned. During the gesture, we should only show the cached // prefetched UI, not dynamic data. @@ -373,7 +377,7 @@ function gesturePush(href: string, options?: NavigateOptions): void { state.tree, state.nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, 'push' ) dispatchGestureState(forkedGestureState) @@ -442,7 +446,9 @@ export const publicAppRouterInstance: AppRouterInstance = { dispatchNavigateAction( href, 'replace', - options?.scroll ?? true, + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default, null, options?.transitionTypes ) @@ -458,7 +464,9 @@ export const publicAppRouterInstance: AppRouterInstance = { dispatchNavigateAction( href, 'push', - options?.scroll ?? true, + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default, null, options?.transitionTypes ) diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx index 9bc1614a88e81..d613c40f6900d 100644 --- a/packages/next/src/client/components/layout-router.tsx +++ b/packages/next/src/client/components/layout-router.tsx @@ -30,7 +30,6 @@ import { } from '../../shared/lib/app-router-context.shared-runtime' import { unresolvedThenable } from './unresolved-thenable' import { ErrorBoundary } from './error-boundary' -import { matchSegment } from './match-segments' import { disableSmoothScrollDuringRouteTransition } from '../../shared/lib/router/utils/disable-smooth-scroll' import { RedirectBoundary } from './redirect-boundary' import { HTTPAccessFallbackBoundary } from './http-access-fallback/error-boundary' @@ -142,76 +141,173 @@ function getHashFragmentDomNode(hashFragment: string) { interface ScrollAndMaybeFocusHandlerProps { focusAndScrollRef: FocusAndScrollRef children: React.ReactNode - segmentPath: FlightSegmentPath + cacheNode: CacheNode } class InnerScrollAndFocusHandlerOld extends React.Component { handlePotentialScroll = () => { // Handle scroll and focus, it's only applied once. - const { focusAndScrollRef, segmentPath } = this.props + const { focusAndScrollRef, cacheNode } = this.props - if (focusAndScrollRef.apply) { - // segmentPaths is an array of segment paths that should be scrolled to - // if the current segment path is not in the array, the scroll is not applied - // unless the array is empty, in which case the scroll is always applied - if ( - focusAndScrollRef.segmentPaths.length !== 0 && - !focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) => - segmentPath.every((segment, index) => - matchSegment(segment, scrollRefSegmentPath[index]) - ) - ) - ) { + const scrollRef = focusAndScrollRef.forceScroll + ? focusAndScrollRef.scrollRef + : cacheNode.scrollRef + if (scrollRef === null || !scrollRef.current) return + + let domNode: + | ReturnType + | ReturnType = null + const hashFragment = focusAndScrollRef.hashFragment + + if (hashFragment) { + domNode = getHashFragmentDomNode(hashFragment) + } + + // `findDOMNode` is tricky because it returns just the first child if the component is a fragment. + // This already caused a bug where the first child was a in head. + if (!domNode) { + domNode = findDOMNode(this) + } + + // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. + if (!(domNode instanceof Element)) { + return + } + + // Verify if the element is a HTMLElement and if we want to consider it for scroll behavior. + // If the element is skipped, try to select the next sibling and try again. + while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) { + if (process.env.NODE_ENV !== 'production') { + if (domNode.parentElement?.localName === 'head') { + // We enter this state when metadata was rendered as part of the page or via Next.js. + // This is always a bug in Next.js and caused by React hoisting metadata. + // Fixed with `experimental.appNewScrollHandler` + } + } + + // No siblings found that match the criteria are found, so handle scroll higher up in the tree instead. + if (domNode.nextElementSibling === null) { return } + domNode = domNode.nextElementSibling + } + + // Mark as scrolled so no other segment scrolls for this navigation. + scrollRef.current = false + + disableSmoothScrollDuringRouteTransition( + () => { + // In case of hash scroll, we only need to scroll the element into view + if (hashFragment) { + domNode.scrollIntoView() + + return + } + // Store the current viewport height because reading `clientHeight` causes a reflow, + // and it won't change during this function. + const htmlElement = document.documentElement + const viewportHeight = htmlElement.clientHeight + + // If the element's top edge is already in the viewport, exit early. + if (topOfElementInViewport(domNode, viewportHeight)) { + return + } + + // Otherwise, try scrolling go the top of the document to be backward compatible with pages + // scrollIntoView() called on `` element scrolls horizontally on chrome and firefox (that shouldn't happen) + // We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left + // scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically + htmlElement.scrollTop = 0 + + // Scroll to domNode if domNode is not in viewport when scrolled to top of document + if (!topOfElementInViewport(domNode, viewportHeight)) { + // Scroll into view doesn't scroll horizontally by default when not needed + domNode.scrollIntoView() + } + }, + { + // We will force layout by querying domNode position + dontForceLayout: true, + onlyHashChange: focusAndScrollRef.onlyHashChange, + } + ) + + // Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition` + focusAndScrollRef.onlyHashChange = false + focusAndScrollRef.hashFragment = null + + // Set focus on the element + domNode.focus() + } + + componentDidMount() { + this.handlePotentialScroll() + } + + componentDidUpdate() { + this.handlePotentialScroll() + } + + render() { + return this.props.children + } +} + +/** + * Fork of InnerScrollAndFocusHandlerOld using Fragment refs for scrolling. + * No longer focuses the first host descendant. + */ +function InnerScrollHandlerNew(props: ScrollAndMaybeFocusHandlerProps) { + const childrenRef = React.useRef(null) + + useLayoutEffect( + () => { + const { focusAndScrollRef, cacheNode } = props - let domNode: - | ReturnType - | ReturnType = null + const scrollRef = focusAndScrollRef.forceScroll + ? focusAndScrollRef.scrollRef + : cacheNode.scrollRef + if (scrollRef === null || !scrollRef.current) return + + let instance: FragmentInstance | HTMLElement | null = null const hashFragment = focusAndScrollRef.hashFragment if (hashFragment) { - domNode = getHashFragmentDomNode(hashFragment) + instance = getHashFragmentDomNode(hashFragment) } - // `findDOMNode` is tricky because it returns just the first child if the component is a fragment. - // This already caused a bug where the first child was a in head. - if (!domNode) { - domNode = findDOMNode(this) + if (!instance) { + instance = childrenRef.current } // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. - if (!(domNode instanceof Element)) { + if (instance === null) { return } - // Verify if the element is a HTMLElement and if we want to consider it for scroll behavior. - // If the element is skipped, try to select the next sibling and try again. - while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) { - if (process.env.NODE_ENV !== 'production') { - if (domNode.parentElement?.localName === 'head') { - // We enter this state when metadata was rendered as part of the page or via Next.js. - // This is always a bug in Next.js and caused by React hoisting metadata. - // Fixed with `experimental.appNewScrollHandler` - } - } + // Mark as scrolled so no other segment scrolls for this navigation. + scrollRef.current = false - // No siblings found that match the criteria are found, so handle scroll higher up in the tree instead. - if (domNode.nextElementSibling === null) { - return - } - domNode = domNode.nextElementSibling + const activeElement = document.activeElement + if ( + activeElement !== null && + 'blur' in activeElement && + typeof activeElement.blur === 'function' + ) { + // Trying to match hard navigations. + // Ideally we'd move the internal focus cursor either to the top + // or at least before the segment. But there's no DOM API to do that, + // so we just blur. + // We could workaround this by moving focus to a temporary element in + // the body. But adding elements might trigger layout or other effects + // so it should be well motivated. + activeElement.blur() } - // State is mutated to ensure that the focus and scroll is applied only once. - focusAndScrollRef.apply = false - focusAndScrollRef.hashFragment = null - focusAndScrollRef.segmentPaths = [] - disableSmoothScrollDuringRouteTransition( () => { // In case of hash scroll, we only need to scroll the element into view if (hashFragment) { - domNode.scrollIntoView() + instance.scrollIntoView() return } @@ -221,7 +317,7 @@ class InnerScrollAndFocusHandlerOld extends React.Component(null) - - useLayoutEffect( - () => { - const { focusAndScrollRef, segmentPath } = props - // Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed. - - if (focusAndScrollRef.apply) { - // segmentPaths is an array of segment paths that should be scrolled to - // if the current segment path is not in the array, the scroll is not applied - // unless the array is empty, in which case the scroll is always applied - if ( - focusAndScrollRef.segmentPaths.length !== 0 && - !focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) => - segmentPath.every((segment, index) => - matchSegment(segment, scrollRefSegmentPath[index]) - ) - ) - ) { - return - } - - let instance: FragmentInstance | HTMLElement | null = null - const hashFragment = focusAndScrollRef.hashFragment - - if (hashFragment) { - instance = getHashFragmentDomNode(hashFragment) - } - - if (!instance) { - instance = childrenRef.current - } - - // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. - if (instance === null) { - return - } - - // State is mutated to ensure that the focus and scroll is applied only once. - focusAndScrollRef.apply = false - focusAndScrollRef.hashFragment = null - focusAndScrollRef.segmentPaths = [] - - const activeElement = document.activeElement - if ( - activeElement !== null && - 'blur' in activeElement && - typeof activeElement.blur === 'function' - ) { - // Trying to match hard navigations. - // Ideally we'd move the internal focus cursor either to the top - // or at least before the segment. But there's no DOM API to do that, - // so we just blur. - // We could workaround this by moving focus to a temporary element in - // the body. But adding elements might trigger layout or other effects - // so it should be well motivated. - activeElement.blur() - } - - disableSmoothScrollDuringRouteTransition( - () => { - // In case of hash scroll, we only need to scroll the element into view - if (hashFragment) { - instance.scrollIntoView() - - return - } - // Store the current viewport height because reading `clientHeight` causes a reflow, - // and it won't change during this function. - const htmlElement = document.documentElement - const viewportHeight = htmlElement.clientHeight - - // If the element's top edge is already in the viewport, exit early. - if (topOfElementInViewport(instance, viewportHeight)) { - return - } - - // Otherwise, try scrolling go the top of the document to be backward compatible with pages - // scrollIntoView() called on `` element scrolls horizontally on chrome and firefox (that shouldn't happen) - // We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left - // scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically - htmlElement.scrollTop = 0 - - // Scroll to domNode if domNode is not in viewport when scrolled to top of document - if (!topOfElementInViewport(instance, viewportHeight)) { - // Scroll into view doesn't scroll horizontally by default when not needed - instance.scrollIntoView() - } - }, - { - // We will force layout by querying domNode position - dontForceLayout: true, - onlyHashChange: focusAndScrollRef.onlyHashChange, - } - ) - - // Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition` - focusAndScrollRef.onlyHashChange = false - } + focusAndScrollRef.hashFragment = null }, // Used to run on every commit. We may be able to be smarter about this // but be prepared for lots of manual testing. @@ -383,11 +357,11 @@ const InnerScrollAndMaybeFocusHandler = enableNewScrollHandler : InnerScrollAndFocusHandlerOld function ScrollAndMaybeFocusHandler({ - segmentPath, children, + cacheNode, }: { - segmentPath: FlightSegmentPath children: React.ReactNode + cacheNode: CacheNode }) { const context = useContext(GlobalLayoutRouterContext) if (!context) { @@ -396,8 +370,8 @@ function ScrollAndMaybeFocusHandler({ return ( {children} @@ -787,7 +761,7 @@ export default function OuterLayoutRouter({ const debugNameToDisplay = isVirtual ? undefined : debugNameContext let templateValue = ( - + | null separateRefreshUrls: Set | null + /** + * Set when a navigation creates new leaf segments that should be + * scrolled to. Stays null when no new segments are created (e.g. + * during a refresh where the route structure didn't change). + */ + scrollRef: ScrollRef | null } const noop = () => {} @@ -133,8 +137,8 @@ export function createInitialCacheNodeForHydration( // Create the initial cache node tree, using the data embedded into the // HTML document. const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } const task = createCacheNodeOnNavigation( navigatedAt, @@ -143,8 +147,6 @@ export function createInitialCacheNodeForHydration( FreshnessPolicy.Hydration, seedData, seedHead, - null, - null, false, accumulation ) @@ -213,8 +215,6 @@ export function startPPRNavigation( seedData, seedHead, isSamePageNavigation, - null, - null, parentNeedsDynamicRequest, oldRootRefreshState, parentRefreshState, @@ -234,8 +234,6 @@ function updateCacheNodeOnNavigation( seedData: CacheNodeSeedData | null, seedHead: HeadData | null, isSamePageNavigation: boolean, - parentSegmentPath: FlightSegmentPath | null, - parentParallelRouteKey: string | null, parentNeedsDynamicRequest: boolean, oldRootRefreshState: RefreshState, parentRefreshState: RefreshState | null, @@ -285,12 +283,6 @@ function updateCacheNodeOnNavigation( ) { return null } - if (parentSegmentPath === null || parentParallelRouteKey === null) { - // The root should never mismatch. If it does, it suggests an internal - // Next.js error, or a malformed server response. Trigger a full- - // page navigation. - return null - } return createCacheNodeOnNavigation( navigatedAt, newRouteTree, @@ -298,24 +290,11 @@ function updateCacheNodeOnNavigation( freshness, seedData, seedHead, - parentSegmentPath, - parentParallelRouteKey, parentNeedsDynamicRequest, accumulation ) } - // TODO: The segment paths are tracked so that LayoutRouter knows which - // segments to scroll to after a navigation. But we should just mark this - // information on the CacheNode directly. It used to be necessary to do this - // separately because CacheNodes were created lazily during render, not when - // rather than when creating the route tree. - const segmentPath = - parentParallelRouteKey !== null && parentSegmentPath !== null - ? parentSegmentPath.concat([parentParallelRouteKey, newSegment]) - : // NOTE: The root segment is intentionally omitted from the segment path - [] - const newSlots = newRouteTree.slots const oldRouterStateChildren = oldRouterState[1] const seedDataChildren = seedData !== null ? seedData[1] : null @@ -331,10 +310,8 @@ function updateCacheNodeOnNavigation( switch (freshness) { case FreshnessPolicy.Default: case FreshnessPolicy.HistoryTraversal: - case FreshnessPolicy.Hydration: // <- shouldn't happen during client nav + case FreshnessPolicy.Hydration: case FreshnessPolicy.Gesture: - // We should never drop dynamic data in shared layouts, except during - // a refresh. shouldRefreshDynamicData = false break case FreshnessPolicy.RefreshAll: @@ -383,6 +360,15 @@ function updateCacheNodeOnNavigation( ) newCacheNode = result.cacheNode needsDynamicRequest = result.needsDynamicRequest + + // Carry forward the old node's scrollRef. This preserves scroll + // intent when a prior navigation's cache node is replaced by a + // refresh before the scroll handler has had a chance to fire — + // e.g. when router.push() and router.refresh() are called in the + // same startTransition batch. + if (oldCacheNode !== undefined) { + newCacheNode.scrollRef = oldCacheNode.scrollRef + } } // During a refresh navigation, there's a special case that happens when @@ -501,8 +487,6 @@ function updateCacheNodeOnNavigation( seedDataChild ?? null, seedHeadChild, isSamePageNavigation, - segmentPath, - parallelRouteKey, parentNeedsDynamicRequest || needsDynamicRequest, oldRootRefreshState, refreshState, @@ -565,6 +549,50 @@ function updateCacheNodeOnNavigation( } } +/** + * Assigns a ScrollRef to a new leaf CacheNode so the scroll handler + * knows to scroll to it after navigation. All leaves in the same + * navigation share the same ScrollRef — the first segment to scroll + * consumes it, preventing others from also scrolling. + * + * This is only called inside `createCacheNodeOnNavigation`, which only + * runs when segments diverge from the previous route. So for a refresh + * where the route structure stays the same, segments match, the update + * path is taken, and this function is never called — no scroll ref is + * assigned. A scroll ref is only assigned when the route actually + * changed (e.g. a redirect, or a dynamic condition on the server that + * produces a different route). + * + * Skipped during hydration (initial render should not scroll) and + * history traversal (scroll restoration is handled separately). + */ +function accumulateScrollRef( + freshness: FreshnessPolicy, + cacheNode: CacheNode, + accumulation: NavigationRequestAccumulation +): void { + switch (freshness) { + case FreshnessPolicy.Default: + case FreshnessPolicy.Gesture: + case FreshnessPolicy.RefreshAll: + case FreshnessPolicy.HMRRefresh: + if (accumulation.scrollRef === null) { + accumulation.scrollRef = { current: true } + } + cacheNode.scrollRef = accumulation.scrollRef + break + case FreshnessPolicy.Hydration: + // Initial render — no scroll. + break + case FreshnessPolicy.HistoryTraversal: + // Back/forward — scroll restoration is handled separately. + break + default: + freshness satisfies never + break + } +} + function createCacheNodeOnNavigation( navigatedAt: number, newRouteTree: RouteTree, @@ -572,8 +600,6 @@ function createCacheNodeOnNavigation( freshness: FreshnessPolicy, seedData: CacheNodeSeedData | null, seedHead: HeadData | null, - parentSegmentPath: FlightSegmentPath | null, - parentParallelRouteKey: string | null, parentNeedsDynamicRequest: boolean, accumulation: NavigationRequestAccumulation ): NavigationTask { @@ -588,33 +614,10 @@ function createCacheNodeOnNavigation( // diverges, which is why we keep them separate. const newSegment = createSegmentFromRouteTree(newRouteTree) - const segmentPath = - parentParallelRouteKey !== null && parentSegmentPath !== null - ? parentSegmentPath.concat([parentParallelRouteKey, newSegment]) - : // NOTE: The root segment is intentionally omitted from the segment path - [] const newSlots = newRouteTree.slots const seedDataChildren = seedData !== null ? seedData[1] : null - const isLeafSegment = newSlots === null - - if (isLeafSegment) { - // The segment path of every leaf segment (i.e. page) is collected into - // a result array. This is used by the LayoutRouter to scroll to ensure that - // new pages are visible after a navigation. - // - // This only happens for new pages, not for refreshed pages. - // - // TODO: We should use a string to represent the segment path instead of - // an array. We already use a string representation for the path when - // accessing the Segment Cache, so we can use the same one. - if (accumulation.scrollableSegments === null) { - accumulation.scrollableSegments = [] - } - accumulation.scrollableSegments.push(segmentPath) - } - const seedRsc = seedData !== null ? seedData[0] : null const result = createCacheNodeForSegment( navigatedAt, @@ -627,6 +630,11 @@ function createCacheNodeOnNavigation( const newCacheNode = result.cacheNode const needsDynamicRequest = result.needsDynamicRequest + const isLeafSegment = newSlots === null + if (isLeafSegment) { + accumulateScrollRef(freshness, newCacheNode, accumulation) + } + let patchedRouterStateChildren: { [parallelRouteKey: string]: FlightRouterState } = {} @@ -653,8 +661,6 @@ function createCacheNodeOnNavigation( freshness, seedDataChild ?? null, seedHead, - segmentPath, - parallelRouteKey, parentNeedsDynamicRequest || needsDynamicRequest, accumulation ) @@ -858,12 +864,15 @@ function reuseSharedCacheNode( dropPrefetchRsc: boolean, existingCacheNode: CacheNode ): CacheNode { - // Clone the CacheNode that was already present in the previous tree + // Clone the CacheNode that was already present in the previous tree. + // Carry forward the scrollRef so scroll intent from a prior navigation + // survives tree rebuilds (e.g. push + refresh in the same batch). return createCacheNode( existingCacheNode.rsc, dropPrefetchRsc ? null : existingCacheNode.prefetchRsc, existingCacheNode.head, - dropPrefetchRsc ? null : existingCacheNode.prefetchHead + dropPrefetchRsc ? null : existingCacheNode.prefetchHead, + existingCacheNode.scrollRef ) } @@ -1191,7 +1200,8 @@ function createCacheNode( rsc: React.ReactNode | null, prefetchRsc: React.ReactNode | null, head: React.ReactNode | null, - prefetchHead: HeadData | null + prefetchHead: HeadData | null, + scrollRef: ScrollRef | null = null ): CacheNode { return { rsc, @@ -1199,6 +1209,7 @@ function createCacheNode( head, prefetchHead, slots: null, + scrollRef, } } diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index eb4184248fa2d..b41483d221676 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -24,7 +24,7 @@ export function navigateReducer( state: ReadonlyReducerState, action: NavigateAction ): ReducerState { - const { url, isExternalUrl, navigateType, shouldScroll } = action + const { url, isExternalUrl, navigateType, scrollBehavior } = action if (isExternalUrl) { return completeHardNavigation(state, url, navigateType) @@ -50,7 +50,7 @@ export function navigateReducer( state.tree, state.nextUrl, FreshnessPolicy.Default, - shouldScroll, + scrollBehavior, navigateType ) } diff --git a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts index 483f65f861697..b742da8f63692 100644 --- a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts @@ -3,6 +3,7 @@ import type { ReducerState, RefreshAction, } from '../router-reducer-types' +import { ScrollBehavior } from '../router-reducer-types' import { convertServerPatchToFullTree, navigateToKnownRoute, @@ -56,7 +57,7 @@ export function refreshDynamicData( const currentUrl = new URL(currentCanonicalUrl, location.origin) const currentRenderedSearch = state.renderedSearch const currentFlightRouterState = state.tree - const shouldScroll = false + const scrollBehavior = ScrollBehavior.NoScroll // Create a NavigationSeed from the current FlightRouterState. // TODO: Eventually we will store this type directly on the state object @@ -82,7 +83,7 @@ export function refreshDynamicData( currentFlightRouterState, freshnessPolicy, nextUrlForRefresh, - shouldScroll, + scrollBehavior, navigateType, null, // Refresh navigations don't use route prediction, so there's no route diff --git a/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts index 45aa87c933dd0..bc53333c10b48 100644 --- a/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts @@ -45,8 +45,8 @@ export function restoreReducer( const now = Date.now() const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } const restoreSeed = convertServerPatchToFullTree( treeToRestore, diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts index b76833e13e527..77dfea089bb9e 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts @@ -29,6 +29,7 @@ import type { ReducerState, ServerActionAction, } from '../router-reducer-types' +import { ScrollBehavior } from '../router-reducer-types' import { assignLocation } from '../../../assign-location' import { createHrefFromUrl } from '../create-href-from-url' import { hasInterceptionRouteInCurrentTree } from './has-interception-route-in-current-tree' @@ -416,7 +417,7 @@ export function serverActionReducer( const redirectUrl = redirectLocation !== undefined ? redirectLocation : currentUrl const currentFlightRouterState = state.tree - const shouldScroll = true + const scrollBehavior = ScrollBehavior.Default // If the action triggered a revalidation of the cache, we should also // refresh all the dynamic data. @@ -472,7 +473,7 @@ export function serverActionReducer( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, null, // Server action redirects don't use route prediction - we already @@ -494,7 +495,7 @@ export function serverActionReducer( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) }, diff --git a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts index c7e42d3e3609a..a1505474cec69 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts @@ -4,6 +4,7 @@ import { type ServerPatchAction, type ReducerState, type ReadonlyReducerState, + ScrollBehavior, } from '../router-reducer-types' import { completeHardNavigation, @@ -41,7 +42,7 @@ export function serverPatchReducer( // using the tree we just received from the server. const retryCanonicalUrl = createHrefFromUrl(retryUrl) const retryNextUrl = action.nextUrl - const shouldScroll = true + const scrollBehavior = ScrollBehavior.Default const now = Date.now() return navigateToKnownRoute( now, @@ -55,7 +56,7 @@ export function serverPatchReducer( state.tree, FreshnessPolicy.RefreshAll, retryNextUrl, - shouldScroll, + scrollBehavior, navigateType, null, // Server patch (retry) navigations don't use route prediction. This is diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/packages/next/src/client/components/router-reducer/router-reducer-types.ts index f6d746d034093..e52a02b9236bc 100644 --- a/packages/next/src/client/components/router-reducer/router-reducer-types.ts +++ b/packages/next/src/client/components/router-reducer/router-reducer-types.ts @@ -1,8 +1,5 @@ -import type { CacheNode } from '../../../shared/lib/app-router-types' -import type { - FlightRouterState, - FlightSegmentPath, -} from '../../../shared/lib/app-router-types' +import type { CacheNode, ScrollRef } from '../../../shared/lib/app-router-types' +import type { FlightRouterState } from '../../../shared/lib/app-router-types' import type { NavigationSeed } from '../segment-cache/navigation' import type { FetchServerResponseResult } from './fetch-server-response' @@ -94,7 +91,7 @@ export interface NavigateAction { isExternalUrl: boolean locationSearch: Location['search'] navigateType: 'push' | 'replace' - shouldScroll: boolean + scrollBehavior: ScrollBehavior } /** @@ -163,19 +160,37 @@ export interface PushRef { preserveCustomHistoryState: boolean } +/** + * Controls the scroll behavior for a navigation. + */ +export const enum ScrollBehavior { + /** Use per-node ScrollRef to decide whether to scroll. */ + Default = 0, + /** Suppress scroll entirely (e.g. scroll={false} on Link or router.push). */ + NoScroll = 1, +} + export type FocusAndScrollRef = { /** - * If focus and scroll should be set in the layout-router's useEffect() + * The scroll ref from the most recent navigation. Set to whatever was + * accumulated during tree construction (or null if nothing was + * accumulated). On the next navigation, if new scroll targets are + * created, the previous scrollRef is invalidated by setting + * `current = false`. */ - apply: boolean + scrollRef: ScrollRef | null /** - * The hash fragment that should be scrolled to. + * When true, the scroll handler uses `focusAndScrollRef.scrollRef` + * for every segment regardless of per-node state. Used for hash-only + * navigations where every segment should be treated as a scroll + * target. When false, the handler checks `cacheNode.scrollRef` + * instead (per-node), so only segments that actually navigated scroll. */ - hashFragment: string | null + forceScroll: boolean /** - * The paths of the segments that should be focused. + * The hash fragment that should be scrolled to. */ - segmentPaths: FlightSegmentPath[] + hashFragment: string | null /** * If only the URLs hash fragment changed */ diff --git a/packages/next/src/client/components/segment-cache/navigation.ts b/packages/next/src/client/components/segment-cache/navigation.ts index 51fc5ab59d130..0b0ce955dc8f0 100644 --- a/packages/next/src/client/components/segment-cache/navigation.ts +++ b/packages/next/src/client/components/segment-cache/navigation.ts @@ -2,6 +2,7 @@ import type { CacheNodeSeedData, FlightRouterState, FlightSegmentPath, + ScrollRef, } from '../../../shared/lib/app-router-types' import type { CacheNode } from '../../../shared/lib/app-router-types' import type { HeadData } from '../../../shared/lib/app-router-types' @@ -34,6 +35,7 @@ import { PrefetchPriority, FetchStrategy } from './types' import { getLinkForCurrentNavigation } from '../links' import type { PageVaryPath } from './vary-path' import type { AppRouterState } from '../router-reducer/router-reducer-types' +import { ScrollBehavior } from '../router-reducer/router-reducer-types' import { computeChangedPath } from '../router-reducer/compute-changed-path' import { isJavaScriptURLString } from '../../lib/javascript-url' @@ -54,7 +56,7 @@ export function navigate( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): AppRouterState | Promise { // Instant Navigation Testing API: when the lock is active, ensure a @@ -76,7 +78,7 @@ export function navigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) } @@ -91,7 +93,7 @@ export function navigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) } @@ -105,7 +107,7 @@ function navigateImpl( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): AppRouterState | Promise { const now = Date.now() @@ -125,7 +127,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType, route ) @@ -161,7 +163,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType, optimisticRoute ) @@ -184,7 +186,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ).catch(() => { // If the navigation fails, return the current state @@ -204,7 +206,7 @@ export function navigateToKnownRoute( currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, nextUrl: string | null, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace', debugInfo: Array | null, // The route cache entry used for this navigation, if it came from route @@ -223,8 +225,8 @@ export function navigateToKnownRoute( // A version of navigate() that accepts the target route tree as an argument // rather than reading it from the prefetch cache. const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } // We special case navigations to the exact same URL as the current location. // It's a common UI pattern for apps to refresh when you click a link to the @@ -280,8 +282,8 @@ export function navigateToKnownRoute( navigationSeed.renderedSearch, canonicalUrl, navigateType, - shouldScroll, - accumulation.scrollableSegments, + scrollBehavior, + accumulation.scrollRef, debugInfo ) } @@ -299,7 +301,7 @@ function navigateUsingPrefetchedRouteTree( currentCacheNode: CacheNode | null, currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace', route: FulfilledRouteCacheEntry ): AppRouterState { @@ -325,7 +327,7 @@ function navigateUsingPrefetchedRouteTree( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, null, route @@ -354,7 +356,7 @@ async function navigateToUnknownRoute( currentCacheNode: CacheNode | null, currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): Promise { // Runs when a navigation happens but there's no cached prefetch we can use. @@ -509,7 +511,7 @@ async function navigateToUnknownRoute( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, debugInfo, // Unknown route navigations don't use route prediction - the route tree @@ -564,8 +566,8 @@ export function completeSoftNavigation( renderedSearch: string, canonicalUrl: string, navigateType: 'push' | 'replace', - shouldScroll: boolean, - scrollableSegments: Array | null, + scrollBehavior: ScrollBehavior, + scrollRef: ScrollRef | null, collectedDebugInfo: Array | null ) { // The "Next-Url" is a special representation of the URL that Next.js @@ -594,20 +596,72 @@ export function completeSoftNavigation( url.search === oldUrl.search && url.hash !== oldUrl.hash - // During a hash-only change, setting scrollableSegments to an empty - // array triggers a scroll for all new and updated segments. See - // `ScrollAndFocusHandler` for more details. + // Determine whether and how the page should scroll after this + // navigation. // - // TODO: Given the previous comment, I don't know why shouldScroll = - // false sets this to an empty array. Seems like an accident. I'm just - // preserving the logic that was already here. Clean this up when we - // move the per-segment scroll state to the CacheNode. - const segmentPathsToScrollTo = - onlyHashChange || !shouldScroll - ? [] - : scrollableSegments !== null - ? scrollableSegments - : oldState.focusAndScrollRef.segmentPaths + // By default, we scroll to the segments that were navigated to — i.e. + // segments in the new part of the route, as opposed to shared segments + // that were already part of the previous route. All newly navigated + // segments share a single ScrollRef. When they mount, the first one + // to mount initiates the scroll. They share a ref so that only one + // scroll happens per navigation. + // + // If a subsequent navigation produces new segments, those supersede + // any pending scroll from the previous navigation by invalidating its + // ScrollRef. If a navigation doesn't produce any new segments (e.g. + // a refresh where the route structure didn't change), any pending + // scrolls from previous navigations are unaffected. + // + // The branches below handle special cases layered on top of this + // default model. + let activeScrollRef: ScrollRef | null + let forceScroll: boolean + if (scrollBehavior === ScrollBehavior.NoScroll) { + // The user explicitly opted out of scrolling (e.g. scroll={false} + // on a Link or router.push). + // + // If this navigation created new scroll targets (scrollRef !== null), + // neutralize them. If it didn't, any prior scroll targets carried + // forward on the cache nodes via reuseSharedCacheNode remain active. + if (scrollRef !== null) { + scrollRef.current = false + } + activeScrollRef = oldState.focusAndScrollRef.scrollRef + forceScroll = false + } else if (onlyHashChange) { + // Hash-only navigations should scroll regardless of per-node state. + // Create a fresh ref so the first segment to scroll consumes it. + // + // Invalidate any scroll ref from a prior navigation that hasn't + // been consumed yet. + const oldScrollRef = oldState.focusAndScrollRef.scrollRef + if (oldScrollRef !== null) { + oldScrollRef.current = false + } + // Also invalidate any per-node refs that were accumulated during + // this navigation's tree construction — the hash-only ref + // supersedes them. + if (scrollRef !== null) { + scrollRef.current = false + } + activeScrollRef = { current: true } + forceScroll = true + } else { + // Default case. Use the accumulated scrollRef (may be null if no + // new segments were created). The handler checks per-node refs, so + // unchanged parallel route slots won't scroll. + activeScrollRef = scrollRef + + // If this navigation created new scroll targets, invalidate any + // pending scroll from a previous navigation. + if (scrollRef !== null) { + const oldScrollRef = oldState.focusAndScrollRef.scrollRef + if (oldScrollRef !== null) { + oldScrollRef.current = false + } + } + forceScroll = false + } const newState: AppRouterState = { canonicalUrl, @@ -618,13 +672,8 @@ export function completeSoftNavigation( preserveCustomHistoryState: false, }, focusAndScrollRef: { - // TODO: We should track all the per-segment scroll state on the CacheNode - // instead of using the paths. - apply: shouldScroll - ? segmentPathsToScrollTo !== null - ? true - : oldState.focusAndScrollRef.apply - : oldState.focusAndScrollRef.apply, + scrollRef: activeScrollRef, + forceScroll, onlyHashChange, hashFragment: // Remove leading # and decode hash to make non-latin hashes work. @@ -633,10 +682,9 @@ export function completeSoftNavigation( // view. #top is handled in layout-router. // // Refer to `ScrollAndFocusHandler` for details on how this is used. - shouldScroll && url.hash !== '' + scrollBehavior !== ScrollBehavior.NoScroll && url.hash !== '' ? decodeURIComponent(url.hash.slice(1)) : oldState.focusAndScrollRef.hashFragment, - segmentPaths: segmentPathsToScrollTo, }, cache, tree, @@ -887,7 +935,7 @@ async function ensurePrefetchThenNavigate( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): Promise { const link = getLinkForCurrentNavigation() @@ -926,7 +974,7 @@ async function ensurePrefetchThenNavigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) diff --git a/packages/next/src/shared/lib/app-router-types.ts b/packages/next/src/shared/lib/app-router-types.ts index 8757a43b36bef..1591d1574bd40 100644 --- a/packages/next/src/shared/lib/app-router-types.ts +++ b/packages/next/src/shared/lib/app-router-types.ts @@ -49,8 +49,25 @@ export type CacheNode = { head: HeadData slots: Record | null + + /** + * A shared mutable ref that tracks whether this segment should be scrolled + * to. All new segments created during a single navigation share the same + * ref. When any segment's scroll handler fires, it sets `current` to + * `false` so no other segment scrolls for the same navigation. + * + * `null` means this segment is not a scroll target (e.g., a reused shared + * layout segment). + */ + scrollRef: ScrollRef | null } +/** + * A mutable ref shared across all new segments created during a single + * navigation. Used to ensure that only one segment scrolls per navigation. + */ +export type ScrollRef = { current: boolean } + export type DynamicParamTypes = | 'catchall' | 'catchall-intercepted-(..)(..)' diff --git a/test/development/browser-logs/browser-logs.test.ts b/test/development/browser-logs/browser-logs.test.ts index 42cdcfa2e3fbb..b0ef324d9a100 100644 --- a/test/development/browser-logs/browser-logs.test.ts +++ b/test/development/browser-logs/browser-logs.test.ts @@ -336,8 +336,8 @@ describe(`Terminal Logging (${bundlerName})`, () => { ... - - <${innerScrollAndMaybeFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}> + + <${innerScrollAndMaybeFocusHandlerName} focusAndScrollRef={{scrollRef:null, ...}} cacheNode={{rsc:{...}, ...}}> diff --git a/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts new file mode 100644 index 0000000000000..77a5ae6a36b87 --- /dev/null +++ b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts @@ -0,0 +1,7 @@ +'use server' + +import { refresh } from 'next/cache' + +export async function refreshAction() { + refresh() +} diff --git a/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx new file mode 100644 index 0000000000000..f32479c427907 --- /dev/null +++ b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx @@ -0,0 +1,21 @@ +import { refreshAction } from './actions' +import { connection } from 'next/server' + +export default async function Page() { + await connection() + + const timestamp = Date.now() + + return ( + <> +
+
+ +
+
{timestamp}
+
+ + ) +} diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts index 3f34d50282773..e2d90ee498317 100644 --- a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts +++ b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts @@ -173,6 +173,33 @@ describe('router autoscrolling on navigation', () => { ) }) + describe('server action refresh', () => { + it('should not scroll when refresh() is called from a server action', async () => { + const browser = await next.browser('/server-action-refresh') + + const initialTimestamp = await browser + .elementByCss('#server-timestamp') + .text() + + // Scroll down past the first spacer div + await scrollTo(browser, { x: 0, y: 1000 }) + + // Click the refresh button which calls refresh() via a server action + await browser.elementByCss('#refresh-button').click() + + // Wait for the action to complete by checking the server timestamp + await retry(async () => { + const newTimestamp = await browser + .elementByCss('#server-timestamp') + .text() + expect(newTimestamp).not.toBe(initialTimestamp) + }) + + // Scroll position should be preserved + await waitForScrollToComplete(browser, { x: 0, y: 1000 }) + }) + }) + describe('bugs', () => { it('Should scroll to the top of the layout when the first child is display none', async () => { const browser = await next.browser('/') From 4cd8a9890f93d1b79856f9957dd3d3b20fdf1e00 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sat, 14 Mar 2026 14:02:25 +0100 Subject: [PATCH 4/4] Turbopack: use keyed cell access for AsyncModulesInfo (#91305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? Switch `AsyncModulesInfo` to use `cell = "keyed"` and per-key access (`is_async()`) instead of reading the full `FxHashSet` via `.await?`. ### Why? `AsyncModulesInfo` is a large set that gets read from many call sites. With the default cell, any change to any module's async status invalidates all readers. The `cell = "keyed"` annotation enables turbo-tasks to track reads at the key level, so a change to one module's async status only invalidates tasks that queried that specific module. This reduces unnecessary recomputation during incremental rebuilds where only a small subset of modules change their async status. ### How? **Core change** (`async_module_info.rs`): - Added `cell = "keyed"` to the `#[turbo_tasks::value]` annotation on `AsyncModulesInfo` - Added `is_async()` method that uses `contains_key()` for keyed reads instead of full-set reads **Call site migration** (5 files): - All call sites changed from `.async_module_info().await?` (reads full set) → `.async_module_info()` (returns `Vc`, defers reads) - `attach_async_info_to_chunkable_module` and `from_chunkable_module_or_batch` now take `Vc` instead of `&ReadRef` - Each call site uses `async_module_info.is_async(module).await?` for per-key reads **`referenced_async_modules`** (`mod.rs`): - Collects neighbor candidates first, then filters via `is_async()` with `try_flat_join` for concurrent keyed reads - Simplified: uses in-place `.reverse()` instead of `.rev().collect()` double allocation **`compute_merged_modules`** (`merged_modules.rs`): - Pre-fetches async status for all mergeable modules into a local `FxHashSet` before the synchronous fixed-point traversal (which cannot do async reads) - Added tracing span `"pre-fetch async module status"` for observability **Cleanup:** - Consistent variable naming (`async_module_info`) across all call sites - Doc comment on `attach_async_info_to_chunkable_module` explaining the keyed access pattern --------- Co-authored-by: Tobias Koppers Co-authored-by: Claude --- .../turbopack-core/src/chunk/chunk_group.rs | 4 ++-- .../src/chunk/chunk_item_batch.rs | 19 ++++++++++++------- .../src/module_graph/merged_modules.rs | 18 +++++++++++++++--- .../turbopack-core/src/module_graph/mod.rs | 10 +++++----- .../src/module_graph/style_groups.rs | 4 ++-- 5 files changed, 36 insertions(+), 19 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs b/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs index 103d8f1dfcfee..3be6cb80137c8 100644 --- a/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs +++ b/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs @@ -81,7 +81,7 @@ pub async fn make_chunk_group( ) .await?; - let async_modules_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); // Attach async info to chunkable modules let mut chunk_items = chunkable_items @@ -90,7 +90,7 @@ pub async fn make_chunk_group( .map(|m| { ChunkItemOrBatchWithAsyncModuleInfo::from_chunkable_module_or_batch( m, - &async_modules_info, + async_module_info, module_graph, *chunking_context, ) diff --git a/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs b/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs index e3830d08801fd..8f3d92ba4b21e 100644 --- a/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs +++ b/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs @@ -20,14 +20,19 @@ use crate::{ }, }; +/// Converts a [`ChunkableModule`] into a [`ChunkItemWithAsyncModuleInfo`] by resolving its chunk +/// item and, if the module is async, looking up its referenced async modules from the graph. +/// +/// Uses keyed access on `async_module_info` so only the queried module's entry is read, +/// enabling per-key invalidation via `cell = "keyed"` on [`AsyncModulesInfo`]. pub async fn attach_async_info_to_chunkable_module( module: ResolvedVc>, - async_module_info: &ReadRef, + async_module_info: Vc, module_graph: Vc, chunking_context: Vc>, ) -> Result { let general_module = ResolvedVc::upcast(module); - let async_info = if async_module_info.contains(&general_module) { + let async_info = if async_module_info.is_async(general_module).await? { Some( module_graph .referenced_async_modules(*general_module) @@ -67,7 +72,7 @@ pub struct ChunkItemOrBatchWithAsyncModuleInfos(Vec, + async_module_info: Vc, module_graph: Vc, chunking_context: Vc>, ) -> Result> { @@ -131,7 +136,7 @@ impl ChunkItemBatchWithAsyncModuleInfo { module_graph: Vc, chunking_context: Vc>, ) -> Result> { - let async_module_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let batch = batch.await?; let chunk_items = batch .modules @@ -139,7 +144,7 @@ impl ChunkItemBatchWithAsyncModuleInfo { .map(|module| { attach_async_info_to_chunkable_module( *module, - &async_module_info, + async_module_info, module_graph, chunking_context, ) @@ -242,7 +247,7 @@ impl ChunkItemBatchGroup { module_graph: Vc, chunking_context: Vc>, ) -> Result> { - let async_module_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let batch_group = batch_group.await?; let items = batch_group .items @@ -250,7 +255,7 @@ impl ChunkItemBatchGroup { .map(|&batch| { ChunkItemOrBatchWithAsyncModuleInfo::from_chunkable_module_or_batch( batch, - &async_module_info, + async_module_info, module_graph, chunking_context, ) diff --git a/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs b/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs index e3c5ac44c9582..70e736d38e5d2 100644 --- a/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs +++ b/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs @@ -93,7 +93,7 @@ pub async fn compute_merged_modules(module_graph: Vc) -> Result) -> Result>(); + // Pre-fetch async status for all mergeable modules using keyed access to avoid + // reading the full AsyncModulesInfo set during the synchronous traversal below. + let inner_span = tracing::info_span!("pre-fetch async module status"); + let async_modules: FxHashSet<_> = mergeable + .iter() + .map(async |&module| Ok(async_module_info.is_async(module).await?.then_some(module))) + .try_flat_join() + .instrument(inner_span) + .await? + .into_iter() + .collect(); + let inner_span = tracing::info_span!("fixed point traversal").entered(); let mut next_index = 0u32; @@ -191,8 +203,8 @@ pub async fn compute_merged_modules(module_graph: Vc) -> Result>, ) -> Result> { let graph_ref = self.await?; - let async_modules_info = self.async_module_info().await?; + let async_module_info = self.async_module_info(); let entry = graph_ref.get_entry(module)?; let referenced_modules = graph_ref @@ -828,9 +828,9 @@ impl ModuleGraph { .collect::>>()? .into_iter() .rev() - .filter(|m| async_modules_info.contains(m)) - .map(|m| *m) - .collect(); + .map(async |m| Ok(async_module_info.is_async(m).await?.then_some(*m))) + .try_flat_join() + .await?; Ok(AsyncModuleInfo::new(referenced_modules)) } diff --git a/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs b/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs index bdbc451f901e3..c938493b83a39 100644 --- a/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs +++ b/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs @@ -79,7 +79,7 @@ pub async fn compute_style_groups( let batches_graph = module_graph .module_batches(chunking_context.batching_config()) .await?; - let async_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let mut module_info_map: FxIndexMap>, Option> = FxIndexMap::default(); @@ -227,7 +227,7 @@ pub async fn compute_style_groups( .map(async |&module| { let chunk_item = attach_async_info_to_chunkable_module( module, - &async_info, + async_module_info, module_graph, chunking_context, )