Repository: vercel/next.js
Pull request: Turbopack: fix duplicate externals modules

Don't attatch the resolve request's affecting sources to the external module itself. This list wasn't part of the module ident and would lead to duplicate modules.

Closes PACK-5038

Changed files:
filename: turbopack/crates/turbopack-core/src/resolve/mod.rs
patch: @@ -76,7 +76,6 @@ pub enum ModuleResolveResultItem {
         /// uri, path, reference, etc.
         name: RcStr,
         ty: ExternalType,
-        traced: Option<ResolvedVc<ModuleResolveResult>>,
     },
     /// A module could not be created (according to the rules, e.g. no module type as assigned)
     Unknown(ResolvedVc<Box<dyn Source>>),
@@ -727,11 +726,7 @@ impl ResolveResult {
                                         // Should use map_primary_items instead
                                         bail!("map_module doesn't handle traced externals");
                                     }
-                                    ModuleResolveResultItem::External {
-                                        name,
-                                        ty,
-                                        traced: None,
-                                    }
+                                    ModuleResolveResultItem::External { name, ty }
                                 }
                                 ResolveResultItem::Ignore => ModuleResolveResultItem::Ignore,
                                 ResolveResultItem::Empty => ModuleResolveResultItem::Empty,

filename: turbopack/crates/turbopack-ecmascript/src/references/external_module.rs
patch: @@ -3,15 +3,21 @@ use std::{fmt::Display, io::Write};
 use anyhow::Result;
 use serde::{Deserialize, Serialize};
 use turbo_rcstr::{RcStr, rcstr};
-use turbo_tasks::{NonLocalValue, ResolvedVc, TaskInput, Vc, trace::TraceRawVcs};
-use turbo_tasks_fs::{FileContent, FileSystem, VirtualFileSystem, glob::Glob, rope::RopeBuilder};
+use turbo_tasks::{NonLocalValue, ResolvedVc, TaskInput, TryJoinIterExt, Vc, trace::TraceRawVcs};
+use turbo_tasks_fs::{
+    FileContent, FileSystem, FileSystemPath, VirtualFileSystem, glob::Glob, rope::RopeBuilder,
+};
 use turbopack_core::{
     asset::{Asset, AssetContent},
     chunk::{AsyncModuleInfo, ChunkItem, ChunkType, ChunkableModule, ChunkingContext},
+    context::AssetContext,
     ident::{AssetIdent, Layer},
     module::Module,
     module_graph::ModuleGraph,
-    reference::{ModuleReference, ModuleReferences},
+    raw_module::RawModule,
+    reference::{ModuleReference, ModuleReferences, TracedModuleReference},
+    reference_type::ReferenceType,
+    resolve::parse::Request,
 };
 
 use crate::{
@@ -49,6 +55,19 @@ pub enum CachedExternalType {
     Script,
 }
 
+#[derive(
+    Clone, Debug, Eq, PartialEq, Serialize, Deserialize, TraceRawVcs, TaskInput, Hash, NonLocalValue,
+)]
+/// Whether to add a traced reference to the external module using the given context and resolve
+/// origin.
+pub enum CachedExternalTracingMode {
+    Untraced,
+    Traced {
+        externals_context: ResolvedVc<Box<dyn AssetContext>>,
+        root_origin: FileSystemPath,
+    },
+}
+
 impl Display for CachedExternalType {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -63,9 +82,9 @@ impl Display for CachedExternalType {
 
 #[turbo_tasks::value]
 pub struct CachedExternalModule {
-    pub request: RcStr,
-    pub external_type: CachedExternalType,
-    pub additional_references: Vec<ResolvedVc<Box<dyn ModuleReference>>>,
+    request: RcStr,
+    external_type: CachedExternalType,
+    tracing_mode: CachedExternalTracingMode,
 }
 
 #[turbo_tasks::value_impl]
@@ -74,12 +93,12 @@ impl CachedExternalModule {
     pub fn new(
         request: RcStr,
         external_type: CachedExternalType,
-        additional_references: Vec<ResolvedVc<Box<dyn ModuleReference>>>,
+        tracing_mode: CachedExternalTracingMode,
     ) -> Vc<Self> {
         Self::cell(CachedExternalModule {
             request,
             external_type,
-            additional_references,
+            tracing_mode,
         })
     }
 
@@ -202,8 +221,46 @@ impl Module for CachedExternalModule {
     }
 
     #[turbo_tasks::function]
-    fn references(&self) -> Result<Vc<ModuleReferences>> {
-        Ok(Vc::cell(self.additional_references.clone()))
+    async fn references(&self) -> Result<Vc<ModuleReferences>> {
+        Ok(match &self.tracing_mode {
+            CachedExternalTracingMode::Untraced => ModuleReferences::empty(),
+            CachedExternalTracingMode::Traced {
+                externals_context,
+                root_origin,
+            } => {
+                let reference_type = match self.external_type {
+                    CachedExternalType::EcmaScriptViaImport => {
+                        ReferenceType::EcmaScriptModules(Default::default())
+                    }
+                    CachedExternalType::CommonJs | CachedExternalType::EcmaScriptViaRequire => {
+                        ReferenceType::CommonJs(Default::default())
+                    }
+                    _ => ReferenceType::Undefined,
+                };
+
+                let external_result = externals_context
+                    .resolve_asset(
+                        root_origin.clone(),
+                        Request::parse_string(self.request.clone()),
+                        externals_context
+                            .resolve_options(root_origin.clone(), reference_type.clone()),
+                        reference_type,
+                    )
+                    .await?;
+                let references = external_result
+                    .affecting_sources
+                    .iter()
+                    .map(|s| Vc::upcast::<Box<dyn Module>>(RawModule::new(**s)))
+                    .chain(external_result.primary_modules_raw_iter().map(|rvc| *rvc))
+                    .map(|s| {
+                        Vc::upcast::<Box<dyn ModuleReference>>(TracedModuleReference::new(s))
+                            .to_resolved()
+                    })
+                    .try_join()
+                    .await?;
+                Vc::cell(references)
+            }
+        })
     }
 
     #[turbo_tasks::function]

filename: turbopack/crates/turbopack/src/lib.rs
patch: @@ -26,7 +26,7 @@ use graph::{AggregatedGraph, AggregatedGraphNodeContent, aggregate};
 use module_options::{ModuleOptions, ModuleOptionsContext, ModuleRuleEffect, ModuleType};
 use tracing::{Instrument, field::Empty};
 use turbo_rcstr::{RcStr, rcstr};
-use turbo_tasks::{FxIndexSet, ResolvedVc, ValueToString, Vc};
+use turbo_tasks::{ResolvedVc, ValueToString, Vc};
 use turbo_tasks_fs::{FileSystemPath, glob::Glob};
 pub use turbopack_core::condition;
 use turbopack_core::{
@@ -40,7 +40,6 @@ use turbopack_core::{
     module::Module,
     output::OutputAsset,
     raw_module::RawModule,
-    reference::{ModuleReference, TracedModuleReference},
     reference_type::{
         CssReferenceSubType, EcmaScriptModulesReferenceSubType, ImportContext, ImportWithType,
         InnerAssets, ReferenceType,
@@ -55,7 +54,9 @@ use turbopack_core::{
 pub use turbopack_css as css;
 pub use turbopack_ecmascript as ecmascript;
 use turbopack_ecmascript::{
-    references::external_module::{CachedExternalModule, CachedExternalType},
+    references::external_module::{
+        CachedExternalModule, CachedExternalTracingMode, CachedExternalType,
+    },
     tree_shake::asset::EcmascriptModulePartAsset,
 };
 use turbopack_json::JsonModuleAsset;
@@ -769,8 +770,6 @@ impl AssetContext for ModuleAssetContext {
 
         let result = result.await?;
 
-        let affecting_sources = &result.affecting_sources;
-
         let result = result
             .map_primary_items(|item| {
                 let reference_type = reference_type.clone();
@@ -789,89 +788,35 @@ impl AssetContext for ModuleAssetContext {
                         }
                         ResolveResultItem::External { name, ty, traced } => {
                             let replacement = if replace_externals {
-                                let additional_refs: Vec<Vc<Box<dyn ModuleReference>>> = if let (
-                                    ExternalTraced::Traced,
-                                    Some(tracing_root),
-                                ) = (
-                                    traced,
-                                    self.module_options_context()
+                                let tracing_mode = if traced == ExternalTraced::Traced
+                                    && let Some(tracing_root) = &self
+                                        .module_options_context()
                                         .await?
                                         .enable_externals_tracing
-                                        .clone(),
-                                ) {
-                                    let externals_context = externals_tracing_module_context(ty);
-                                    let root_origin = tracing_root.join("_")?;
-
-                                    // Normalize reference type, there is no such thing as a
-                                    // `ReferenceType::EcmaScriptModules(ImportPart(Evaluation))`
-                                    // for externals (and otherwise, this causes duplicate
-                                    // CachedExternalModules for both `ImportPart(Evaluation)` and
-                                    // `ImportPart(Export("CacheProvider"))`)
-                                    let reference_type = match reference_type {
-                                        ReferenceType::EcmaScriptModules(_) => {
-                                            ReferenceType::EcmaScriptModules(Default::default())
-                                        }
-                                        ReferenceType::CommonJs(_) => {
-                                            ReferenceType::CommonJs(Default::default())
-                                        }
-                                        ReferenceType::Css(_) => {
-                                            ReferenceType::Css(Default::default())
-                                        }
-                                        ReferenceType::Url(_) => {
-                                            ReferenceType::Url(Default::default())
-                                        }
-                                        _ => ReferenceType::Undefined,
-                                    };
-
-                                    let external_result = externals_context
-                                        .resolve_asset(
-                                            root_origin.clone(),
-                                            Request::parse_string(name.clone()),
-                                            externals_context.resolve_options(
-                                                root_origin,
-                                                reference_type.clone(),
-                                            ),
-                                            reference_type,
-                                        )
-                                        .await?;
-
-                                    let modules = affecting_sources
-                                        .iter()
-                                        .chain(external_result.affecting_sources.iter())
-                                        .map(|s| Vc::upcast::<Box<dyn Module>>(RawModule::new(**s)))
-                                        .chain(
-                                            external_result
-                                                .primary_modules_raw_iter()
-                                                .map(|rvc| *rvc),
-                                        )
-                                        .collect::<FxIndexSet<_>>();
-
-                                    modules
-                                        .into_iter()
-                                        .map(|s| {
-                                            Vc::upcast::<Box<dyn ModuleReference>>(
-                                                TracedModuleReference::new(s),
-                                            )
-                                        })
-                                        .collect()
+                                {
+                                    // result.affecting_sources can be ignored for tracing, as this
+                                    // request will later be resolved relative to tracing_root
+                                    // anyway.
+
+                                    CachedExternalTracingMode::Traced {
+                                        externals_context: ResolvedVc::upcast(
+                                            externals_tracing_module_context(ty)
+                                                .to_resolved()
+                                                .await?,
+                                        ),
+                                        root_origin: tracing_root.join("_")?,
+                                    }
                                 } else {
-                                    vec![]
+                                    CachedExternalTracingMode::Untraced
                                 };
 
-                                replace_external(&name, ty, additional_refs, import_externals)
-                                    .await?
+                                replace_external(&name, ty, import_externals, tracing_mode).await?
                             } else {
                                 None
                             };
 
-                            replacement.unwrap_or_else(|| {
-                                ModuleResolveResultItem::External {
-                                    name,
-                                    ty,
-                                    // TODO(micshnic) remove that field entirely ?
-                                    traced: None,
-                                }
-                            })
+                            replacement
+                                .unwrap_or_else(|| ModuleResolveResultItem::External { name, ty })
                         }
                         ResolveResultItem::Ignore => ModuleResolveResultItem::Ignore,
                         ResolveResultItem::Empty => ModuleResolveResultItem::Empty,
@@ -1000,8 +945,8 @@ pub async fn emit_asset_into_dir(
 pub async fn replace_external(
     name: &RcStr,
     ty: ExternalType,
-    additional_refs: Vec<Vc<Box<dyn ModuleReference>>>,
     import_externals: bool,
+    tracing_mode: CachedExternalTracingMode,
 ) -> Result<Option<ModuleResolveResultItem>> {
     let external_type = match ty {
         ExternalType::CommonJs => CachedExternalType::CommonJs,
@@ -1020,7 +965,7 @@ pub async fn replace_external(
         }
     };
 
-    let module = CachedExternalModule::new(name.clone(), external_type, additional_refs)
+    let module = CachedExternalModule::new(name.clone(), external_type, tracing_mode)
         .to_resolved()
         .await?;