[turbopack] Fix URL fragment and query handling in Turbopack (#79993)

Fix URL fragment and query handling in Turbopack

## What?
This PR improves how URL fragments and query strings are handled in Turbopack by:

1. Standardizing the representation of fragments and queries in `AssetIdent`
2. Ensuring fragments and queries are either empty or include their prefix characters (`#` and `?`)
3. Fixing the URL parsing logic to correctly handle fragments that contain `?` character
4. Removing some ad-hoc workarounds for this issue in the `resolver` and `Assetident::to_string`

## Why?
The previous implementation had inconsistencies in how fragments and queries were represented which lead to a variety of confusing issues e.g. does `AssetIdent::to_string` need to add a `?` ?, well sometimes depending on how it was constructed.

Author: lukesandberg

crates/next-api/src/server_actions.rs

@@ -213,7 +213,7 @@ pub async fn to_rsc_context(
     // module.
     let source = FileSource::new_with_query(
         client_module.ident().path().root().join(entry_path.into()),
-        Vc::cell(entry_query.into()),
+        entry_query.into(),
     );
     let module = asset_context
         .process(

crates/next-core/src/next_client_reference/ecmascript_client_reference/ecmascript_client_reference_transition.rs

@@ -71,7 +71,11 @@ impl Transition for NextEcmascriptClientReferenceTransition {
                     .replace("next/dist/esm/", "next/dist/")
                     .into(),
             );
-            Vc::upcast(FileSource::new_with_query(path, *ident_ref.query))
+            Vc::upcast(FileSource::new_with_query_and_fragment(
+                path,
+                (*ident_ref.query.await?).clone(),
+                (*ident_ref.fragment.await?).clone(),
+            ))
         } else {
             source
         };

turbopack/crates/turbopack-core/src/file_source.rs

@@ -13,38 +13,48 @@ use crate::{
 /// references to other [Source]s.
 #[turbo_tasks::value]
 pub struct FileSource {
-    pub path: ResolvedVc<FileSystemPath>,
-    pub query: ResolvedVc<RcStr>,
+    path: ResolvedVc<FileSystemPath>,
+    query: RcStr,
+    fragment: RcStr,
+}
+
+impl FileSource {
+    pub fn new(path: Vc<FileSystemPath>) -> Vc<Self> {
+        FileSource::new_with_query_and_fragment(path, RcStr::default(), RcStr::default())
+    }
+    pub fn new_with_query(path: Vc<FileSystemPath>, query: RcStr) -> Vc<Self> {
+        FileSource::new_with_query_and_fragment(path, query, RcStr::default())
+    }
 }
 
 #[turbo_tasks::value_impl]
 impl FileSource {
     #[turbo_tasks::function]
-    pub fn new(path: ResolvedVc<FileSystemPath>) -> Vc<Self> {
+    pub fn new_with_query_and_fragment(
+        path: ResolvedVc<FileSystemPath>,
+        query: RcStr,
+        fragment: RcStr,
+    ) -> Vc<Self> {
         Self::cell(FileSource {
             path,
-            query: ResolvedVc::cell(RcStr::default()),
+            query,
+            fragment,
         })
     }
-
-    #[turbo_tasks::function]
-    pub async fn new_with_query(
-        path: ResolvedVc<FileSystemPath>,
-        query: ResolvedVc<RcStr>,
-    ) -> Result<Vc<Self>> {
-        if query.await?.is_empty() {
-            Ok(Self::new(*path))
-        } else {
-            Ok(Self::cell(FileSource { path, query }))
-        }
-    }
 }
 
 #[turbo_tasks::value_impl]
 impl Source for FileSource {
     #[turbo_tasks::function]
     fn ident(&self) -> Vc<AssetIdent> {
-        AssetIdent::from_path(*self.path).with_query(*self.query)
+        let mut ident = AssetIdent::from_path(*self.path);
+        if !self.query.is_empty() {
+            ident = ident.with_query(Vc::cell(self.query.clone()));
+        }
+        if !self.fragment.is_empty() {
+            ident = ident.with_fragment(Vc::cell(self.fragment.clone()));
+        }
+        ident
     }
 }
 

turbopack/crates/turbopack-core/src/ident.rs

@@ -15,10 +15,12 @@ use crate::resolve::ModulePart;
 pub struct AssetIdent {
     /// The primary path of the asset
     pub path: ResolvedVc<FileSystemPath>,
-    /// The query string of the asset (e.g. `?foo=bar`)
+    /// The query string of the asset this is either the empty string or a query string that starts
+    /// with a `?` (e.g. `?foo=bar`)
     pub query: ResolvedVc<RcStr>,
-    /// The fragment of the asset (e.g. `#foo`)
-    pub fragment: Option<ResolvedVc<RcStr>>,
+    /// The fragment of the asset, this is either the empty string or a fragment string that starts
+    /// with a `#` (e.g. `#foo`)
+    pub fragment: ResolvedVc<RcStr>,
     /// The assets that are nested in this asset
     pub assets: Vec<(ResolvedVc<RcStr>, ResolvedVc<AssetIdent>)>,
     /// The modifiers of this asset (e.g. `client chunks`)
@@ -57,14 +59,10 @@ impl ValueToString for AssetIdent {
     async fn to_string(&self) -> Result<Vc<RcStr>> {
         let mut s = self.path.to_string().owned().await?.into_owned();
 
-        let query = self.query.await?;
-        if !query.is_empty() {
-            write!(s, "?{}", &*query)?;
-        }
-
-        if let Some(fragment) = &self.fragment {
-            write!(s, "#{}", fragment.await?)?;
-        }
+        // The query string is either empty or non-empty starting with `?` so we can just concat
+        s.push_str(&self.query.await?);
+        // ditto for fragment
+        s.push_str(&self.fragment.await?);
 
         if !self.assets.is_empty() {
             s.push_str(" {");
@@ -121,8 +119,19 @@ impl ValueToString for AssetIdent {
 #[turbo_tasks::value_impl]
 impl AssetIdent {
     #[turbo_tasks::function]
-    pub fn new(ident: Value<AssetIdent>) -> Vc<Self> {
-        ident.into_value().cell()
+    pub async fn new(ident: Value<AssetIdent>) -> Result<Vc<Self>> {
+        let query = &*ident.query.await?;
+        // TODO(lukesandberg); downgrade to debug_assert
+        assert!(
+            query.is_empty() || query.starts_with("?"),
+            "query should be empty or start with a `?`"
+        );
+        let fragment = &*ident.fragment.await?;
+        assert!(
+            fragment.is_empty() || fragment.starts_with("#"),
+            "query should be empty or start with a `?`"
+        );
+        Ok(ident.into_value().cell())
     }
 
     /// Creates an [AssetIdent] from a [Vc<FileSystemPath>]
@@ -131,7 +140,7 @@ impl AssetIdent {
         Self::new(Value::new(AssetIdent {
             path,
             query: ResolvedVc::cell(RcStr::default()),
-            fragment: None,
+            fragment: ResolvedVc::cell(RcStr::default()),
             assets: Vec::new(),
             modifiers: Vec::new(),
             parts: Vec::new(),
@@ -147,6 +156,13 @@ impl AssetIdent {
         Self::new(Value::new(this))
     }
 
+    #[turbo_tasks::function]
+    pub fn with_fragment(&self, fragment: ResolvedVc<RcStr>) -> Vc<Self> {
+        let mut this = self.clone();
+        this.fragment = fragment;
+        Self::new(Value::new(this))
+    }
+
     #[turbo_tasks::function]
     pub fn with_modifier(&self, modifier: ResolvedVc<RcStr>) -> Vc<Self> {
         let mut this = self.clone();
@@ -252,9 +268,10 @@ impl AssetIdent {
             query.deterministic_hash(&mut hasher);
             has_hash = true;
         }
-        if let Some(fragment) = fragment {
+        let fragment = fragment.await?;
+        if !fragment.is_empty() {
             1_u8.deterministic_hash(&mut hasher);
-            fragment.await?.deterministic_hash(&mut hasher);
+            fragment.deterministic_hash(&mut hasher);
             has_hash = true;
         }
         for (key, ident) in assets.iter() {

turbopack/crates/turbopack-core/src/resolve/mod.rs

@@ -2203,18 +2203,17 @@ async fn resolve_relative_request(
 
     let fragment_val = fragment.await?;
     if !fragment_val.is_empty() {
-        new_path.push(Pattern::Alternatives(
-            once(Pattern::Constant("".into()))
-                .chain(once(Pattern::Constant(format!("#{fragment_val}").into())))
-                .collect(),
-        ));
+        new_path.push(Pattern::Alternatives(vec![
+            Pattern::Constant(RcStr::default()),
+            Pattern::Constant((*fragment_val).clone()),
+        ]));
     }
 
     if !options_value.fully_specified {
         // Add the extensions as alternatives to the path
         // read_matches keeps the order of alternatives intact
         new_path.push(Pattern::Alternatives(
-            once(Pattern::Constant("".into()))
+            once(Pattern::Constant(RcStr::default()))
                 .chain(
                     options_value
                         .extensions
@@ -2328,10 +2327,7 @@ async fn resolve_relative_request(
             }
             if !fragment_val.is_empty() {
                 // If the fragment is not empty, we need to strip it from the matched pattern
-                if let Some(matched_pattern) = matched_pattern
-                    .strip_suffix(&**fragment_val)
-                    .and_then(|s| s.strip_suffix('#'))
-                {
+                if let Some(matched_pattern) = matched_pattern.strip_suffix(&**fragment_val) {
                     results.push(
                         resolved(
                             RequestKey::new(matched_pattern.into()),
@@ -2864,9 +2860,13 @@ async fn resolved(
     Ok(*ResolveResult::source_with_affecting_sources(
         request_key,
         ResolvedVc::upcast(
-            FileSource::new_with_query(**path, query)
-                .to_resolved()
-                .await?,
+            FileSource::new_with_query_and_fragment(
+                **path,
+                (*query.await?).clone(),
+                (*fragment.await?).clone(),
+            )
+            .to_resolved()
+            .await?,
         ),
         symlinks
             .iter()