Repository: vercel/next.js
Pull request: cleanup(turbopack): Added documentation comments and small optimizations to CSS import validations

## Refactor CSS Import Validation Logic in Next.js

### What?
Refactored the CSS import validation logic to improve readability and maintainability by reorganizing the code flow and adding comprehensive comments.

### Why?
The previous implementation had a confusing flow that made it difficult to understand the validation rules for global CSS imports. This refactor makes the logic clearer by:
1. Checking if the imported module is a global CSS module first
2. Validating imports from node_modules
3. Handling root nodes with no parents
4. Allowing imports from CSS modules
5. Finally checking if the parent is the app module

### How?
- Reordered the validation checks in `validate_pages_css_imports` to follow a more optimized flow
- Added detailed comments explaining each validation rule and its purpose
- Added documentation for the `validate_pages_css_imports` function
- Improved comments in the `pages.rs` file to explain why we recreate the app_module


Closes PACK-4915

Changed files:
filename: crates/next-api/src/module_graph.rs
patch: @@ -512,6 +512,15 @@ async fn validate_pages_css_imports(
     graph.traverse_edges_from_entries(entries, |parent_info, node| {
         let module = node.module;
 
+        // If the module being imported isn't a global css module, there is nothing to validate.
+        let module_is_global_css =
+            ResolvedVc::try_downcast_type::<CssModuleAsset>(module).is_some();
+
+        if !module_is_global_css {
+            return GraphTraversalAction::Continue;
+        }
+
+        // We allow imports of global CSS files which are inside of `node_modules`.
         let module_name_contains_node_modules = module_name_map
             .get(&module)
             .is_some_and(|s| s.contains("node_modules"));
@@ -520,8 +529,9 @@ async fn validate_pages_css_imports(
             return GraphTraversalAction::Continue;
         }
 
+        // If we're at a root node, there is nothing importing this module and we can skip
+        // any further validations.
         let Some((parent_node, _)) = parent_info else {
-            // root node, no parent
             return GraphTraversalAction::Continue;
         };
 
@@ -530,17 +540,14 @@ async fn validate_pages_css_imports(
             .is_some()
             || ResolvedVc::try_downcast_type::<CssModuleAsset>(parent_module).is_some();
 
+        // We also always allow .module css/scss/sass files to import global css files as well.
         if parent_is_css_module {
             return GraphTraversalAction::Continue;
         }
 
-        let module_is_global_css =
-            ResolvedVc::try_downcast_type::<CssModuleAsset>(module).is_some();
-
-        if !module_is_global_css {
-            return GraphTraversalAction::Continue;
-        }
-
+        // If all of the above invariants have been checked, we look to see if the parent module is
+        // the same as the app module. If it isn't we know it isn't a valid place to import global
+        // css.
         if parent_module != app_module {
             CssGlobalImportIssue::new(parent_module, module)
                 .resolved_cell()
@@ -740,43 +747,54 @@ impl ReducedGraphs {
     }
 
     #[turbo_tasks::function]
+    /// Validates that the global CSS/SCSS/SASS imports are only valid imports with the following
+    /// rules:
+    /// * The import is made from a `node_modules` package
+    /// * The import is made from a `.module.css` file
+    /// * The import is made from the `pages/_app.js`, or equivalent file.
     pub async fn validate_pages_css_imports(
         &self,
         entry: Vc<Box<dyn Module>>,
         app_module: Vc<Box<dyn Module>>,
     ) -> Result<()> {
         let span = tracing::info_span!("validate pages css imports");
         async move {
-            let mut ident_vec = Vec::new();
-            for graph in self.bare_graphs.await?.graphs.iter() {
-                let inputs = graph
+            let graphs = &self.bare_graphs.await?.graphs;
+
+            // We need to collect the module names here to pass into the
+            // `validate_pages_css_imports` function. This is because the function is
+            // called for each graph, and we need to know the module names of the parent
+            // modules to determine if the import is valid. We can't do this in the
+            // called function because it's within a closure that can't resolve turbo tasks.
+            let graph_to_module_ident_tuples = async |graph: &ResolvedVc<SingleModuleGraph>| {
+                graph
                     .await?
                     .graph
                     .node_weights()
-                    .map(async |n| {
-                        Ok((
-                            n.module(),
-                            RcStr::from(n.module().ident().to_string().await?.as_str()),
-                        ))
-                    })
+                    .map(async |n| Ok((n.module(), n.module().ident().to_string().owned().await?)))
                     .try_join()
-                    .await?;
-                ident_vec.extend(inputs);
-            }
-            let idents = ModuleNameMap(ident_vec.into_iter().collect::<FxIndexMap<_, _>>()).cell();
+                    .await
+            };
 
-            let _ = self
-                .bare_graphs
+            let identifier_map = graphs
+                .iter()
+                .map(graph_to_module_ident_tuples)
+                .try_join()
                 .await?
-                .graphs
+                .into_iter()
+                .flatten()
+                .collect::<FxIndexMap<_, _>>();
+            let identifier_map = ModuleNameMap(identifier_map).cell();
+
+            let _ = graphs
                 .iter()
                 .map(|graph| {
                     validate_pages_css_imports(
                         **graph,
                         self.is_single_page,
                         entry,
                         app_module,
-                        idents,
+                        identifier_map,
                     )
                 })
                 .try_join()

filename: crates/next-api/src/pages.rs
patch: @@ -1005,6 +1005,11 @@ impl PageEndpoint {
                 // We only validate the global css imports when there is not a `app` folder at the
                 // root of the project.
                 if project.app_project().await?.is_none() {
+                    // We recreate the app_module here because the one provided from the
+                    // `internal_ssr_chunk_module` is not the same as the one
+                    // provided from the `client_module_graph`. There can be cases where
+                    // the `app_module` is None, and we are processing the `pages/_app.js` file
+                    // as a page rather than the app module.
                     let app_module = project
                         .pages_project()
                         .client_module_context()

filename: turbopack/crates/turbopack-css/src/asset.rs
patch: @@ -33,6 +33,7 @@ use crate::{
 
 #[turbo_tasks::value]
 #[derive(Clone)]
+/// A global CSS asset. Notably not a `.module.css` module, which is [`ModuleCssAsset`] instead.
 pub struct CssModuleAsset {
     source: ResolvedVc<Box<dyn Source>>,
     asset_context: ResolvedVc<Box<dyn AssetContext>>,

filename: turbopack/crates/turbopack-css/src/module_asset.rs
patch: @@ -37,6 +37,7 @@ use crate::{
 
 #[turbo_tasks::value]
 #[derive(Clone)]
+/// A CSS Module asset, as in `.module.css`. For a global CSS module, see [`CssModuleAsset`].
 pub struct ModuleCssAsset {
     pub source: ResolvedVc<Box<dyn Source>>,
     pub asset_context: ResolvedVc<Box<dyn AssetContext>>,