Turbopack: disable tree shaking for tracing (#85722)

mischnic · huozhi · commit df38b10c0d81 · 2025-11-12T20:46:54.000+01:00
We had tree shaking enabled for NFT. That is incorrect however, as even unused imports (if you don't use any of the imported bindings, and the file is marked as `sideEffects: false`) are still needed at runtime, since Node.js will execute everything anyway. - I made the `treeShakingMode: None` explicit now (though that was already the `Default::default` anyway) - The real problem was that we still dropped side-effect free `ModulePart::Evaluation` references even when tree shaking was disabled. Closes #85172 Fixes PACK-5756
diff --git a/test/production/standalone-mode/tracing-side-effects-false/app/layout.js b/test/production/standalone-mode/tracing-side-effects-false/app/layout.js
@@ -0,0 +1,8 @@
+export default function Root({ children }) {
+  return (
+    <html>
+      <head></head>
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/production/standalone-mode/tracing-side-effects-false/app/page.js b/test/production/standalone-mode/tracing-side-effects-false/app/page.js
@@ -0,0 +1,5 @@
+import getValue from 'foo'
+
+export default function Page() {
+  return <p>{getValue()}</p>
+}
diff --git a/test/production/standalone-mode/tracing-side-effects-false/foo/index.js b/test/production/standalone-mode/tracing-side-effects-false/foo/index.js
@@ -0,0 +1,6 @@
+import value from './value.js'
+import './side-effect.js'
+
+export default function getValue() {
+  return value
+}
diff --git a/test/production/standalone-mode/tracing-side-effects-false/foo/package.json b/test/production/standalone-mode/tracing-side-effects-false/foo/package.json
@@ -0,0 +1,6 @@
+{
+  "name": "foo",
+  "type": "module",
+  "version": "0.0.0",
+  "sideEffects": false
+}
diff --git a/test/production/standalone-mode/tracing-side-effects-false/foo/side-effect.js b/test/production/standalone-mode/tracing-side-effects-false/foo/side-effect.js
@@ -0,0 +1 @@
+// some noop, should still resolve and not crash though
diff --git a/test/production/standalone-mode/tracing-side-effects-false/foo/value.js b/test/production/standalone-mode/tracing-side-effects-false/foo/value.js
@@ -0,0 +1 @@
+export default 123
diff --git a/test/production/standalone-mode/tracing-side-effects-false/next.config.js b/test/production/standalone-mode/tracing-side-effects-false/next.config.js
@@ -0,0 +1,8 @@
+/**
+ * @type {import('next').NextConfig}
+ */
+const nextConfig = {
+  serverExternalPackages: ['foo'],
+}
+
+module.exports = nextConfig
diff --git a/test/production/standalone-mode/tracing-side-effects-false/package.json b/test/production/standalone-mode/tracing-side-effects-false/package.json
@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "foo": "file:./foo"
+  }
+}
diff --git a/test/production/standalone-mode/tracing-side-effects-false/tracing-side-effects-false.test.ts b/test/production/standalone-mode/tracing-side-effects-false/tracing-side-effects-false.test.ts
@@ -0,0 +1,36 @@
+import { nextTestSetup } from 'e2e-utils'
+
+describe('standalone mode - tracing-side-effects-false', () => {
+  const dependencies = require('./package.json').dependencies
+
+  const { next, skipped } = nextTestSetup({
+    files: __dirname,
+    dependencies,
+    skipDeployment: true,
+    skipStart: true,
+  })
+
+  if (skipped) {
+    return
+  }
+
+  it('should trace sideeffect imports even when sideEffects is false', async () => {
+    let { exitCode } = await next.build()
+    expect(exitCode).toBe(0)
+
+    let trace = await next.readJSON('.next/server/app/page.js.nft.json')
+
+    expect(trace.files).toContainEqual(
+      expect.stringMatching(/node_modules\/foo\/index\.js$/)
+    )
+    expect(trace.files).toContainEqual(
+      expect.stringMatching(/node_modules\/foo\/package\.json$/)
+    )
+    expect(trace.files).toContainEqual(
+      expect.stringMatching(/node_modules\/foo\/side-effect\.js$/)
+    )
+    expect(trace.files).toContainEqual(
+      expect.stringMatching(/node_modules\/foo\/value\.js$/)
+    )
+  })
+})
diff --git a/turbopack/crates/turbopack-ecmascript/benches/references.rs b/turbopack/crates/turbopack-ecmascript/benches/references.rs
@@ -86,7 +86,11 @@ async fn setup(
         ResolvedVc::upcast(module_asset_context),
         EcmascriptInputTransforms::empty().to_resolved().await?,
         EcmascriptOptions {
-            tree_shaking_mode: Some(TreeShakingMode::ReexportsOnly),
+            tree_shaking_mode: if analyze_mode == AnalyzeMode::Tracing {
+                None
+            } else {
+                Some(TreeShakingMode::ReexportsOnly)
+            },
             analyze_mode,
             ..Default::default()
         }
diff --git a/turbopack/crates/turbopack-ecmascript/src/lib.rs b/turbopack/crates/turbopack-ecmascript/src/lib.rs
@@ -168,8 +168,8 @@ pub enum SpecifiedModuleType {
 )]
 #[serde(rename_all = "kebab-case")]
 pub enum TreeShakingMode {
-    #[default]
     ModuleFragments,
+    #[default]
     ReexportsOnly,
 }
 
diff --git a/turbopack/crates/turbopack-nft/src/nft.rs b/turbopack/crates/turbopack-nft/src/nft.rs
@@ -79,8 +79,10 @@ async fn node_file_trace_operation(
     let module_asset_context = ModuleAssetContext::new(
         Default::default(),
         // This config should be kept in sync with
-        // turbopack/crates/turbopack/tests/node-file-trace.rs and
-        // turbopack/crates/turbopack/src/lib.rs
+        // turbopack/crates/turbopack-tracing/tests/node-file-trace.rs and
+        // turbopack/crates/turbopack-tracing/tests/unit.rs and
+        // turbopack/crates/turbopack/src/lib.rs and
+        // turbopack/crates/turbopack-nft/src/nft.rs
         CompileTimeInfo::new(environment),
         ModuleOptionsContext {
             ecmascript: EcmascriptOptionsContext {
@@ -97,6 +99,9 @@ async fn node_file_trace_operation(
             // node-file-trace.
             environment: None,
             analyze_mode: AnalyzeMode::Tracing,
+            // Disable tree shaking. Even side-effect-free imports need to be traced, as they will
+            // execute at runtime.
+            tree_shaking_mode: None,
             ..Default::default()
         }
         .cell(),
diff --git a/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs b/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs
@@ -361,8 +361,11 @@ async fn node_file_trace_operation(
         Default::default(),
         // TODO These test cases should move into the `node-file-trace` crate and use the same
         // config.
-        // It's easy to make a mistake here as this should match the config in the binary from
-        // turbopack/crates/turbopack/src/lib.rs
+        // This config should be kept in sync with
+        // turbopack/crates/turbopack-tracing/tests/node-file-trace.rs and
+        // turbopack/crates/turbopack-tracing/tests/unit.rs and
+        // turbopack/crates/turbopack/src/lib.rs and
+        // turbopack/crates/turbopack-nft/src/nft.rs
         CompileTimeInfo::new(environment),
         ModuleOptionsContext {
             ecmascript: EcmascriptOptionsContext {
@@ -380,6 +383,9 @@ async fn node_file_trace_operation(
             // node-file-trace.
             environment: None,
             analyze_mode: AnalyzeMode::Tracing,
+            // Disable tree shaking. Even side-effect-free imports need to be traced, as they will
+            // execute at runtime.
+            tree_shaking_mode: None,
             ..Default::default()
         }
         .cell(),
diff --git a/turbopack/crates/turbopack-tracing/tests/unit.rs b/turbopack/crates/turbopack-tracing/tests/unit.rs
@@ -198,8 +198,11 @@ async fn node_file_trace_operation(package_root: RcStr, input: RcStr) -> Result<
         Default::default(),
         // TODO These test cases should move into the `node-file-trace` crate and use the same
         // config.
-        // It's easy to make a mistake here as this should match the config in the binary from
-        // turbopack/crates/turbopack/src/lib.rs
+        // This config should be kept in sync with
+        // turbopack/crates/turbopack-tracing/tests/node-file-trace.rs and
+        // turbopack/crates/turbopack-tracing/tests/unit.rs and
+        // turbopack/crates/turbopack/src/lib.rs and
+        // turbopack/crates/turbopack-nft/src/nft.rs
         CompileTimeInfo::new(environment),
         ModuleOptionsContext {
             ecmascript: EcmascriptOptionsContext {
@@ -216,6 +219,9 @@ async fn node_file_trace_operation(package_root: RcStr, input: RcStr) -> Result<
             // node-file-trace.
             environment: None,
             analyze_mode: AnalyzeMode::Tracing,
+            // Disable tree shaking. Even side-effect-free imports need to be traced, as they will
+            // execute at runtime.
+            tree_shaking_mode: None,
             ..Default::default()
         }
         .cell(),
diff --git a/turbopack/crates/turbopack/src/lib.rs b/turbopack/crates/turbopack/src/lib.rs
@@ -166,8 +166,12 @@ async fn apply_module_type(
             if runtime_code {
                 ResolvedVc::upcast(module)
             } else {
-                if matches!(&part, Some(ModulePart::Evaluation)) {
-                    // Skip the evaluation part if the module is marked as side effect free.
+                let options_value = options.await?;
+                if options_value.tree_shaking_mode.is_some()
+                    && matches!(&part, Some(ModulePart::Evaluation))
+                {
+                    // If we are tree shaking, skip the evaluation part if the module is marked as
+                    // side effect free.
                     let side_effect_free_packages = module_asset_context
                         .side_effect_free_packages()
                         .resolve()
@@ -181,7 +185,6 @@ async fn apply_module_type(
                     }
                 }
 
-                let options_value = options.await?;
                 match options_value.tree_shaking_mode {
                     Some(TreeShakingMode::ModuleFragments) => {
                         Vc::upcast(EcmascriptModulePartAsset::select_part(
@@ -740,9 +743,11 @@ pub async fn externals_tracing_module_context(
     Ok(ModuleAssetContext::new_without_replace_externals(
         Default::default(),
         compile_time_info,
-        // Keep these options more or less in sync with
-        // turbopack/crates/turbopack/tests/node-file-trace.rs to ensure that the NFT unit tests
-        // are actually representative of what Turbopack does.
+        // This config should be kept in sync with
+        // turbopack/crates/turbopack-tracing/tests/node-file-trace.rs and
+        // turbopack/crates/turbopack-tracing/tests/unit.rs and
+        // turbopack/crates/turbopack/src/lib.rs and
+        // turbopack/crates/turbopack-nft/src/nft.rs
         ModuleOptionsContext {
             ecmascript: EcmascriptOptionsContext {
                 enable_typescript_transform: Some(
@@ -762,6 +767,9 @@ pub async fn externals_tracing_module_context(
             // node-file-trace.
             environment: None,
             analyze_mode: AnalyzeMode::Tracing,
+            // Disable tree shaking. Even side-effect-free imports need to be traced, as they will
+            // execute at runtime.
+            tree_shaking_mode: None,
             ..Default::default()
         }
         .cell(),

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+// some noop, should still resolve and not crash though`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +{
 +  "dependencies": {
 +    "foo": "file:./foo"
 +  }
 +}