From 800012af69082db680b0673e8efc1e2d150fb01f Mon Sep 17 00:00:00 2001 From: lukekania Date: Mon, 18 May 2026 10:19:34 +0200 Subject: [PATCH 1/2] feat(bundler): per-provider tree-shake for vendor chunks (#171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalize cross-chunk used-names collection so every chunk gets its own externally-used set, not just main. Vendor chunks holding `@angular/core` or `rxjs` previously pinned every export the package declared because `externally_used = None` made the shaker fall back to entry-walk reachability — on `index.mjs`-style packages that reaches almost everything. - `shake::collect_cross_chunk_used_names_per_provider` returns `Vec>` indexed by chunk index; for each chunk i it collects the names other chunks import from any module in i. - `bundle()` builds a bare-specifier → canonical-path map from the existing namespace tables so bare imports (`'@angular/core'`) attribute to the owning vendor chunk, then feeds `externally_used_per_chunk[idx]` into `analyze_unused_exports` for every chunk — the `is_main` gate is dropped. - `npm_wrap::wrap_npm_module` now accepts `unused_exports` and drops both the unused `export const X = ...` declarations and the matching `__exports.X = ns.X` re-export bridges, so shake decisions reach the emitted vendor chunk code. Bumps version to 0.10.13. --- Cargo.lock | 20 +- Cargo.toml | 2 +- crates/bundler/src/concat.rs | 51 ++- crates/bundler/src/npm_wrap.rs | 94 ++++- crates/bundler/src/shake.rs | 355 +++++++++++++----- .../vendor_chunk_splitting_integration.rs | 173 +++++++++ 6 files changed, 552 insertions(+), 143 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e70b989..6bed691 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -730,7 +730,7 @@ dependencies = [ [[package]] name = "ngc-bundler" -version = "0.10.12" +version = "0.10.13" dependencies = [ "dashmap", "ngc-diagnostics", @@ -755,7 +755,7 @@ dependencies = [ [[package]] name = "ngc-dev-server" -version = "0.10.12" +version = "0.10.13" dependencies = [ "ngc-diagnostics", "serde_json", @@ -766,7 +766,7 @@ dependencies = [ [[package]] name = "ngc-diagnostics" -version = "0.10.12" +version = "0.10.13" dependencies = [ "serde_json", "thiserror", @@ -774,7 +774,7 @@ dependencies = [ [[package]] name = "ngc-linker" -version = "0.10.12" +version = "0.10.13" dependencies = [ "dashmap", "insta", @@ -792,7 +792,7 @@ dependencies = [ [[package]] name = "ngc-npm-resolver" -version = "0.10.12" +version = "0.10.13" dependencies = [ "dashmap", "ngc-diagnostics", @@ -807,7 +807,7 @@ dependencies = [ [[package]] name = "ngc-project-resolver" -version = "0.10.12" +version = "0.10.13" dependencies = [ "dashmap", "glob", @@ -823,7 +823,7 @@ dependencies = [ [[package]] name = "ngc-rs" -version = "0.10.12" +version = "0.10.13" dependencies = [ "base64", "clap", @@ -857,7 +857,7 @@ dependencies = [ [[package]] name = "ngc-template-compiler" -version = "0.10.12" +version = "0.10.13" dependencies = [ "insta", "ngc-diagnostics", @@ -879,7 +879,7 @@ dependencies = [ [[package]] name = "ngc-ts-transform" -version = "0.10.12" +version = "0.10.13" dependencies = [ "ngc-diagnostics", "oxc_allocator", @@ -898,7 +898,7 @@ dependencies = [ [[package]] name = "ngc-watch" -version = "0.10.12" +version = "0.10.13" dependencies = [ "ngc-diagnostics", "notify", diff --git a/Cargo.toml b/Cargo.toml index 3408cfc..44371e9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "2" members = ["crates/cli", "crates/diagnostics", "crates/project-resolver", "crates/ts-transform", "crates/bundler", "crates/template-compiler", "crates/npm-resolver", "crates/linker", "crates/watch", "crates/dev-server"] [workspace.package] -version = "0.10.12" +version = "0.10.13" edition = "2021" license = "MIT OR Apache-2.0" authors = ["lukekania"] diff --git a/crates/bundler/src/concat.rs b/crates/bundler/src/concat.rs index fc96c57..b06b40b 100644 --- a/crates/bundler/src/concat.rs +++ b/crates/bundler/src/concat.rs @@ -163,24 +163,40 @@ pub fn bundle(input: &BundleInput) -> NgcResult { .map(|c| c.filename.clone()) .collect(); - // Lazy chunks consume symbols from main cross-chunk; those consumptions - // are invisible to the per-chunk shake analysis. Precompute the union - // before fan-out so the main chunk's analyze_unused_exports preserves - // them. - let externally_used: Option> = if input.options.tree_shake { - let mut lazy_consumers: Vec = Vec::new(); - for chunk in &chunk_graph.chunks[1..] { - lazy_consumers.extend(chunk.modules.iter().cloned()); + // Every chunk consumes symbols from other chunks (main from lazy, lazy + // from main, lazy from vendor, ...) — those consumptions are invisible + // to per-chunk shake analysis. Precompute the per-provider used-name + // set before fan-out so each chunk's analyze_unused_exports preserves + // exactly what its consumers reach into. + // + // For bare npm specifiers (e.g. `'@angular/core'`), build a + // specifier → canonical-entry-path map by composing the bare-spec → + // namespace and namespace → owning-path lookups we already computed. + // Without this, the shake walker can't attribute an `import { X } from + // '@angular/core'` to its vendor chunk and the chunk falls back to + // pinning every export the package declares. + let externally_used_per_chunk: Vec> = if input.options.tree_shake { + let mut ns_to_path: HashMap<&str, &PathBuf> = HashMap::new(); + for (path, ns) in &all_file_to_ns { + ns_to_path.insert(ns.as_str(), path); } - Some(shake::collect_cross_chunk_used_names( - &lazy_consumers, - &main_chunk.modules, + let specifier_to_path: HashMap = specifier_to_namespace + .iter() + .filter_map(|(spec, ns)| { + ns_to_path + .get(ns.as_str()) + .map(|path| (spec.clone(), (*path).clone())) + }) + .collect(); + shake::collect_cross_chunk_used_names_per_provider( + &chunk_graph, &input.modules, &prefix_refs, + &specifier_to_path, subpath_ctx, - )?) + )? } else { - None + vec![HashSet::new(); chunk_graph.chunks.len()] }; // Process every chunk (main + lazy/shared) in a single rayon fan-out. @@ -194,13 +210,8 @@ pub fn bundle(input: &BundleInput) -> NgcResult { .par_iter() .enumerate() .map(|(idx, chunk)| -> NgcResult<(String, ChunkBundleResult)> { - let is_main = idx == 0; let unused_exports = if input.options.tree_shake { - let externally_used_ref = if is_main { - externally_used.as_ref() - } else { - None - }; + let externally_used_ref = externally_used_per_chunk.get(idx); shake::analyze_unused_exports( &chunk.modules, &input.modules, @@ -648,10 +659,12 @@ fn bundle_chunk(p: &ChunkBundleParams<'_>) -> NgcResult { // or another — cross-chunk refs become imports at the // chunk's top, emitted in the post-process pass. let namespace = &file_to_namespace[module_path]; + let module_unused = p.unused_exports.get(module_path); let wrapped = crate::npm_wrap::wrap_npm_module( js_code, &file_name, namespace, + module_unused, |specifier| { if specifier.starts_with('.') { let from_dir = module_path.parent()?; diff --git a/crates/bundler/src/npm_wrap.rs b/crates/bundler/src/npm_wrap.rs index 635caf9..64b6a04 100644 --- a/crates/bundler/src/npm_wrap.rs +++ b/crates/bundler/src/npm_wrap.rs @@ -15,7 +15,7 @@ //! })(__ns_abc123); //! ``` -use std::collections::{BTreeSet, HashMap}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::path::Path; use ngc_diagnostics::{NgcError, NgcResult}; @@ -39,15 +39,29 @@ pub struct NpmModuleInfo { /// /// `resolve_import` is a closure that maps an import specifier to a namespace /// variable name, or `None` if the import should be left as-is (truly external). +/// +/// `unused_exports` carries the set of exported names that per-provider +/// shake decided no consumer reaches. When provided, both the declarations +/// (`export const X = ...`) and the matching `__exports.X = ...` bridge +/// lines are dropped, shrinking vendor chunks of large packages like +/// `@angular/core` whose `index.mjs` re-exports far more than any consumer +/// actually uses. pub fn wrap_npm_module( js_code: &str, file_name: &str, namespace: &str, + unused_exports: Option<&HashSet>, resolve_import: F, ) -> NgcResult where F: Fn(&str) -> Option, { + let is_unused = |name: &str| -> bool { + unused_exports + .map(|set| set.contains(name)) + .unwrap_or(false) + }; + // Strip sourcemap comments upfront to prevent them from interfering with // the IIFE wrapping (they can eat export assignments on the same line). let cleaned_code = strip_sourcemap_comments(js_code); @@ -133,6 +147,9 @@ where for spec in &export.specifiers { let exported = spec.exported.name().to_string(); let local = spec.local.name().to_string(); + if is_unused(&exported) { + continue; + } // Don't add to exported_names — we handle the export inline if let Some(ref ns) = target_ns { replacements.push(format!("__exports.{exported} = {ns}.{local};")); @@ -150,21 +167,39 @@ where end: export.span.end, replacement, }); - } else if export.declaration.is_some() { - // export const X = ...; → strip "export " - if let Some(decl) = &export.declaration { - collect_decl_names(decl, &mut exported_names); + } else if let Some(decl) = &export.declaration { + // export const X = ...; — if X is unused, drop the + // entire declaration so the body isn't pinned by + // its `__exports.X = X` line and any const + // initializer side-effect is also eliminated. + let mut decl_names = Vec::new(); + collect_decl_names(decl, &mut decl_names); + let all_unused = + !decl_names.is_empty() && decl_names.iter().all(|n| is_unused(n)); + if all_unused { + edits.push(TextEdit { + start: export.span.start, + end: export.span.end, + replacement: None, + }); + } else { + for n in &decl_names { + exported_names.push(n.clone()); + } + edits.push(TextEdit { + start: export.span.start, + end: export.span.start + 7, // "export " + replacement: None, + }); } - edits.push(TextEdit { - start: export.span.start, - end: export.span.start + 7, // "export " - replacement: None, - }); } else { // export { X, Y }; or export { X as Y }; → collect names and remove for spec in &export.specifiers { let exported = spec.exported.name().to_string(); let local = spec.local.name().to_string(); + if is_unused(&exported) { + continue; + } if exported != local { renamed_exports.insert(exported.clone(), local); } @@ -428,7 +463,7 @@ mod tests { #[test] fn test_wrap_simple_module() { let code = "export function hello() { return 42; }\n"; - let result = wrap_npm_module(code, "test.js", "__ns_test", no_resolve).unwrap(); + let result = wrap_npm_module(code, "test.js", "__ns_test", None, no_resolve).unwrap(); assert!(result.wrapped_code.contains("var __ns_test = {};")); assert!(result.wrapped_code.contains("(function(__exports)")); assert!(result.wrapped_code.contains("__exports.hello = hello;")); @@ -447,7 +482,7 @@ mod tests { None } }; - let result = wrap_npm_module(code, "test.js", "__ns_test", resolve).unwrap(); + let result = wrap_npm_module(code, "test.js", "__ns_test", None, resolve).unwrap(); assert!(result .wrapped_code .contains("var Component = __ns_core.Component;")); @@ -465,7 +500,7 @@ mod tests { None } }; - let result = wrap_npm_module(code, "test.js", "__ns_test", resolve).unwrap(); + let result = wrap_npm_module(code, "test.js", "__ns_test", None, resolve).unwrap(); assert!(result .wrapped_code .contains("Object.assign(__exports, __ns_utils)")); @@ -474,11 +509,42 @@ mod tests { #[test] fn test_wrap_default_export() { let code = "export default function helper() { return 1; }\n"; - let result = wrap_npm_module(code, "test.js", "__ns_test", no_resolve).unwrap(); + let result = wrap_npm_module(code, "test.js", "__ns_test", None, no_resolve).unwrap(); assert!(result.wrapped_code.contains("function helper()")); assert!(result.wrapped_code.contains("__exports.default = helper;")); } + #[test] + fn test_wrap_drops_unused_declaration() { + let code = "export const used = 1;\nexport const unused = 2;\n"; + let mut unused: HashSet = HashSet::new(); + unused.insert("unused".to_string()); + let result = + wrap_npm_module(code, "test.js", "__ns_test", Some(&unused), no_resolve).unwrap(); + assert!(result.wrapped_code.contains("const used = 1")); + assert!(!result.wrapped_code.contains("const unused = 2")); + assert!(result.wrapped_code.contains("__exports.used = used;")); + assert!(!result.wrapped_code.contains("__exports.unused")); + } + + #[test] + fn test_wrap_drops_unused_reexport_bridge() { + let code = "export { used, unused } from './impl';\n"; + let resolve = |spec: &str| -> Option { + if spec == "./impl" { + Some("__ns_impl".to_string()) + } else { + None + } + }; + let mut unused: HashSet = HashSet::new(); + unused.insert("unused".to_string()); + let result = + wrap_npm_module(code, "test.js", "__ns_test", Some(&unused), resolve).unwrap(); + assert!(result.wrapped_code.contains("__exports.used = __ns_impl.used")); + assert!(!result.wrapped_code.contains("__exports.unused")); + } + #[test] fn test_namespace_from_path() { use std::path::PathBuf; diff --git a/crates/bundler/src/shake.rs b/crates/bundler/src/shake.rs index 29ae265..239e76a 100644 --- a/crates/bundler/src/shake.rs +++ b/crates/bundler/src/shake.rs @@ -337,106 +337,138 @@ fn resolve_local_specifier( None } -/// Collect symbol names imported by `consumer_modules` from any module in -/// `provider_modules`, by parsing each consumer's source for `ImportDeclaration` -/// statements and resolving their specifiers against the provider set. +/// Per-provider collection of names consumed cross-chunk. /// -/// Used by the bundler to tell the main-chunk tree-shaker which symbols are -/// consumed by lazy chunks and must therefore be preserved — such consumption -/// is invisible when shaking each chunk in isolation, and would otherwise -/// leave the cross-chunk `export { ... }` block referring to names whose -/// declarations have been tree-shaken away. +/// Returns a `Vec>` indexed by chunk index. `result[i]` holds +/// the set of names that modules in *other* chunks import from any module +/// owned by chunk `i`. Used by the bundler's per-chunk tree-shaker so a +/// vendor chunk holding `@angular/core` / `rxjs` can drop exports no +/// consumer references, instead of pinning every name the package declares +/// just because its entry walk happens to reach them. /// -/// For named and default imports, the specific name is collected. For -/// namespace imports (`import * as X from '...'`), every exported name of -/// the target provider module is collected since individual accesses can't -/// be known statically here. -pub fn collect_cross_chunk_used_names( - consumer_modules: &[PathBuf], - provider_modules: &[PathBuf], +/// `specifier_to_path` resolves bare npm specifiers (`'@angular/core'`) to +/// the canonical entry-module path so bare-specifier imports can be +/// attributed to their owning provider chunk. Relative and `#`-subpath +/// imports flow through [`resolve_local_specifier`] as usual. +pub fn collect_cross_chunk_used_names_per_provider( + chunk_graph: &crate::chunk::ChunkGraph, all_code: &HashMap, local_prefixes: &[&str], + specifier_to_path: &HashMap, subpath_ctx: Option>, -) -> NgcResult> { - let provider_set: HashSet<&PathBuf> = provider_modules.iter().collect(); +) -> NgcResult>> { + let n = chunk_graph.chunks.len(); + let mut result: Vec> = vec![HashSet::new(); n]; + if n == 0 { + return Ok(result); + } - // Phase A (parallel): parse each consumer and extract the named symbols - // it imports from provider modules. The parse dominates, so fanning this - // out across rayon workers recovers most of the tree-shake wall time. - let per_consumer: Vec> = consumer_modules + let module_to_chunk_idx = &chunk_graph.module_to_chunk_idx; + + // Flat (consumer_chunk_idx, consumer_path) list — every module across + // every chunk is a potential consumer of names in some other chunk. + let consumers: Vec<(usize, PathBuf)> = chunk_graph + .chunks + .iter() + .enumerate() + .flat_map(|(idx, chunk)| chunk.modules.iter().map(move |m| (idx, m.clone()))) + .collect(); + + // The full provider candidate set — every module across all chunks. + // `resolve_local_specifier` scans this when matching a relative import + // path; chunk membership is then read from `module_to_chunk_idx`. + let all_provider_paths: Vec = consumers.iter().map(|(_, p)| p.clone()).collect(); + + // Phase A (parallel): for each consumer, parse once and produce a list + // of (target_chunk_idx, imported_name) entries. Intra-chunk imports + // are dropped here — those are handled by `analyze_unused_exports`'s + // per-chunk reachability pass. + let per_consumer: Vec> = consumers .par_iter() - .filter_map(|consumer_path| { + .filter_map(|(consumer_idx, consumer_path)| { all_code .get(consumer_path) - .map(|code| (consumer_path, code)) + .map(|code| (*consumer_idx, consumer_path.clone(), code)) }) - .map(|(consumer_path, code)| -> NgcResult> { - let info = analyze_module(code, consumer_path)?; - let mut local_used: HashSet = HashSet::new(); - for (specifier, imported_names) in &info.local_imports { - let Some(target) = resolve_local_specifier( - specifier, - consumer_path, - provider_modules, - local_prefixes, - subpath_ctx, - ) else { - continue; - }; - if !provider_set.contains(&target) { - continue; - } - for name in imported_names { - if name == "* as " || name.starts_with("* as ") { - // ImportNamespaceSpecifier — `analyze_module` currently - // drops these (returns None). Left defensive; the - // namespace pass below handles the real case. + .map( + |(consumer_idx, consumer_path, code)| -> NgcResult> { + let info = analyze_module(code, &consumer_path)?; + let mut out: Vec<(usize, String)> = Vec::new(); + for (specifier, imported_names) in &info.local_imports { + let target_path = resolve_local_specifier( + specifier, + &consumer_path, + &all_provider_paths, + local_prefixes, + subpath_ctx, + ) + .or_else(|| specifier_to_path.get(specifier).cloned()); + + let Some(target) = target_path else { + continue; + }; + let Some(&target_idx) = module_to_chunk_idx.get(&target) else { continue; + }; + if target_idx == consumer_idx { + continue; + } + for name in imported_names { + out.push((target_idx, name.clone())); } - local_used.insert(name.clone()); } - } - Ok(local_used) - }) + Ok(out) + }, + ) .collect::>>()?; - let mut used: HashSet = HashSet::new(); - for names in per_consumer { - used.extend(names); + for entries in per_consumer { + for (idx, name) in entries { + if let Some(set) = result.get_mut(idx) { + set.insert(name); + } + } } - // Phase B (serial): namespace-import expansion. Kept serial so the - // `provider_exports` cache parses each provider at most once even when - // several consumers import the same namespace. + // Phase B (serial): namespace-import expansion. Each `import * as X + // from '...'` in a consumer adds every export of the target module to + // the owning chunk's used set. Provider parses are cached so a hot + // namespace import is parsed at most once. let mut provider_exports: HashMap> = HashMap::new(); - for consumer_path in consumer_modules { + for (consumer_idx, consumer_path) in &consumers { let Some(code) = all_code.get(consumer_path) else { continue; }; - expand_namespace_imports( + expand_namespace_imports_per_provider( code, consumer_path, - provider_modules, + *consumer_idx, + &all_provider_paths, + module_to_chunk_idx, + specifier_to_path, local_prefixes, all_code, &mut provider_exports, - &mut used, + &mut result, subpath_ctx, )?; } - Ok(used) + Ok(result) } #[allow(clippy::too_many_arguments)] -fn expand_namespace_imports( +fn expand_namespace_imports_per_provider( code: &str, consumer_path: &Path, + consumer_chunk_idx: usize, provider_modules: &[PathBuf], + module_to_chunk_idx: &HashMap, + specifier_to_path: &HashMap, local_prefixes: &[&str], all_code: &HashMap, provider_exports: &mut HashMap>, - used: &mut HashSet, + per_chunk_used: &mut [HashSet], subpath_ctx: Option>, ) -> NgcResult<()> { let allocator = Allocator::new(); @@ -460,15 +492,22 @@ fn expand_namespace_imports( continue; } let source = import.source.value.to_string(); - let Some(target) = resolve_local_specifier( + let target = resolve_local_specifier( &source, consumer_path, provider_modules, local_prefixes, subpath_ctx, - ) else { + ) + .or_else(|| specifier_to_path.get(&source).cloned()); + let Some(target) = target else { continue }; + let Some(&target_idx) = module_to_chunk_idx.get(&target) else { continue; }; + if target_idx == consumer_chunk_idx { + continue; + } + let exports = match provider_exports.get(&target) { Some(e) => e.clone(), None => { @@ -480,7 +519,9 @@ fn expand_namespace_imports( info.exported_names } }; - used.extend(exports); + if let Some(set) = per_chunk_used.get_mut(target_idx) { + set.extend(exports); + } } Ok(()) @@ -630,11 +671,13 @@ mod tests { } #[test] - fn test_collect_cross_chunk_used_names_dotted_filename() { + fn test_collect_cross_chunk_used_names_per_provider_dotted_filename() { // Regression: resolve_local_specifier previously used `with_extension`, // which treated `.service` as an existing extension and replaced it. // Imports like `./foo.service` then failed to resolve against // `foo.service.ts` and cross-chunk consumption was missed. + use crate::chunk::{Chunk, ChunkGraph, ChunkKind}; + let dir = tempfile::tempdir().expect("create temp dir"); let svc = dir.path().join("analytics.service.ts"); let comp = dir.path().join("comp.ts"); @@ -648,59 +691,173 @@ mod tests { let canon_svc = svc.canonicalize().expect("canon svc"); let canon_comp = comp.canonicalize().expect("canon comp"); - let mut modules = HashMap::new(); - modules.insert( + let mut all_code = HashMap::new(); + all_code.insert( canon_svc.clone(), "export class AnalyticsService {}\n".into(), ); - modules.insert( + all_code.insert( canon_comp.clone(), "import { AnalyticsService } from './analytics.service';\nnew AnalyticsService();\n" .into(), ); - let used = - collect_cross_chunk_used_names(&[canon_comp], &[canon_svc], &modules, &["."], None) - .expect("should collect"); + let chunks = vec![ + Chunk { + kind: ChunkKind::Main, + filename: "main.js".to_string(), + modules: vec![canon_svc.clone()], + entry: canon_svc.clone(), + }, + Chunk { + kind: ChunkKind::Lazy, + filename: "lazy.js".to_string(), + modules: vec![canon_comp.clone()], + entry: canon_comp.clone(), + }, + ]; + let mut module_to_chunk_idx: HashMap = HashMap::new(); + for (idx, chunk) in chunks.iter().enumerate() { + for m in &chunk.modules { + module_to_chunk_idx.insert(m.clone(), idx); + } + } + let chunk_graph = ChunkGraph { + chunks, + dynamic_import_map: HashMap::new(), + module_to_chunk_idx, + }; + + let per_provider = collect_cross_chunk_used_names_per_provider( + &chunk_graph, + &all_code, + &["."], + &HashMap::new(), + None, + ) + .expect("should collect"); assert!( - used.contains("AnalyticsService"), - "import of ./foo.service must resolve to foo.service.ts" + per_provider[0].contains("AnalyticsService"), + "import of ./foo.service must resolve to foo.service.ts: {per_provider:?}" ); } #[test] - fn test_collect_cross_chunk_used_names_named_import() { - // A lazy-chunk module imports AnalyticsService from a main-chunk module; - // collect_cross_chunk_used_names must surface it. Uses a real tempdir - // so resolve_local_specifier's canonicalize step can succeed. + fn test_collect_cross_chunk_used_names_per_provider_multi_chunk() { + // Three chunks: main (chunk 0), lazy (chunk 1) sourced via dynamic + // import, and a vendor chunk (chunk 2) providing an npm-style + // module. The lazy chunk imports one name from main and one name + // from vendor; main imports nothing externally. Per-provider + // result must attribute each import to the correct chunk only. + use crate::chunk::{Chunk, ChunkGraph, ChunkKind}; + let dir = tempfile::tempdir().expect("create temp dir"); - let main_svc = dir.path().join("svc.js"); - let lazy_dir = dir.path().join("lazy"); - std::fs::create_dir_all(&lazy_dir).expect("create lazy dir"); - let lazy_comp = lazy_dir.join("comp.js"); - std::fs::write(&main_svc, "export class AnalyticsService {}\n").expect("write svc"); + let main_path = dir.path().join("main.js"); + let svc_path = dir.path().join("svc.js"); + let lazy_path = dir.path().join("lazy.js"); + let vendor_path = dir.path().join("vendor_pkg.js"); + + std::fs::write(&main_path, "// main entry\n").expect("write main"); + std::fs::write(&svc_path, "export class MainService {}\n").expect("write svc"); std::fs::write( - &lazy_comp, - "import { AnalyticsService } from '../svc';\nnew AnalyticsService();\n", + &lazy_path, + "import { MainService } from './svc';\n\ + import { vendorFn } from 'vendor-pkg';\n\ + new MainService(); vendorFn();\n", ) - .expect("write comp"); + .expect("write lazy"); + std::fs::write( + &vendor_path, + "export const vendorFn = () => 1;\nexport const vendorUnused = () => 2;\n", + ) + .expect("write vendor"); + + let canon_main = main_path.canonicalize().expect("canon main"); + let canon_svc = svc_path.canonicalize().expect("canon svc"); + let canon_lazy = lazy_path.canonicalize().expect("canon lazy"); + let canon_vendor = vendor_path.canonicalize().expect("canon vendor"); + + let mut all_code: HashMap = HashMap::new(); + all_code.insert(canon_main.clone(), "// main entry\n".into()); + all_code.insert(canon_svc.clone(), "export class MainService {}\n".into()); + all_code.insert( + canon_lazy.clone(), + "import { MainService } from './svc';\n\ + import { vendorFn } from 'vendor-pkg';\n\ + new MainService(); vendorFn();\n" + .into(), + ); + all_code.insert( + canon_vendor.clone(), + "export const vendorFn = () => 1;\nexport const vendorUnused = () => 2;\n".into(), + ); + + let chunks = vec![ + Chunk { + kind: ChunkKind::Main, + filename: "main.js".to_string(), + modules: vec![canon_main.clone(), canon_svc.clone()], + entry: canon_main.clone(), + }, + Chunk { + kind: ChunkKind::Lazy, + filename: "lazy.js".to_string(), + modules: vec![canon_lazy.clone()], + entry: canon_lazy.clone(), + }, + Chunk { + kind: ChunkKind::Shared, + filename: "vendor.js".to_string(), + modules: vec![canon_vendor.clone()], + entry: canon_vendor.clone(), + }, + ]; + let mut module_to_chunk_idx: HashMap = HashMap::new(); + for (idx, chunk) in chunks.iter().enumerate() { + for m in &chunk.modules { + module_to_chunk_idx.insert(m.clone(), idx); + } + } + let chunk_graph = ChunkGraph { + chunks, + dynamic_import_map: HashMap::new(), + module_to_chunk_idx, + }; - let canon_svc = main_svc.canonicalize().expect("canon svc"); - let canon_comp = lazy_comp.canonicalize().expect("canon comp"); + let mut specifier_to_path: HashMap = HashMap::new(); + specifier_to_path.insert("vendor-pkg".to_string(), canon_vendor.clone()); - let mut modules = HashMap::new(); - modules.insert( - canon_svc.clone(), - "export class AnalyticsService {}\n".into(), + let per_provider = collect_cross_chunk_used_names_per_provider( + &chunk_graph, + &all_code, + &["."], + &specifier_to_path, + None, + ) + .expect("should collect"); + + assert_eq!(per_provider.len(), 3); + assert!( + per_provider[0].contains("MainService"), + "lazy's import of MainService should land in main's set: {per_provider:?}" ); - modules.insert( - canon_comp.clone(), - "import { AnalyticsService } from '../svc';\nnew AnalyticsService();\n".into(), + assert!( + !per_provider[0].contains("vendorFn"), + "vendorFn must not be attributed to main" + ); + assert!( + per_provider[1].is_empty(), + "no one imports from the lazy chunk; its set must be empty: {:?}", + per_provider[1] + ); + assert!( + per_provider[2].contains("vendorFn"), + "lazy's `import {{ vendorFn }} from 'vendor-pkg'` must land in vendor's set: {per_provider:?}" + ); + assert!( + !per_provider[2].contains("vendorUnused"), + "vendorUnused is never imported — must not be in vendor's set" ); - - let used = - collect_cross_chunk_used_names(&[canon_comp], &[canon_svc], &modules, &["."], None) - .expect("should collect"); - assert!(used.contains("AnalyticsService")); } + } diff --git a/crates/bundler/tests/vendor_chunk_splitting_integration.rs b/crates/bundler/tests/vendor_chunk_splitting_integration.rs index cf1ab6d..55ab766 100644 --- a/crates/bundler/tests/vendor_chunk_splitting_integration.rs +++ b/crates/bundler/tests/vendor_chunk_splitting_integration.rs @@ -262,6 +262,179 @@ fn lazy_only_vendor_chunk_is_not_initial() { ); } +/// Per-provider shake (issue #171): if a vendor chunk's npm module exports +/// both a name some other chunk imports and a name no consumer touches, only +/// the consumed name must survive in the emitted vendor chunk code. Before +/// per-provider shake, vendor chunks pinned every export the package +/// declared because `externally_used` was `None` and the entry-walk reached +/// every name. Now each chunk's tree-shaker gets its own externally-used +/// set computed from cross-chunk imports. +#[test] +fn vendor_chunk_drops_unreferenced_exports() { + let temp = tempdir().expect("create temp dir"); + let root = temp.path(); + + fs::write( + root.join("tsconfig.json"), + r#"{ "include": ["src/**/*.ts"], "exclude": [] }"#, + ) + .expect("write tsconfig"); + + fs::write( + root.join("package.json"), + r#"{ "name": "shake-fixture", "dependencies": { "shake-pkg": "1.0.0" } }"#, + ) + .expect("write package.json"); + + let src = root.join("src"); + fs::create_dir_all(&src).expect("create src"); + fs::write( + src.join("main.ts"), + "function loadA(){return import('./route-a');}\n\ + function loadB(){return import('./route-b');}\n\ + console.log(loadA, loadB);\n", + ) + .expect("write main.ts"); + // Both lazy routes import only `usedSentinel` from the npm package. + // `unusedSentinel` has no consumer anywhere in the bundle. + fs::write( + src.join("route-a.ts"), + "import { usedSentinel } from 'shake-pkg';\n\ + export const A = () => usedSentinel('a');\n", + ) + .expect("write route-a.ts"); + fs::write( + src.join("route-b.ts"), + "import { usedSentinel } from 'shake-pkg';\n\ + export const B = () => usedSentinel('b');\n", + ) + .expect("write route-b.ts"); + + // The npm package is split: a re-export entry plus an implementation + // file. Lexicographic order picks `a-entry.mjs` as the chunk entry, so + // `impl.js`'s declarations are subject to per-provider shake (not + // pinned by the entry-always-kept rule that protects entry's own + // export names). + let pkg_dir = root.join("node_modules").join("shake-pkg"); + fs::create_dir_all(&pkg_dir).expect("create pkg dir"); + fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "shake-pkg", "version": "1.0.0", "main": "a-entry.mjs" }"#, + ) + .expect("write pkg package.json"); + fs::write( + pkg_dir.join("a-entry.mjs"), + "export { usedSentinel, unusedSentinel } from './impl';\n", + ) + .expect("write a-entry.mjs"); + fs::write( + pkg_dir.join("impl.js"), + "export const usedSentinel = (x) => `USED_SENTINEL:${x}`;\n\ + export const unusedSentinel = (x) => `UNUSED_SENTINEL:${x}`;\n", + ) + .expect("write impl.js"); + + let file_graph = resolve_project(&root.join("tsconfig.json")).expect("resolve project"); + let entry = file_graph + .entry_points + .iter() + .find(|p| p.file_name().is_some_and(|n| n == "main.ts")) + .cloned() + .expect("main.ts entry"); + let bare_specs: Vec = file_graph.npm_import_sites.keys().cloned().collect(); + let npm = resolve_npm_dependencies(&bare_specs, root, DEVELOPMENT_BROWSER_CONDITIONS) + .expect("npm resolution"); + + let mut graph = file_graph.graph; + let mut path_index = file_graph.path_index; + for path in npm.modules.keys() { + if !path_index.contains_key(path) { + let idx = graph.add_node(path.clone()); + path_index.insert(path.clone(), idx); + } + } + for (spec, sites) in &file_graph.npm_import_sites { + if let Some(target_path) = npm + .modules + .keys() + .find(|p| p.to_string_lossy().contains(&format!("/{spec}/a-entry.mjs"))) + { + let to_idx = path_index[target_path]; + for (from_file, kind) in sites { + if let Some(&from_idx) = path_index.get(from_file) { + graph.add_edge(from_idx, to_idx, *kind); + } + } + } + } + // Wire the re-export edge a-entry.mjs -> impl.js so chunk graph keeps + // them in the same vendor partition. + let entry_path = npm + .modules + .keys() + .find(|p| p.to_string_lossy().ends_with("/a-entry.mjs")) + .cloned() + .expect("a-entry.mjs in npm.modules"); + let impl_path = npm + .modules + .keys() + .find(|p| p.to_string_lossy().ends_with("/impl.js")) + .cloned() + .expect("impl.js in npm.modules"); + graph.add_edge( + path_index[&entry_path], + path_index[&impl_path], + ngc_project_resolver::ImportKind::Static, + ); + + let mut modules: HashMap = HashMap::new(); + for idx in graph.node_indices() { + let path = &graph[idx]; + let source = npm + .modules + .get(path) + .cloned() + .or_else(|| fs::read_to_string(path).ok()) + .unwrap_or_else(|| panic!("source missing for {}", path.display())); + modules.insert(path.clone(), source); + } + + let input = BundleInput { + modules, + graph, + entry, + local_prefixes: vec![".".to_string()], + root_dir: root.to_path_buf(), + options: BundleOptions { + tree_shake: true, + ..BundleOptions::default() + }, + per_module_maps: HashMap::new(), + bundled_specifiers: npm.resolved_specifiers.clone(), + export_conditions: Vec::new(), + external_specifiers: Default::default(), + }; + + let output = bundle(&input).expect("bundle succeeds"); + + let vendor_name = output + .chunk_kinds + .iter() + .find(|(_, k)| **k == ChunkKind::Shared) + .map(|(n, _)| n.clone()) + .expect("vendor chunk"); + let vendor_code = &output.chunks[&vendor_name]; + + assert!( + vendor_code.contains("USED_SENTINEL"), + "used export must survive in vendor chunk: {vendor_code}" + ); + assert!( + !vendor_code.contains("UNUSED_SENTINEL"), + "unreferenced export must be tree-shaken from vendor chunk: {vendor_code}" + ); +} + /// Determinism: bundling the same input twice produces byte-identical chunk /// filenames + content. Vendor naming hashes absolute module paths, so we /// build twice in the same temp directory (real builds have a stable project From 05b30d0e654f858e5fbda34843e9a3f9f86e15f9 Mon Sep 17 00:00:00 2001 From: lukekania Date: Mon, 18 May 2026 10:51:29 +0200 Subject: [PATCH 2/2] chore: cargo fmt --- crates/bundler/src/npm_wrap.rs | 7 ++++--- crates/bundler/src/shake.rs | 1 - .../bundler/tests/vendor_chunk_splitting_integration.rs | 9 ++++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/bundler/src/npm_wrap.rs b/crates/bundler/src/npm_wrap.rs index 64b6a04..b343931 100644 --- a/crates/bundler/src/npm_wrap.rs +++ b/crates/bundler/src/npm_wrap.rs @@ -539,9 +539,10 @@ mod tests { }; let mut unused: HashSet = HashSet::new(); unused.insert("unused".to_string()); - let result = - wrap_npm_module(code, "test.js", "__ns_test", Some(&unused), resolve).unwrap(); - assert!(result.wrapped_code.contains("__exports.used = __ns_impl.used")); + let result = wrap_npm_module(code, "test.js", "__ns_test", Some(&unused), resolve).unwrap(); + assert!(result + .wrapped_code + .contains("__exports.used = __ns_impl.used")); assert!(!result.wrapped_code.contains("__exports.unused")); } diff --git a/crates/bundler/src/shake.rs b/crates/bundler/src/shake.rs index 239e76a..e2d4708 100644 --- a/crates/bundler/src/shake.rs +++ b/crates/bundler/src/shake.rs @@ -859,5 +859,4 @@ mod tests { "vendorUnused is never imported — must not be in vendor's set" ); } - } diff --git a/crates/bundler/tests/vendor_chunk_splitting_integration.rs b/crates/bundler/tests/vendor_chunk_splitting_integration.rs index 55ab766..b7a9b39 100644 --- a/crates/bundler/tests/vendor_chunk_splitting_integration.rs +++ b/crates/bundler/tests/vendor_chunk_splitting_integration.rs @@ -354,11 +354,10 @@ fn vendor_chunk_drops_unreferenced_exports() { } } for (spec, sites) in &file_graph.npm_import_sites { - if let Some(target_path) = npm - .modules - .keys() - .find(|p| p.to_string_lossy().contains(&format!("/{spec}/a-entry.mjs"))) - { + if let Some(target_path) = npm.modules.keys().find(|p| { + p.to_string_lossy() + .contains(&format!("/{spec}/a-entry.mjs")) + }) { let to_idx = path_index[target_path]; for (from_file, kind) in sites { if let Some(&from_idx) = path_index.get(from_file) {