From 3cdf0170982171edb18fe928543c5614888583cc Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Wed, 28 Aug 2024 18:59:34 -0700 Subject: [PATCH 01/12] Point to branch --- Cargo.lock | 3 +-- cli/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cbbe67da057c44..278f81afb1a291 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1376,8 +1376,7 @@ dependencies = [ [[package]] name = "deno_config" version = "0.30.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9657dbcc5210407fd9a1b1571310f2fe25c6dd6be2195c964d19f43d70045a95" +source = "git+https://github.com/nathanwhit/deno_config?branch=node-modules#4d4a2e0b1ec99ca61b50faf024fdea3e77f866c2" dependencies = [ "anyhow", "deno_package_json", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ed804d50bf3e71..697afdfaaf78e3 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.30.1", features = ["workspace", "sync"], git = "https://github.com/nathanwhit/deno_config", branch = "node-modules"} deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = { version = "0.147.0", features = ["html", "syntect"] } deno_emit = "=0.44.0" From 343a43d92213d548ed2aab58f2ecd08cdac686f9 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 11:48:06 -0700 Subject: [PATCH 02/12] Node modules config --- cli/args/flags.rs | 53 +++++++++++++++++++++++--------- cli/args/mod.rs | 68 ++++++++++++++++++++++++++++++----------- cli/graph_util.rs | 6 +++- cli/lsp/config.rs | 13 ++++++-- cli/tools/run/mod.rs | 7 ++++- cli/tools/vendor/mod.rs | 5 ++- 6 files changed, 115 insertions(+), 37 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index ca8a0a82fa6275..dbe8ea7dec24d6 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()) + .arg(node_modules_arg()) .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()) + .arg(node_modules_arg()) .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()) + .arg(node_modules_arg()) .arg(vendor_arg()) .arg(config_arg()) .arg(no_config_arg()) @@ -3916,16 +3918,39 @@ 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 s = matches.remove_one::("node-modules"); + if let Some(s) = s { + let mode = NodeModulesMode::parse(&s).unwrap(); + flags.node_modules_mode = Some(mode); + } + } else { + flags.node_modules_dir = matches.remove_one::("node-modules-dir"); + } +} + +fn node_modules_arg() -> Arg { + if *DENO_FUTURE { + Arg::new("node-modules") + .long("node-modules") + .num_args(0..=1) + .value_parser(|s: &str| NodeModulesMode::parse(s).map(|_| s.to_string())) + .value_name("MODE") + .require_equals(true) + .help("Sets the node modules management mode for npm packages") + .help_heading(DEPENDENCY_MANAGEMENT_HEADING) + } else { + 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 +5340,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 bd5309c01ecf4a..8a0170e8115368 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, @@ -1255,11 +1257,27 @@ 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) -> Option { + if *DENO_FUTURE { + self + .flags + .node_modules_mode + .or_else(|| self.workspace().node_modules_mode().ok().flatten()) + } else { + 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 +1629,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().is_none() && self.maybe_node_modules_folder.is_some() && self .workspace() @@ -1624,13 +1652,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 +1784,25 @@ 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 { + flags + .node_modules_mode + .or_else(|| workspace.node_modules_mode().ok().flatten()) + .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 c8432b67ee9200..0080e0f6d7eaf5 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -19,6 +19,7 @@ use crate::tools::check; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path; +use deno_config::deno_json::NodeModulesMode; use deno_config::workspace::JsrPackageConfig; use deno_emit::LoaderChecksum; use deno_graph::JsrLoadError; @@ -541,7 +542,10 @@ 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 matches!( + self.options.node_modules_mode(), + Some(NodeModulesMode::LocalAuto | NodeModulesMode::LocalManual) + ) { 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 ba5cc3ac4402a6..ec2690506d1ab7 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 1964cfdd9ab6cb..644158e7345fca 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 e1445237247a0a..5329bfc4919cdd 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.",), From f8e30f70d051297c05d9587b6258e2e559169a74 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 11:48:19 -0700 Subject: [PATCH 03/12] Update tests --- tests/integration/npm_tests.rs | 46 ++++++++++++++++++++++++------- tests/integration/vendor_tests.rs | 33 ++++++++++++++++++++-- 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index f4bf3d6e9dd59e..ae63282855f768 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -11,6 +11,7 @@ use url::Url; use util::assert_contains; use util::env_vars_for_npm_tests; use util::http_server; +use util::PathRef; use util::TestContextBuilder; // NOTE: See how to make test npm packages at ./testdata/npm/README.md @@ -2024,10 +2025,17 @@ fn top_level_install_package_json_explicit_opt_in() { temp_dir.write("main.ts", "console.log(5);"); let output = test_context.new_command().args("cache main.ts").run(); - output.assert_matches_text(concat!( - "Download http://localhost:4260/@denotest/esm-basic\n", - "Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz\n", - "Initialize @denotest/esm-basic@1.0.0\n", + output.assert_matches_text(format!( + concat!( + "{}", + "Download http://localhost:4260/@denotest/esm-basic\n", + "Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz\n", + "Initialize @denotest/esm-basic@1.0.0\n", + ), + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ) )); rm_created_files(); @@ -2035,9 +2043,12 @@ fn top_level_install_package_json_explicit_opt_in() { .new_command() .args_vec(["eval", "console.log(5)"]) .run(); - output.assert_matches_text(concat!( - "Initialize @denotest/esm-basic@1.0.0\n", - "5\n" + output.assert_matches_text(format!( + concat!("{}", "Initialize @denotest/esm-basic@1.0.0\n", "5\n"), + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ) )); rm_created_files(); @@ -2046,9 +2057,12 @@ fn top_level_install_package_json_explicit_opt_in() { .args("run -") .stdin_text("console.log(5)") .run(); - output.assert_matches_text(concat!( - "Initialize @denotest/esm-basic@1.0.0\n", - "5\n" + output.assert_matches_text(format!( + concat!("{}", "Initialize @denotest/esm-basic@1.0.0\n", "5\n"), + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ) )); // now ensure this is cached in the lsp @@ -2895,3 +2909,15 @@ async fn test_private_npm_registry() { let resp = client.execute(req).await.unwrap(); assert_eq!(resp.status(), reqwest::StatusCode::OK); } + +fn node_modules_dir_deprecated_text(suggest: &str, path: PathRef) -> String { + format!( + concat!( + "Warning \"nodeModulesDir\" field is deprecated in deno 2.0. Use ", + "`\"nodeModules\": \"{}\"` instead\n", + " at {}\n", + ), + suggest, + path.canonicalize().url_file(), + ) +} diff --git a/tests/integration/vendor_tests.rs b/tests/integration/vendor_tests.rs index 55ffe8734fe3d8..8502d6a84997ee 100644 --- a/tests/integration/vendor_tests.rs +++ b/tests/integration/vendor_tests.rs @@ -7,6 +7,7 @@ use std::fmt::Write as _; use std::path::PathBuf; use test_util as util; use test_util::itest; +use test_util::PathRef; use test_util::TempDir; use util::http_server; use util::new_deno_dir; @@ -601,15 +602,25 @@ fn vendor_npm_node_specifiers() { success_text_updated_deno_json("vendor/") )); let output = context.new_command().args("run -A my_app.ts").run(); - output.assert_matches_text("true 5\n"); + output.assert_matches_text(format!( + "{}true 5\n", + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ) + )); assert!(temp_dir.path().join("node_modules").exists()); assert!(temp_dir.path().join("deno.lock").exists()); // now try re-vendoring with a lockfile let output = context.new_command().args("vendor --force my_app.ts").run(); output.assert_matches_text(format!( - "{}{}\n{}\n\n{}\n", + "{}{}{}\n{}\n\n{}\n", &DEPRECATION_NOTICE, + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ), ignoring_import_map_text(), vendored_text("1 module", "vendor/"), success_text_updated_deno_json("vendor/"), @@ -624,8 +635,12 @@ fn vendor_npm_node_specifiers() { .args("vendor --node-modules-dir=false --force my_app.ts") .run(); output.assert_matches_text(format!( - "{}{}\n{}\n\n{}\n", + "{}{}{}\n{}\n\n{}\n", &DEPRECATION_NOTICE, + node_modules_dir_deprecated_text( + "local-auto", + temp_dir.path().join("deno.json") + ), ignoring_import_map_text(), vendored_text("1 module", "vendor/"), success_text_updated_deno_json("vendor/") @@ -747,3 +762,15 @@ fn ignoring_import_map_text() -> String { PathBuf::from("vendor").join("import_map.json").display(), ) } + +fn node_modules_dir_deprecated_text(suggest: &str, path: PathRef) -> String { + format!( + concat!( + "Warning \"nodeModulesDir\" field is deprecated in deno 2.0. Use ", + "`\"nodeModules\": \"{}\"` instead\n", + " at {}\n", + ), + suggest, + path.canonicalize().url_file(), + ) +} From d730ca494abfa075cee344c6c9f89c04d33b87b4 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 11:53:46 -0700 Subject: [PATCH 04/12] Fmt --- cli/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 697afdfaaf78e3..d198a7549df89e 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"], git = "https://github.com/nathanwhit/deno_config", branch = "node-modules"} +deno_config = { version = "=0.30.1", features = ["workspace", "sync"], git = "https://github.com/nathanwhit/deno_config", branch = "node-modules" } deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = { version = "0.147.0", features = ["html", "syntect"] } deno_emit = "=0.44.0" From 6f5f4b8ecc578a575a30e016b128edba35b9e08e Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 13:12:48 -0700 Subject: [PATCH 05/12] Surface error --- cli/args/mod.rs | 55 ++++++++++++++++++++++------------------- cli/graph_util.rs | 11 +++++---- cli/tools/run/mod.rs | 2 +- cli/tools/vendor/mod.rs | 2 +- 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 8a0170e8115368..e22ad725118b2c 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1257,26 +1257,28 @@ impl CliOptions { } } - pub fn node_modules_mode(&self) -> Option { + pub fn node_modules_mode(&self) -> Result, AnyError> { if *DENO_FUTURE { - self - .flags - .node_modules_mode - .or_else(|| self.workspace().node_modules_mode().ok().flatten()) + if let Some(flag) = self.flags.node_modules_mode { + return Ok(Some(flag)); + } + self.workspace().node_modules_mode().map_err(Into::into) } else { - 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 - } - }) + 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 + } + }), + ) } } @@ -1641,7 +1643,7 @@ impl CliOptions { pub fn use_byonm(&self) -> bool { if self.enable_future_features() - && self.node_modules_mode().is_none() + && self.node_modules_mode().ok().flatten().is_none() && self.maybe_node_modules_folder.is_some() && self .workspace() @@ -1790,12 +1792,15 @@ fn resolve_node_modules_folder( deno_dir_provider: &Arc, ) -> Result, AnyError> { let use_node_modules_dir = if *DENO_FUTURE { - flags - .node_modules_mode - .or_else(|| workspace.node_modules_mode().ok().flatten()) - .map(|m| m.uses_node_modules_dir()) - .or(flags.vendor) - .or_else(|| maybe_config_file.and_then(|c| c.json.vendor)) + 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 diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 0080e0f6d7eaf5..8ff6c9ae3f5ddd 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -19,7 +19,6 @@ use crate::tools::check; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path; -use deno_config::deno_json::NodeModulesMode; use deno_config::workspace::JsrPackageConfig; use deno_emit::LoaderChecksum; use deno_graph::JsrLoadError; @@ -542,10 +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 matches!( - self.options.node_modules_mode(), - Some(NodeModulesMode::LocalAuto | NodeModulesMode::LocalManual) - ) { + 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/tools/run/mod.rs b/cli/tools/run/mod.rs index 644158e7345fca..9d1d5e78bb996d 100644 --- a/cli/tools/run/mod.rs +++ b/cli/tools/run/mod.rs @@ -196,7 +196,7 @@ pub async fn maybe_npm_install(factory: &CliFactory) -> Result<(), AnyError> { // opted into using a managed node_modules directory if factory .cli_options()? - .node_modules_mode() + .node_modules_mode()? .map(|m| m.uses_node_modules_dir()) .unwrap_or(false) { diff --git a/cli/tools/vendor/mod.rs b/cli/tools/vendor/mod.rs index 5329bfc4919cdd..d21d1752974b15 100644 --- a/cli/tools/vendor/mod.rs +++ b/cli/tools/vendor/mod.rs @@ -89,7 +89,7 @@ pub async fn vendor( let npm_package_count = graph.npm_packages.len(); let try_add_node_modules_dir = npm_package_count > 0 && cli_options - .node_modules_mode() + .node_modules_mode()? .map(|m| m.uses_node_modules_dir()) .unwrap_or(true); From f0d7d2e48cd526481f50b3988006f8f02f5bee49 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 13:58:08 -0700 Subject: [PATCH 06/12] Suggest new arg --- cli/args/flags.rs | 55 +++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index dbe8ea7dec24d6..757f5c51b8c7c8 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -2365,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_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg( Arg::new("json") @@ -3180,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_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg(reload_arg()) .arg(ca_file_arg()) @@ -3242,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_arg()) + .args(node_modules_args()) .arg(vendor_arg()) .arg(config_arg()) .arg(no_config_arg()) @@ -3930,26 +3930,39 @@ fn node_modules_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { } } -fn node_modules_arg() -> Arg { +fn node_modules_args() -> Vec { if *DENO_FUTURE { - Arg::new("node-modules") - .long("node-modules") - .num_args(0..=1) - .value_parser(|s: &str| NodeModulesMode::parse(s).map(|_| s.to_string())) - .value_name("MODE") - .require_equals(true) - .help("Sets the node modules management mode for npm packages") - .help_heading(DEPENDENCY_MANAGEMENT_HEADING) + vec![ + Arg::new("node-modules") + .long("node-modules") + .num_args(0..=1) + .value_parser(|s: &str| { + NodeModulesMode::parse(s).map(|_| s.to_string()) + }) + .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 { - 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) + 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) + ] } } From 1abc018b34ee1bceb90060bbb1f683e16e2cb534 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 14:16:02 -0700 Subject: [PATCH 07/12] Only warn with deno_future --- cli/args/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index e22ad725118b2c..a0888ca581d1c1 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -919,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); } From 2fa689b1b2bc73b0b98a0954200e9d0d21b9a758 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 14:16:52 -0700 Subject: [PATCH 08/12] Revert "Update tests" This reverts commit f8e30f70d051297c05d9587b6258e2e559169a74. --- tests/integration/npm_tests.rs | 46 +++++++------------------------ tests/integration/vendor_tests.rs | 33 ++-------------------- 2 files changed, 13 insertions(+), 66 deletions(-) diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index ae63282855f768..f4bf3d6e9dd59e 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -11,7 +11,6 @@ use url::Url; use util::assert_contains; use util::env_vars_for_npm_tests; use util::http_server; -use util::PathRef; use util::TestContextBuilder; // NOTE: See how to make test npm packages at ./testdata/npm/README.md @@ -2025,17 +2024,10 @@ fn top_level_install_package_json_explicit_opt_in() { temp_dir.write("main.ts", "console.log(5);"); let output = test_context.new_command().args("cache main.ts").run(); - output.assert_matches_text(format!( - concat!( - "{}", - "Download http://localhost:4260/@denotest/esm-basic\n", - "Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz\n", - "Initialize @denotest/esm-basic@1.0.0\n", - ), - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ) + output.assert_matches_text(concat!( + "Download http://localhost:4260/@denotest/esm-basic\n", + "Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz\n", + "Initialize @denotest/esm-basic@1.0.0\n", )); rm_created_files(); @@ -2043,12 +2035,9 @@ fn top_level_install_package_json_explicit_opt_in() { .new_command() .args_vec(["eval", "console.log(5)"]) .run(); - output.assert_matches_text(format!( - concat!("{}", "Initialize @denotest/esm-basic@1.0.0\n", "5\n"), - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ) + output.assert_matches_text(concat!( + "Initialize @denotest/esm-basic@1.0.0\n", + "5\n" )); rm_created_files(); @@ -2057,12 +2046,9 @@ fn top_level_install_package_json_explicit_opt_in() { .args("run -") .stdin_text("console.log(5)") .run(); - output.assert_matches_text(format!( - concat!("{}", "Initialize @denotest/esm-basic@1.0.0\n", "5\n"), - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ) + output.assert_matches_text(concat!( + "Initialize @denotest/esm-basic@1.0.0\n", + "5\n" )); // now ensure this is cached in the lsp @@ -2909,15 +2895,3 @@ async fn test_private_npm_registry() { let resp = client.execute(req).await.unwrap(); assert_eq!(resp.status(), reqwest::StatusCode::OK); } - -fn node_modules_dir_deprecated_text(suggest: &str, path: PathRef) -> String { - format!( - concat!( - "Warning \"nodeModulesDir\" field is deprecated in deno 2.0. Use ", - "`\"nodeModules\": \"{}\"` instead\n", - " at {}\n", - ), - suggest, - path.canonicalize().url_file(), - ) -} diff --git a/tests/integration/vendor_tests.rs b/tests/integration/vendor_tests.rs index 8502d6a84997ee..55ffe8734fe3d8 100644 --- a/tests/integration/vendor_tests.rs +++ b/tests/integration/vendor_tests.rs @@ -7,7 +7,6 @@ use std::fmt::Write as _; use std::path::PathBuf; use test_util as util; use test_util::itest; -use test_util::PathRef; use test_util::TempDir; use util::http_server; use util::new_deno_dir; @@ -602,25 +601,15 @@ fn vendor_npm_node_specifiers() { success_text_updated_deno_json("vendor/") )); let output = context.new_command().args("run -A my_app.ts").run(); - output.assert_matches_text(format!( - "{}true 5\n", - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ) - )); + output.assert_matches_text("true 5\n"); assert!(temp_dir.path().join("node_modules").exists()); assert!(temp_dir.path().join("deno.lock").exists()); // now try re-vendoring with a lockfile let output = context.new_command().args("vendor --force my_app.ts").run(); output.assert_matches_text(format!( - "{}{}{}\n{}\n\n{}\n", + "{}{}\n{}\n\n{}\n", &DEPRECATION_NOTICE, - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ), ignoring_import_map_text(), vendored_text("1 module", "vendor/"), success_text_updated_deno_json("vendor/"), @@ -635,12 +624,8 @@ fn vendor_npm_node_specifiers() { .args("vendor --node-modules-dir=false --force my_app.ts") .run(); output.assert_matches_text(format!( - "{}{}{}\n{}\n\n{}\n", + "{}{}\n{}\n\n{}\n", &DEPRECATION_NOTICE, - node_modules_dir_deprecated_text( - "local-auto", - temp_dir.path().join("deno.json") - ), ignoring_import_map_text(), vendored_text("1 module", "vendor/"), success_text_updated_deno_json("vendor/") @@ -762,15 +747,3 @@ fn ignoring_import_map_text() -> String { PathBuf::from("vendor").join("import_map.json").display(), ) } - -fn node_modules_dir_deprecated_text(suggest: &str, path: PathRef) -> String { - format!( - concat!( - "Warning \"nodeModulesDir\" field is deprecated in deno 2.0. Use ", - "`\"nodeModules\": \"{}\"` instead\n", - " at {}\n", - ), - suggest, - path.canonicalize().url_file(), - ) -} From 07d713d8427793ea7eed32c64482358fa0236d8e Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 14:32:32 -0700 Subject: [PATCH 09/12] Not needed --- ext/node/polyfills/child_process.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/node/polyfills/child_process.ts b/ext/node/polyfills/child_process.ts index bb38b746c13082..f77a430c2cb352 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, From b1c38176b949f968dfa5f814b68a469c2720d65d Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 14:34:19 -0700 Subject: [PATCH 10/12] Use published version --- Cargo.lock | 5 +++-- cli/Cargo.toml | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 278f81afb1a291..15a859174dcaf4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1375,8 +1375,9 @@ dependencies = [ [[package]] name = "deno_config" -version = "0.30.1" -source = "git+https://github.com/nathanwhit/deno_config?branch=node-modules#4d4a2e0b1ec99ca61b50faf024fdea3e77f866c2" +version = "0.31.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "efba70c2fbec59e6d0c6040376644803f2cebbb4d55a651cbab4794e390a8592" dependencies = [ "anyhow", "deno_package_json", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index d198a7549df89e..8059d59e529d9b 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"], git = "https://github.com/nathanwhit/deno_config", branch = "node-modules" } +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" From e04a8e6bfd15f35c7da3e63356fe59af86cf707b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 14:34:52 -0700 Subject: [PATCH 11/12] Update tests --- .../lockfile/frozen_lockfile/__test__.jsonc | 22 +++++++++---------- .../npm/bin_entries_prefer_closer/deno.json | 2 +- .../__test__.jsonc | 4 ++-- .../npm/lifecycle_scripts/__test__.jsonc | 7 ++++++ 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/specs/lockfile/frozen_lockfile/__test__.jsonc b/tests/specs/lockfile/frozen_lockfile/__test__.jsonc index 3036c8db52e693..0ac69c6bded2cf 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 176354f98fadaf..b2edaa0354d2d7 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 b7c3b90d775ab8..bc39e85da60e11 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 302b40d1e3fb17..c62bc47b49942e 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", From 20201691d2ac63564f1279476432a065b0c0ba7b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 29 Aug 2024 15:00:06 -0700 Subject: [PATCH 12/12] Better flag parse --- cli/args/flags.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 757f5c51b8c7c8..3ac28086941eb1 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -3920,9 +3920,8 @@ fn no_npm_arg() -> Arg { fn node_modules_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { if *DENO_FUTURE { - let s = matches.remove_one::("node-modules"); - if let Some(s) = s { - let mode = NodeModulesMode::parse(&s).unwrap(); + let value = matches.remove_one::("node-modules"); + if let Some(mode) = value { flags.node_modules_mode = Some(mode); } } else { @@ -3936,9 +3935,7 @@ fn node_modules_args() -> Vec { Arg::new("node-modules") .long("node-modules") .num_args(0..=1) - .value_parser(|s: &str| { - NodeModulesMode::parse(s).map(|_| s.to_string()) - }) + .value_parser(NodeModulesMode::parse) .value_name("MODE") .require_equals(true) .help("Sets the node modules management mode for npm packages")