diff --git a/Cargo.lock b/Cargo.lock index e6b09e0..c83a18c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -730,7 +730,7 @@ dependencies = [ [[package]] name = "ngc-bundler" -version = "0.10.9" +version = "0.10.10" dependencies = [ "dashmap", "ngc-diagnostics", @@ -755,7 +755,7 @@ dependencies = [ [[package]] name = "ngc-dev-server" -version = "0.10.9" +version = "0.10.10" dependencies = [ "ngc-diagnostics", "serde_json", @@ -766,7 +766,7 @@ dependencies = [ [[package]] name = "ngc-diagnostics" -version = "0.10.9" +version = "0.10.10" dependencies = [ "serde_json", "thiserror", @@ -774,7 +774,7 @@ dependencies = [ [[package]] name = "ngc-linker" -version = "0.10.9" +version = "0.10.10" dependencies = [ "dashmap", "insta", @@ -792,7 +792,7 @@ dependencies = [ [[package]] name = "ngc-npm-resolver" -version = "0.10.9" +version = "0.10.10" dependencies = [ "dashmap", "ngc-diagnostics", @@ -807,7 +807,7 @@ dependencies = [ [[package]] name = "ngc-project-resolver" -version = "0.10.9" +version = "0.10.10" dependencies = [ "dashmap", "glob", @@ -823,7 +823,7 @@ dependencies = [ [[package]] name = "ngc-rs" -version = "0.10.9" +version = "0.10.10" dependencies = [ "base64", "clap", @@ -857,7 +857,7 @@ dependencies = [ [[package]] name = "ngc-template-compiler" -version = "0.10.9" +version = "0.10.10" dependencies = [ "insta", "ngc-diagnostics", @@ -879,7 +879,7 @@ dependencies = [ [[package]] name = "ngc-ts-transform" -version = "0.10.9" +version = "0.10.10" dependencies = [ "ngc-diagnostics", "oxc_allocator", @@ -898,7 +898,7 @@ dependencies = [ [[package]] name = "ngc-watch" -version = "0.10.9" +version = "0.10.10" dependencies = [ "ngc-diagnostics", "notify", diff --git a/Cargo.toml b/Cargo.toml index 8c90479..cadb00f 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.9" +version = "0.10.10" 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 bf6ac5d..fc96c57 100644 --- a/crates/bundler/src/concat.rs +++ b/crates/bundler/src/concat.rs @@ -66,6 +66,15 @@ pub struct BundleInput { /// Bare specifiers that have been resolved and included in the graph. /// The rewriter treats imports of these specifiers as local (strips them). pub bundled_specifiers: HashSet, + /// Bare specifiers declared as external (`externalDependencies` in + /// `angular.json`). Imports of these specifiers — or of subpaths of + /// them, e.g. `jquery/dist/jquery.slim` — stay as bare ESM specifiers + /// in the emitted bundle, with no namespace rewrite and no inlining. + /// This list is enforced *in addition to* the absence of an entry in + /// `bundled_specifiers`: if a specifier appears in `external_specifiers`, + /// it is always treated as external, even if some upstream pass leaked + /// it into `bundled_specifiers`. + pub external_specifiers: HashSet, /// Active `exports` conditions (e.g. `browser`, `import`, `production`). /// Forwarded to the npm resolver when re-resolving specifiers during /// bundling so the same branch of conditional exports selected during @@ -215,6 +224,7 @@ pub fn bundle(input: &BundleInput) -> NgcResult { generate_source_maps: input.options.source_maps, unused_exports: &unused_exports, bundled_specifiers: &input.bundled_specifiers, + external_specifiers: &input.external_specifiers, chunk_entry: &chunk.entry, chunk_kind: &chunk.kind, chunk_module_set: &chunk_module_set, @@ -537,6 +547,9 @@ struct ChunkBundleParams<'a> { generate_source_maps: bool, unused_exports: &'a HashMap>, bundled_specifiers: &'a HashSet, + /// Specifiers declared external via `externalDependencies` in + /// `angular.json`. Imports matching these stay as bare specifiers. + external_specifiers: &'a HashSet, /// The chunk's entry module — exports from this module are preserved. chunk_entry: &'a Path, /// The kind of chunk being bundled (Main, Lazy, or Shared). @@ -707,6 +720,7 @@ fn bundle_chunk(p: &ChunkBundleParams<'_>) -> NgcResult { module_unused, effective_bundled, effective_ns_map, + p.external_specifiers, is_chunk_entry, )?; @@ -743,8 +757,16 @@ fn bundle_chunk(p: &ChunkBundleParams<'_>) -> NgcResult { if is_lazy { let mut main_js_named = BTreeSet::new(); let mut main_js_default = None; + // Externalised imports survive intact — they are bare specifiers the + // runtime resolves (import map, CDN). They are NOT re-exported from + // main, NOT routed cross-chunk, and NOT folded into `npm_externals`. + let mut keep_as_bare: Vec = Vec::new(); for ext in all_externals { + if rewrite::matches_external_specifier(&ext.source, p.external_specifiers) { + keep_as_bare.push(ext); + continue; + } let is_from_npm = ext.source.starts_with("__resolved_ns__") || ext.source.starts_with("__npm_") || p.bundled_specifiers.contains(&ext.source) @@ -775,7 +797,7 @@ fn bundle_chunk(p: &ChunkBundleParams<'_>) -> NgcResult { } } - all_externals = Vec::new(); + all_externals = keep_as_bare; if let Some(default_name) = main_js_default { main_js_named.insert(default_name); @@ -891,6 +913,14 @@ fn classify_lazy_externals( let mut out: Vec = Vec::with_capacity(externals.len()); for ext in externals { + // Externalised dependencies (`externalDependencies` in angular.json) + // stay as bare specifiers — they neither resolve to a chunk module + // nor get routed through `./main.js`, so pass them through verbatim + // and let the lazy-chunk routing block keep them in `keep_as_bare`. + if rewrite::matches_external_specifier(&ext.source, p.external_specifiers) { + out.push(ext); + continue; + } // Subpath imports (`#foo`) point at a file under the importing // package's `imports` map — typically a project file, occasionally // a bare specifier. Resolve to the actual target so chunk-membership @@ -1426,6 +1456,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1473,6 +1504,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1485,6 +1517,95 @@ mod tests { assert!(result.contains("Injectable")); } + #[test] + fn test_external_specifier_kept_as_bare_import_even_if_bundled() { + // Regression for issue #146: when a specifier appears in + // `external_specifiers`, the rewriter must leave the import as a + // bare ESM import, even if `bundled_specifiers` happens to contain + // the same string (the negative set vetoes the positive one). + let mut graph = DiGraph::new(); + let entry = graph.add_node(make_path("/root/main.ts")); + let _ = entry; + + let mut modules = HashMap::new(); + modules.insert( + make_path("/root/main.ts"), + "import $ from 'jquery';\nimport { trim } from 'jquery';\nconsole.log($, trim);\n" + .to_string(), + ); + + let mut bundled = HashSet::new(); + bundled.insert("jquery".to_string()); // simulate it leaking in + let mut externals = HashSet::new(); + externals.insert("jquery".to_string()); + + let input = BundleInput { + modules, + graph, + entry: make_path("/root/main.ts"), + local_prefixes: vec![".".to_string()], + root_dir: make_path("/root"), + options: BundleOptions::default(), + per_module_maps: HashMap::new(), + bundled_specifiers: bundled, + external_specifiers: externals, + export_conditions: Vec::new(), + }; + + let output = bundle(&input).expect("should bundle"); + let result = main_chunk(&output); + assert!( + result.contains("from 'jquery'"), + "external jquery import should survive verbatim, got:\n{result}" + ); + // Verify it's a hoisted ESM import — not rewritten into a + // `var $ = __ns_jquery.default` namespace assignment. + assert!( + !result.contains("__ns_jquery"), + "external import must not be rewritten to namespace; got:\n{result}" + ); + } + + #[test] + fn test_external_specifier_subpath_kept_as_bare_import() { + // `externalDependencies: ["jquery"]` should also externalise + // subpath imports like `jquery/dist/jquery.slim`, matching how + // esbuild's `--external: jquery` behaves under + // `@angular/build:application`. + let mut graph = DiGraph::new(); + let entry = graph.add_node(make_path("/root/main.ts")); + let _ = entry; + + let mut modules = HashMap::new(); + modules.insert( + make_path("/root/main.ts"), + "import $ from 'jquery/dist/jquery.slim';\nconsole.log($);\n".to_string(), + ); + + let mut externals = HashSet::new(); + externals.insert("jquery".to_string()); + + let input = BundleInput { + modules, + graph, + entry: make_path("/root/main.ts"), + local_prefixes: vec![".".to_string()], + root_dir: make_path("/root"), + options: BundleOptions::default(), + per_module_maps: HashMap::new(), + bundled_specifiers: HashSet::new(), + external_specifiers: externals, + export_conditions: Vec::new(), + }; + + let output = bundle(&input).expect("should bundle"); + let result = main_chunk(&output); + assert!( + result.contains("from 'jquery/dist/jquery.slim'"), + "subpath of external package should survive verbatim, got:\n{result}" + ); + } + #[test] fn test_unreachable_module_excluded() { let mut graph = DiGraph::new(); @@ -1516,6 +1637,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1560,6 +1682,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1632,6 +1755,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1766,6 +1890,7 @@ mod tests { }, per_module_maps, bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1816,6 +1941,7 @@ mod tests { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; @@ -1874,6 +2000,7 @@ mod tests { }, per_module_maps: HashMap::new(), bundled_specifiers: HashSet::new(), + external_specifiers: HashSet::new(), export_conditions: Vec::new(), }; diff --git a/crates/bundler/src/rewrite.rs b/crates/bundler/src/rewrite.rs index ae918f1..15dde5c 100644 --- a/crates/bundler/src/rewrite.rs +++ b/crates/bundler/src/rewrite.rs @@ -66,6 +66,7 @@ pub fn rewrite_module( None, &HashSet::new(), &HashMap::new(), + &HashSet::new(), false, ) } @@ -83,6 +84,7 @@ pub fn rewrite_module_with_shaking( unused_exports: Option<&HashSet>, bundled_specifiers: &HashSet, namespace_map: &HashMap, + external_specifiers: &HashSet, preserve_exports: bool, ) -> NgcResult { let allocator = Allocator::new(); @@ -116,6 +118,7 @@ pub fn rewrite_module_with_shaking( unused_exports, bundled_specifiers, namespace_map, + external_specifiers, preserve_exports, ) } else { @@ -158,12 +161,13 @@ fn collect_module_decl_edits( unused_exports: Option<&HashSet>, bundled_specifiers: &HashSet, namespace_map: &HashMap, + external_specifiers: &HashSet, preserve_exports: bool, ) -> bool { match module_decl { ModuleDeclaration::ImportDeclaration(import) => { let source = import.source.value.as_str(); - if is_local(source, local_prefixes, bundled_specifiers) { + if is_local(source, local_prefixes, bundled_specifiers, external_specifiers) { // Check if this import has a namespace mapping (npm module) if let Some(ns) = namespace_map.get(source) { // Replace import with namespace lookups @@ -326,6 +330,7 @@ fn collect_module_decl_edits( export.source.value.as_str(), local_prefixes, bundled_specifiers, + external_specifiers, ) => { edits.push(TextEdit { @@ -774,17 +779,46 @@ fn get_declaration_name(decl: &oxc_ast::ast::Declaration) -> Option { } /// Check if an import specifier is local based on known prefixes or bundled specifiers. +/// +/// `external_specifiers` is a *veto* set — when a specifier matches one of +/// its entries (by exact name or `/...` subpath), it is treated as +/// external no matter what else would classify it. This is how +/// `angular.json`'s `externalDependencies` keeps imports like +/// `import $ from 'jquery'` from being inlined. fn is_local( specifier: &str, local_prefixes: &[&str], bundled_specifiers: &HashSet, + external_specifiers: &HashSet, ) -> bool { + if matches_external_specifier(specifier, external_specifiers) { + return false; + } local_prefixes .iter() .any(|prefix| specifier.starts_with(prefix)) || bundled_specifiers.contains(specifier) } +/// Returns true when `specifier` is either an exact entry in +/// `external_specifiers` or a subpath of one (e.g. `jquery/dist/jquery.slim` +/// when `external_specifiers` lists `jquery`). Mirrors esbuild's `--external` +/// matching, which is what `@angular/build:application` uses under the hood. +pub(crate) fn matches_external_specifier( + specifier: &str, + external_specifiers: &HashSet, +) -> bool { + if external_specifiers.is_empty() { + return false; + } + if external_specifiers.contains(specifier) { + return true; + } + external_specifiers + .iter() + .any(|ext| specifier.starts_with(ext) && specifier[ext.len()..].starts_with('/')) +} + /// Apply text edits to the source, producing the rewritten code. fn apply_edits(source: &str, edits: &mut [TextEdit]) -> String { // Sort in reverse order so later edits don't shift earlier offsets @@ -1094,6 +1128,7 @@ mod tests { Some(&unused), &HashSet::new(), &HashMap::new(), + &HashSet::new(), false, ) .expect("should rewrite"); @@ -1128,6 +1163,7 @@ mod tests { Some(&empty_unused), &HashSet::new(), &HashMap::new(), + &HashSet::new(), false, ) .expect("should rewrite"); @@ -1155,6 +1191,7 @@ mod tests { Some(&unused), &HashSet::new(), &HashMap::new(), + &HashSet::new(), false, ) .expect("should rewrite"); @@ -1185,6 +1222,7 @@ mod tests { Some(&empty_unused), &HashSet::new(), &HashMap::new(), + &HashSet::new(), false, ) .expect("should rewrite"); diff --git a/crates/bundler/tests/defer_integration.rs b/crates/bundler/tests/defer_integration.rs index d55c8d1..c71e752 100644 --- a/crates/bundler/tests/defer_integration.rs +++ b/crates/bundler/tests/defer_integration.rs @@ -116,6 +116,7 @@ fn defer_deferred_component_is_chunk_split_placeholder_stays_in_main() { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: Default::default(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; diff --git a/crates/bundler/tests/subpath_imports_integration.rs b/crates/bundler/tests/subpath_imports_integration.rs index 7fa66ad..fb1570f 100644 --- a/crates/bundler/tests/subpath_imports_integration.rs +++ b/crates/bundler/tests/subpath_imports_integration.rs @@ -134,6 +134,7 @@ fn subpath_import_helper_is_inlined_into_main_chunk() { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: npm.resolved_specifiers.clone(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; @@ -270,6 +271,7 @@ fn subpath_import_in_lazy_chunk_does_not_leak_to_main_import() { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: npm.resolved_specifiers.clone(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; @@ -421,6 +423,7 @@ fn subpath_import_const_referenced_from_class_field_survives_tree_shake() { }, per_module_maps: HashMap::new(), bundled_specifiers: npm.resolved_specifiers.clone(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; @@ -526,6 +529,7 @@ fn relative_import_const_referenced_from_class_field_survives_tree_shake() { }, per_module_maps: HashMap::new(), bundled_specifiers: npm.resolved_specifiers.clone(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; diff --git a/crates/bundler/tests/vendor_chunk_splitting_integration.rs b/crates/bundler/tests/vendor_chunk_splitting_integration.rs index 9efca34..cf1ab6d 100644 --- a/crates/bundler/tests/vendor_chunk_splitting_integration.rs +++ b/crates/bundler/tests/vendor_chunk_splitting_integration.rs @@ -135,6 +135,7 @@ fn build_two_lazy_routes_sharing_npm(root: &std::path::Path) -> BundleInput { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: npm.resolved_specifiers.clone(), + external_specifiers: Default::default(), export_conditions: Vec::new(), } } diff --git a/crates/bundler/tests/worker_integration.rs b/crates/bundler/tests/worker_integration.rs index f3c98a7..9c57fb6 100644 --- a/crates/bundler/tests/worker_integration.rs +++ b/crates/bundler/tests/worker_integration.rs @@ -81,6 +81,7 @@ fn worker_new_url_is_bundled_as_separate_chunk_and_rewritten() { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: Default::default(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; @@ -207,6 +208,7 @@ fn worker_url_in_class_constructor_under_web_worker_dir_is_rewritten() { options: BundleOptions::default(), per_module_maps: HashMap::new(), bundled_specifiers: Default::default(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; @@ -301,6 +303,7 @@ fn worker_url_rewrite_uses_content_hashed_filename() { }, per_module_maps: HashMap::new(), bundled_specifiers: Default::default(), + external_specifiers: Default::default(), export_conditions: Vec::new(), }; diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index d497255..5aa980a 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -687,19 +687,40 @@ pub(crate) fn run_build_with_options( // Collect bare specifiers from project scanning AND from transformed output // (oxc may inject new imports like @oxc-project/runtime/helpers/decorate) let npm_span = tracing::info_span!("npm_resolve").entered(); - let mut bare_specifiers: Vec = file_graph.npm_import_sites.keys().cloned().collect(); + // `externalDependencies` from angular.json — these are NOT bundled + // (their imports stay as bare ESM specifiers for the runtime to + // resolve via an import map / CDN). Build the set once and use it + // to filter every list we hand to `resolve_npm_dependencies` so the + // BFS never walks into an externalised package's modules. + let external_specifiers: std::collections::HashSet = angular_project + .as_ref() + .map(|ap| ap.external_dependencies.iter().cloned().collect()) + .unwrap_or_default(); + let is_external = |spec: &str| -> bool { + external_specifiers.contains(spec) + || external_specifiers.iter().any(|ext| { + spec.starts_with(ext.as_str()) && spec[ext.len()..].starts_with('/') + }) + }; + let mut bare_specifiers: Vec = file_graph + .npm_import_sites + .keys() + .filter(|s| !is_external(s)) + .cloned() + .collect(); let post_transform_specifiers = scan_transformed_bare_specifiers(&modules, &local_prefixes); for spec in post_transform_specifiers { - if !bare_specifiers.contains(&spec) { + if !bare_specifiers.contains(&spec) && !is_external(&spec) { bare_specifiers.push(spec); } } let export_conditions = ngc_npm_resolver::package_json::conditions_for_configuration(configuration); - let mut npm_resolution = ngc_npm_resolver::resolve_npm_dependencies( + let mut npm_resolution = ngc_npm_resolver::resolve_npm_dependencies_with_externals( &bare_specifiers, &config_dir, export_conditions, + &external_specifiers, )?; // Merge npm modules into the modules map (they're already JS — no transform needed) @@ -759,7 +780,7 @@ pub(crate) fn run_build_with_options( let prescan_new: Vec = if public_exports.has_specifier_outside(&bare_set) { ngc_linker::flatten::scan_introduced_specifiers(&modules, ®istry, &public_exports) .into_iter() - .filter(|s| !bare_set.contains(s)) + .filter(|s| !bare_set.contains(s) && !is_external(s)) .collect() } else { Vec::new() @@ -771,10 +792,11 @@ pub(crate) fn run_build_with_options( prescan_new ); bare_specifiers.extend(prescan_new.iter().cloned()); - let extra = ngc_npm_resolver::resolve_npm_dependencies( + let extra = ngc_npm_resolver::resolve_npm_dependencies_with_externals( &prescan_new, &config_dir, export_conditions, + &external_specifiers, )?; tracing::info!( "pre-scan: pulled in {} additional file(s) before flatten", @@ -854,7 +876,7 @@ pub(crate) fn run_build_with_options( let post_link_specifiers = scan_transformed_bare_specifiers(&project_modules, &local_prefixes); let mut new_specifiers: Vec = Vec::new(); for spec in post_link_specifiers { - if !bare_specifiers.contains(&spec) { + if !bare_specifiers.contains(&spec) && !is_external(&spec) { new_specifiers.push(spec); } } @@ -865,10 +887,11 @@ pub(crate) fn run_build_with_options( new_specifiers ); bare_specifiers.extend(new_specifiers.iter().cloned()); - let extra = ngc_npm_resolver::resolve_npm_dependencies( + let extra = ngc_npm_resolver::resolve_npm_dependencies_with_externals( &new_specifiers, &config_dir, export_conditions, + &external_specifiers, )?; tracing::info!( "post-flatten npm resolution pulled in {} file(s)", @@ -1011,6 +1034,14 @@ pub(crate) fn run_build_with_options( drop(define_span); } + // Belt-and-braces: even though the resolver was told to skip externals, + // strip them from `bundled_specifiers` so the rewriter never sees an + // externalised name in its "local" set. This also handles the edge + // case where `inject_oxc_runtime_helpers` adds a specifier later — if + // somehow an external name showed up, this last filter keeps the + // bundle output correct. + bundled_specifiers.retain(|s| !external_specifiers.contains(s)); + let bundle_input = BundleInput { modules, graph, @@ -1020,6 +1051,7 @@ pub(crate) fn run_build_with_options( options: bundle_options, per_module_maps, bundled_specifiers, + external_specifiers, export_conditions: export_conditions.iter().map(|s| (*s).to_string()).collect(), }; drop(graph_span); diff --git a/crates/cli/src/polyfills.rs b/crates/cli/src/polyfills.rs index 2eab67d..7be5a42 100644 --- a/crates/cli/src/polyfills.rs +++ b/crates/cli/src/polyfills.rs @@ -264,6 +264,7 @@ pub fn generate_polyfills( options: polyfill_bundle_options, per_module_maps, bundled_specifiers, + external_specifiers: Default::default(), export_conditions: export_conditions.iter().map(|s| (*s).to_string()).collect(), }; diff --git a/crates/npm-resolver/src/lib.rs b/crates/npm-resolver/src/lib.rs index 7067d57..9eee6d1 100644 --- a/crates/npm-resolver/src/lib.rs +++ b/crates/npm-resolver/src/lib.rs @@ -43,6 +43,36 @@ pub fn resolve_npm_dependencies( specifiers: &[String], project_root: &Path, conditions: &[&str], +) -> NgcResult { + resolve_npm_dependencies_with_externals(specifiers, project_root, conditions, &HashSet::new()) +} + +/// Returns `true` when `specifier` matches one of the externalised package +/// names — either exactly (`jquery` matches `jquery`) or as a subpath +/// (`jquery/dist/slim` matches `jquery`). Mirrors esbuild's `--external` +/// matching used by `@angular/build:application`. +fn is_external_specifier(specifier: &str, externals: &HashSet) -> bool { + if externals.is_empty() { + return false; + } + if externals.contains(specifier) { + return true; + } + externals + .iter() + .any(|ext| specifier.starts_with(ext) && specifier[ext.len()..].starts_with('/')) +} + +/// Variant of [`resolve_npm_dependencies`] that skips any specifier whose +/// package name appears in `externals`. The BFS does not walk into those +/// packages, so their modules never enter the bundle — they stay as bare +/// runtime imports for the host (browser import map, CDN loader) to +/// resolve. Used to honour `angular.json`'s `externalDependencies`. +pub fn resolve_npm_dependencies_with_externals( + specifiers: &[String], + project_root: &Path, + conditions: &[&str], + externals: &HashSet, ) -> NgcResult { let node_modules = project_root.join("node_modules"); if !node_modules.is_dir() { @@ -80,6 +110,7 @@ pub fn resolve_npm_dependencies( // probes — fully independent per specifier. let initial_entries: Vec<(String, PathBuf)> = specifiers .par_iter() + .filter(|spec| !is_external_specifier(spec, externals)) .filter_map(|spec| { let outcome = if spec.starts_with('#') { resolve::resolve_subpath_import(spec, None, project_root, conditions) @@ -134,6 +165,12 @@ pub fn resolve_npm_dependencies( let mut resolved_imports: Vec = Vec::with_capacity(scanned.len()); for import in &scanned { + // Honour `externalDependencies`: an import targeting an + // externalised package never enters the BFS, so the + // package's modules never reach the bundler. + if is_external_specifier(&import.specifier, externals) { + continue; + } let kind = if import.is_dynamic { ImportKind::Dynamic } else { @@ -307,6 +344,69 @@ mod tests { assert_eq!(result.edges.len(), 2, "should have 2 dependency edges"); } + #[test] + fn test_externals_skip_top_level_and_transitive() { + // Issue #146: a package listed in `externalDependencies` must NOT + // enter the resolution — neither when requested directly nor when + // reached transitively from another package. + let dir = tempfile::tempdir().unwrap(); + setup_crawl_fixture(dir.path()); + + let mut externals = HashSet::new(); + externals.insert("beta".to_string()); + + let result = resolve_npm_dependencies_with_externals( + &["alpha".to_string(), "beta".to_string()], + dir.path(), + DEV, + &externals, + ) + .expect("should resolve"); + + // alpha + utils.mjs only — beta is external so its index.mjs must + // not appear in modules and `beta` must not show up in resolved. + assert_eq!(result.modules.len(), 2, "beta's modules must not be pulled in"); + assert!( + !result.resolved_specifiers.contains("beta"), + "external 'beta' must not appear in resolved_specifiers" + ); + assert!(result.resolved_specifiers.contains("alpha")); + } + + #[test] + fn test_externals_match_subpath() { + let dir = tempfile::tempdir().unwrap(); + // Set up a single package that imports a subpath of an external pkg. + let pkg_dir = dir.path().join("node_modules/consumer"); + fs::create_dir_all(&pkg_dir).unwrap(); + fs::write( + pkg_dir.join("package.json"), + r#"{ "module": "./index.mjs" }"#, + ) + .unwrap(); + fs::write( + pkg_dir.join("index.mjs"), + "import slim from 'jquery/dist/jquery.slim';\nexport default slim;\n", + ) + .unwrap(); + + let mut externals = HashSet::new(); + externals.insert("jquery".to_string()); + + let result = resolve_npm_dependencies_with_externals( + &["consumer".to_string()], + dir.path(), + DEV, + &externals, + ) + .expect("should resolve"); + + // Only consumer is pulled in; the subpath import of jquery is + // treated as external and never walked. + assert_eq!(result.modules.len(), 1); + assert!(!result.resolved_specifiers.contains("jquery/dist/jquery.slim")); + } + #[test] fn test_crawl_deduplication() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/project-resolver/src/angular_json.rs b/crates/project-resolver/src/angular_json.rs index fe0c7cf..3f84d8b 100644 --- a/crates/project-resolver/src/angular_json.rs +++ b/crates/project-resolver/src/angular_json.rs @@ -160,6 +160,11 @@ pub struct RawBuildOptions { /// CDN libraries, polyfill shims that don't fit through `polyfills.ts`). /// Each entry is a string path or `{ input, inject, bundleName }` object. pub scripts: Option>, + /// npm package names that should NOT be bundled — their `import` statements + /// stay as bare ESM specifiers for the runtime (browser import map, CDN + /// loader, etc.) to resolve. Matches `@angular/build:application`'s + /// `externalDependencies` option. + pub external_dependencies: Option>, } /// One entry in `architect.build.options.budgets` (or in a per-configuration @@ -376,6 +381,11 @@ pub struct RawBuildConfiguration { /// of the base `define` map: same-key entries replace the base value, /// keys that appear only in the base are preserved. pub define: Option>, + /// Override for `externalDependencies`. When present, replaces the + /// base list (matches `@angular/build:application`'s semantics — the + /// configuration value wholly substitutes for the base value rather + /// than appending). + pub external_dependencies: Option>, } // --------------------------------------------------------------------------- @@ -515,6 +525,13 @@ pub struct ResolvedAngularProject { /// entry that shares the same `bundleName`. Empty when no `scripts` /// are declared. pub scripts: Vec, + /// npm package names declared as `externalDependencies` in + /// `angular.json`. Imports matching one of these specifiers stay as + /// bare ESM specifiers in the emitted bundle — the package is not + /// inlined and the resolver does not BFS into its modules. Matching + /// is by exact name or `/...` prefix, mirroring how + /// `@angular/build:application` (esbuild) treats package externals. + pub external_dependencies: Vec, } /// Type of a resolved size budget. @@ -816,6 +833,13 @@ pub fn resolve_angular_project( .map(|raw_scripts| resolve_scripts(raw_scripts, &base_dir)) .unwrap_or_default(); + // `externalDependencies` resolution: per-configuration override wholly + // replaces the base list when present (matching ng build's behaviour). + let external_dependencies = build_config + .and_then(|bc| bc.external_dependencies.clone()) + .or_else(|| options.and_then(|o| o.external_dependencies.clone())) + .unwrap_or_default(); + debug!( project = %name, output_path = %output_path.display(), @@ -847,6 +871,7 @@ pub fn resolve_angular_project( budgets, define, scripts, + external_dependencies, }) } @@ -1811,6 +1836,81 @@ mod tests { assert!(result.define.is_empty()); } + #[test] + fn test_parse_external_dependencies() { + let json = r#"{ + "projects": { + "app": { + "architect": { + "build": { + "options": { + "outputPath": "dist", + "tsConfig": "tsconfig.json", + "externalDependencies": ["jquery", "@stripe/stripe-js"] + } + } + } + } + } + }"#; + let f = write_temp_json(json); + let result = resolve_angular_project(f.path(), None, None).unwrap(); + assert_eq!( + result.external_dependencies, + vec!["jquery".to_string(), "@stripe/stripe-js".to_string()] + ); + } + + #[test] + fn test_external_dependencies_default_to_empty() { + let json = r#"{ + "projects": { + "app": { + "architect": { + "build": { + "options": { "outputPath": "dist", "tsConfig": "tsconfig.json" } + } + } + } + } + }"#; + let f = write_temp_json(json); + let result = resolve_angular_project(f.path(), None, None).unwrap(); + assert!(result.external_dependencies.is_empty()); + } + + #[test] + fn test_external_dependencies_configuration_override_replaces_base() { + // Per-configuration `externalDependencies` wholly replaces the base + // list (matches @angular/build:application). + let json = r#"{ + "projects": { + "app": { + "architect": { + "build": { + "options": { + "outputPath": "dist", + "tsConfig": "tsconfig.json", + "externalDependencies": ["jquery"] + }, + "configurations": { + "production": { + "externalDependencies": ["@stripe/stripe-js"] + } + } + } + } + } + } + }"#; + let f = write_temp_json(json); + let result = resolve_angular_project(f.path(), None, Some("production")).unwrap(); + assert_eq!( + result.external_dependencies, + vec!["@stripe/stripe-js".to_string()] + ); + } + #[test] fn test_no_i18n_block_resolves_to_none() { let json = r#"{ diff --git a/packages/builder/src/build/options.ts b/packages/builder/src/build/options.ts index aec4326..b019cea 100644 --- a/packages/builder/src/build/options.ts +++ b/packages/builder/src/build/options.ts @@ -175,11 +175,6 @@ export function translateOptions( 'The `outputHashing` option is hardcoded by ngc-rs per `--configuration` (production hashes bundles, development does not); the option value is ignored.', ); } - if (raw.externalDependencies && raw.externalDependencies.length > 0) { - warnings.push( - '`externalDependencies` is currently ignored by ngc-rs; all imports are bundled.', - ); - } if (Array.isArray(raw.localize)) { warnings.push( 'Selecting a locale subset via `localize` array is not yet honoured by ngc-rs; all locales declared in `angular.json` `i18n.locales` are emitted.',