From 86d5b919d8ef55070e95230c9268f69cf58c25c8 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 29 Aug 2024 15:57:43 -0700 Subject: [PATCH] feat(config): Node modules option for 2.0 (#25299) --- Cargo.lock | 4 +- cli/Cargo.toml | 2 +- cli/args/flags.rs | 63 +++++++++++---- cli/args/mod.rs | 77 ++++++++++++++----- cli/graph_util.rs | 7 +- cli/lsp/config.rs | 13 +++- cli/tools/run/mod.rs | 7 +- cli/tools/vendor/mod.rs | 5 +- ext/node/polyfills/child_process.ts | 1 - .../lockfile/frozen_lockfile/__test__.jsonc | 22 +++--- .../npm/bin_entries_prefer_closer/deno.json | 2 +- .../__test__.jsonc | 4 +- .../npm/lifecycle_scripts/__test__.jsonc | 7 ++ 13 files changed, 158 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c3b16cea95..ad685a9f0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1375,9 +1375,9 @@ dependencies = [ [[package]] name = "deno_config" -version = "0.30.1" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9657dbcc5210407fd9a1b1571310f2fe25c6dd6be2195c964d19f43d70045a95" +checksum = "efba70c2fbec59e6d0c6040376644803f2cebbb4d55a651cbab4794e390a8592" dependencies = [ "anyhow", "deno_package_json", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index c7462ba005..6617cc90b4 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -65,7 +65,7 @@ winres.workspace = true [dependencies] deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] } deno_cache_dir = { workspace = true } -deno_config = { version = "=0.30.1", features = ["workspace", "sync"] } +deno_config = { version = "=0.31.0", features = ["workspace", "sync"] } deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = { version = "0.147.0", features = ["html", "syntect"] } deno_emit = "=0.44.0" diff --git a/cli/args/flags.rs b/cli/args/flags.rs index ca8a0a82fa..3ac2808694 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -10,6 +10,7 @@ use clap::ColorChoice; use clap::Command; use clap::ValueHint; use color_print::cstr; +use deno_config::deno_json::NodeModulesMode; use deno_config::glob::FilePatterns; use deno_config::glob::PathOrPatternSet; use deno_core::anyhow::bail; @@ -602,6 +603,7 @@ pub struct Flags { pub type_check_mode: TypeCheckMode, pub config_flag: ConfigFlag, pub node_modules_dir: Option, + pub node_modules_mode: Option, pub vendor: Option, pub enable_op_summary_metrics: bool, pub enable_testing_features: bool, @@ -2363,7 +2365,7 @@ TypeScript compiler cache: Subdirectory containing TS compiler output.", .arg(no_lock_arg()) .arg(config_arg()) .arg(import_map_arg()) - .arg(node_modules_dir_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg( Arg::new("json") @@ -3178,7 +3180,7 @@ Remote modules and multiple modules may also be specified: .arg(config_arg()) .arg(import_map_arg()) .arg(lock_arg()) - .arg(node_modules_dir_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg(reload_arg()) .arg(ca_file_arg()) @@ -3240,7 +3242,7 @@ fn compile_args_without_check_args(app: Command) -> Command { .arg(import_map_arg()) .arg(no_remote_arg()) .arg(no_npm_arg()) - .arg(node_modules_dir_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg(config_arg()) .arg(no_config_arg()) @@ -3916,16 +3918,49 @@ fn no_npm_arg() -> Arg { .help_heading(DEPENDENCY_MANAGEMENT_HEADING) } -fn node_modules_dir_arg() -> Arg { - Arg::new("node-modules-dir") - .long("node-modules-dir") - .num_args(0..=1) - .value_parser(value_parser!(bool)) - .value_name("DIRECTORY") - .default_missing_value("true") - .require_equals(true) - .help("Enables or disables the use of a local node_modules folder for npm packages") - .help_heading(DEPENDENCY_MANAGEMENT_HEADING) +fn node_modules_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { + if *DENO_FUTURE { + let value = matches.remove_one::("node-modules"); + if let Some(mode) = value { + flags.node_modules_mode = Some(mode); + } + } else { + flags.node_modules_dir = matches.remove_one::("node-modules-dir"); + } +} + +fn node_modules_args() -> Vec { + if *DENO_FUTURE { + vec![ + Arg::new("node-modules") + .long("node-modules") + .num_args(0..=1) + .value_parser(NodeModulesMode::parse) + .value_name("MODE") + .require_equals(true) + .help("Sets the node modules management mode for npm packages") + .help_heading(DEPENDENCY_MANAGEMENT_HEADING), + Arg::new("node-modules-dir") + .long("node-modules-dir") + .num_args(0..=1) + .value_parser(clap::builder::UnknownArgumentValueParser::suggest_arg( + "--node-modules", + )) + .require_equals(true), + ] + } else { + vec![ + Arg::new("node-modules-dir") + .long("node-modules-dir") + .num_args(0..=1) + .value_parser(value_parser!(bool)) + .value_name("ENABLED") + .default_missing_value("true") + .require_equals(true) + .help("Enables or disables the use of a local node_modules folder for npm packages") + .help_heading(DEPENDENCY_MANAGEMENT_HEADING) + ] + } } fn vendor_arg() -> Arg { @@ -5315,7 +5350,7 @@ fn node_modules_and_vendor_dir_arg_parse( flags: &mut Flags, matches: &mut ArgMatches, ) { - flags.node_modules_dir = matches.remove_one::("node-modules-dir"); + node_modules_arg_parse(flags, matches); flags.vendor = matches.remove_one::("vendor"); } diff --git a/cli/args/mod.rs b/cli/args/mod.rs index bd5309c01e..a0888ca581 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -8,6 +8,7 @@ mod lockfile; mod package_json; use deno_ast::SourceMapOption; +use deno_config::deno_json::NodeModulesMode; use deno_config::workspace::CreateResolverOptions; use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::VendorEnablement; @@ -819,6 +820,7 @@ impl CliOptions { let maybe_node_modules_folder = resolve_node_modules_folder( &initial_cwd, &flags, + &start_dir.workspace, root_folder.deno_json.as_deref(), root_folder.pkg_json.as_deref(), &deno_dir_provider, @@ -917,6 +919,10 @@ impl CliOptions { }; for diagnostic in start_dir.workspace.diagnostics() { + // TODO(2.0): remove + if matches!(diagnostic.kind, deno_config::workspace::WorkspaceDiagnosticKind::DeprecatedNodeModulesDirOption(_)) && !*DENO_FUTURE { + continue; + } log::warn!("{} {}", colors::yellow("Warning"), diagnostic); } @@ -1255,11 +1261,29 @@ impl CliOptions { } } - pub fn node_modules_dir_enablement(&self) -> Option { - self - .flags - .node_modules_dir - .or_else(|| self.workspace().node_modules_dir()) + pub fn node_modules_mode(&self) -> Result, AnyError> { + if *DENO_FUTURE { + if let Some(flag) = self.flags.node_modules_mode { + return Ok(Some(flag)); + } + self.workspace().node_modules_mode().map_err(Into::into) + } else { + Ok( + self + .flags + .node_modules_dir + .or_else(|| self.workspace().node_modules_dir()) + .map(|enabled| { + if enabled && self.byonm_enabled() { + NodeModulesMode::LocalManual + } else if enabled { + NodeModulesMode::LocalAuto + } else { + NodeModulesMode::GlobalAuto + } + }), + ) + } } pub fn vendor_dir_path(&self) -> Option<&PathBuf> { @@ -1611,9 +1635,19 @@ impl CliOptions { || self.workspace().has_unstable("bare-node-builtins") } + fn byonm_enabled(&self) -> bool { + // check if enabled via unstable + self.flags.unstable_config.byonm + || NPM_PROCESS_STATE + .as_ref() + .map(|s| matches!(s.kind, NpmProcessStateKind::Byonm)) + .unwrap_or(false) + || self.workspace().has_unstable("byonm") + } + pub fn use_byonm(&self) -> bool { if self.enable_future_features() - && self.node_modules_dir_enablement().is_none() + && self.node_modules_mode().ok().flatten().is_none() && self.maybe_node_modules_folder.is_some() && self .workspace() @@ -1624,13 +1658,7 @@ impl CliOptions { return true; } - // check if enabled via unstable - self.flags.unstable_config.byonm - || NPM_PROCESS_STATE - .as_ref() - .map(|s| matches!(s.kind, NpmProcessStateKind::Byonm)) - .unwrap_or(false) - || self.workspace().has_unstable("byonm") + self.byonm_enabled() } pub fn unstable_sloppy_imports(&self) -> bool { @@ -1762,15 +1790,28 @@ impl CliOptions { fn resolve_node_modules_folder( cwd: &Path, flags: &Flags, + workspace: &Workspace, maybe_config_file: Option<&ConfigFile>, maybe_package_json: Option<&PackageJson>, deno_dir_provider: &Arc, ) -> Result, AnyError> { - let use_node_modules_dir = flags - .node_modules_dir - .or_else(|| maybe_config_file.and_then(|c| c.json.node_modules_dir)) - .or(flags.vendor) - .or_else(|| maybe_config_file.and_then(|c| c.json.vendor)); + let use_node_modules_dir = if *DENO_FUTURE { + if let Some(mode) = flags.node_modules_mode { + Some(mode.uses_node_modules_dir()) + } else { + workspace + .node_modules_mode()? + .map(|m| m.uses_node_modules_dir()) + .or(flags.vendor) + .or_else(|| maybe_config_file.and_then(|c| c.json.vendor)) + } + } else { + flags + .node_modules_dir + .or_else(|| maybe_config_file.and_then(|c| c.json.node_modules_dir)) + .or(flags.vendor) + .or_else(|| maybe_config_file.and_then(|c| c.json.vendor)) + }; let path = if use_node_modules_dir == Some(false) { return Ok(None); } else if let Some(state) = &*NPM_PROCESS_STATE { diff --git a/cli/graph_util.rs b/cli/graph_util.rs index c8432b67ee..8ff6c9ae3f 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -541,7 +541,12 @@ impl ModuleGraphBuilder { ) -> Result<(), AnyError> { // ensure an "npm install" is done if the user has explicitly // opted into using a node_modules directory - if self.options.node_modules_dir_enablement() == Some(true) { + if self + .options + .node_modules_mode()? + .map(|m| m.uses_node_modules_dir()) + .unwrap_or(false) + { if let Some(npm_resolver) = self.npm_resolver.as_managed() { npm_resolver.ensure_top_level_package_json_install().await?; } diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index ba5cc3ac44..ec2690506d 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -5,6 +5,7 @@ use deno_config::deno_json::DenoJsonCache; use deno_config::deno_json::FmtConfig; use deno_config::deno_json::FmtOptionsConfig; use deno_config::deno_json::LintConfig; +use deno_config::deno_json::NodeModulesMode; use deno_config::deno_json::TestConfig; use deno_config::deno_json::TsConfig; use deno_config::fs::DenoConfigFs; @@ -1390,8 +1391,16 @@ impl ConfigData { let byonm = std::env::var("DENO_UNSTABLE_BYONM").is_ok() || member_dir.workspace.has_unstable("byonm") || (*DENO_FUTURE - && member_dir.workspace.package_jsons().next().is_some() - && member_dir.workspace.node_modules_dir().is_none()); + && matches!( + member_dir.workspace.node_modules_mode().unwrap_or_default(), + Some(NodeModulesMode::LocalManual) + )) + || ( + *DENO_FUTURE + && member_dir.workspace.package_jsons().next().is_some() + && member_dir.workspace.node_modules_dir().is_none() + // TODO(2.0): remove + ); if byonm { lsp_log!(" Enabled 'bring your own node_modules'."); } diff --git a/cli/tools/run/mod.rs b/cli/tools/run/mod.rs index 1964cfdd9a..9d1d5e78bb 100644 --- a/cli/tools/run/mod.rs +++ b/cli/tools/run/mod.rs @@ -194,7 +194,12 @@ pub async fn eval_command( pub async fn maybe_npm_install(factory: &CliFactory) -> Result<(), AnyError> { // ensure an "npm install" is done if the user has explicitly // opted into using a managed node_modules directory - if factory.cli_options()?.node_modules_dir_enablement() == Some(true) { + if factory + .cli_options()? + .node_modules_mode()? + .map(|m| m.uses_node_modules_dir()) + .unwrap_or(false) + { if let Some(npm_resolver) = factory.npm_resolver().await?.as_managed() { npm_resolver.ensure_top_level_package_json_install().await?; } diff --git a/cli/tools/vendor/mod.rs b/cli/tools/vendor/mod.rs index e144523724..d21d175297 100644 --- a/cli/tools/vendor/mod.rs +++ b/cli/tools/vendor/mod.rs @@ -88,7 +88,10 @@ pub async fn vendor( let graph = output.graph; let npm_package_count = graph.npm_packages.len(); let try_add_node_modules_dir = npm_package_count > 0 - && cli_options.node_modules_dir_enablement().unwrap_or(true); + && cli_options + .node_modules_mode()? + .map(|m| m.uses_node_modules_dir()) + .unwrap_or(true); log::info!( concat!("Vendored {} {} into {} directory.",), diff --git a/ext/node/polyfills/child_process.ts b/ext/node/polyfills/child_process.ts index bb38b746c1..f77a430c2c 100644 --- a/ext/node/polyfills/child_process.ts +++ b/ext/node/polyfills/child_process.ts @@ -145,7 +145,6 @@ export function fork( args = [ "run", ...op_bootstrap_unstable_args(), - "--node-modules-dir", "-A", ...stringifiedV8Flags, ...execArgv, diff --git a/tests/specs/lockfile/frozen_lockfile/__test__.jsonc b/tests/specs/lockfile/frozen_lockfile/__test__.jsonc index 3036c8db52..0ac69c6bde 100644 --- a/tests/specs/lockfile/frozen_lockfile/__test__.jsonc +++ b/tests/specs/lockfile/frozen_lockfile/__test__.jsonc @@ -58,18 +58,22 @@ ] }, "error_when_package_json_changed": { + "envs": { + "DENO_FUTURE": "1" + }, "steps": [ { - "envs": { - "DENO_FUTURE": "1" - }, + "args": [ + "eval", + "Deno.writeTextFileSync('deno.json', `{ \"nodeModules\": \"local-auto\" }`)" + ], + "output": "[WILDCARD]" + }, + { "args": "cache add.ts", "output": "[WILDCARD]" }, { - "envs": { - "DENO_FUTURE": "1" - }, "args": [ "eval", "Deno.writeTextFileSync(\"package.json\", JSON.stringify({ dependencies: { \"@denotest/bin\": \"0.7.0\" } }))" @@ -77,17 +81,11 @@ "output": "" }, { - "envs": { - "DENO_FUTURE": "1" - }, "args": "cache --frozen add.ts", "output": "frozen_package_json_changed.out", "exitCode": 1 }, { - "envs": { - "DENO_FUTURE": "1" - }, "args": "install --frozen", "output": "frozen_package_json_changed_install.out", "exitCode": 1 diff --git a/tests/specs/npm/bin_entries_prefer_closer/deno.json b/tests/specs/npm/bin_entries_prefer_closer/deno.json index 176354f98f..b2edaa0354 100644 --- a/tests/specs/npm/bin_entries_prefer_closer/deno.json +++ b/tests/specs/npm/bin_entries_prefer_closer/deno.json @@ -1,3 +1,3 @@ { - "nodeModulesDir": true + "nodeModules": "local-auto" } diff --git a/tests/specs/npm/future_node_modules_dir_setting/__test__.jsonc b/tests/specs/npm/future_node_modules_dir_setting/__test__.jsonc index b7c3b90d77..bc39e85da6 100644 --- a/tests/specs/npm/future_node_modules_dir_setting/__test__.jsonc +++ b/tests/specs/npm/future_node_modules_dir_setting/__test__.jsonc @@ -10,11 +10,11 @@ "exitCode": 1 }, { // this should override byonm - "args": "run --node-modules-dir=false --quiet main.ts", + "args": "run --node-modules=global-auto --quiet main.ts", "output": "main.out" }, { // same with this - "args": "run --node-modules-dir --quiet main.ts", + "args": "run --node-modules=local-auto --quiet main.ts", "output": "main.out" }] } diff --git a/tests/specs/npm/lifecycle_scripts/__test__.jsonc b/tests/specs/npm/lifecycle_scripts/__test__.jsonc index 302b40d1e3..c62bc47b49 100644 --- a/tests/specs/npm/lifecycle_scripts/__test__.jsonc +++ b/tests/specs/npm/lifecycle_scripts/__test__.jsonc @@ -78,6 +78,13 @@ "DENO_FUTURE": "1" }, "steps": [ + { + "args": [ + "eval", + "Deno.removeSync('./deno.json')" + ], + "output": "[WILDCARD]" + }, { "args": [ "eval",