From cacc8c9a4debe6a7d32f743ab9b2f636ae51dec1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 23 May 2023 18:51:48 -0400 Subject: [PATCH] feat: top level package.json install when node_modules dir is explicitly opted into (#19233) When someone explicitly opts into using the node_modules dir via `--node-modules-dir` or setting `"nodeModulesDir": true` in the deno.json file, we should eagerly ensure a top level package.json install is done on startup. This was initially always done when we added package.json support and a package.json was auto-discovered, but scaled it back to be lazily done when a bare specifier matched an entry in the package.json because of how disruptive it was for people using Deno scripts in Node projects. That said, it does not make sense for someone to opt-into having deno control and use their node_modules directory and not want a package.json install to occur. If such a rare scenario exists, the `DENO_NO_PACKAGE_JSON=1` environment variable can be set. Ideally, we would only ever use a node_modules directory with this explicit opt-in so everything is very clear, but we still have this automatic scenario when there's a package.json in order to make more node projects work out of the box. --- cli/graph_util.rs | 6 +++ cli/resolver.rs | 14 ++++-- cli/tests/integration/npm_tests.rs | 71 ++++++++++++++++++++++++++++++ cli/tools/run.rs | 37 ++++++++++++---- 4 files changed, 116 insertions(+), 12 deletions(-) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 53d06071c8..90c4f8b388 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -296,6 +296,12 @@ impl ModuleGraphBuilder { loader: &mut dyn deno_graph::source::Loader, options: deno_graph::BuildOptions<'a>, ) -> 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) { + self.resolver.force_top_level_package_json_install().await?; + } + graph.build(roots, loader, options).await; // ensure that the top level package.json is installed if a diff --git a/cli/resolver.rs b/cli/resolver.rs index 7f49116f46..6fa8eaabef 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -182,14 +182,20 @@ impl CliGraphResolver { self } + pub async fn force_top_level_package_json_install( + &self, + ) -> Result<(), AnyError> { + self + .package_json_deps_installer + .ensure_top_level_install() + .await + } + pub async fn top_level_package_json_install_if_necessary( &self, ) -> Result<(), AnyError> { if self.found_package_json_dep_flag.is_raised() { - self - .package_json_deps_installer - .ensure_top_level_install() - .await?; + self.force_top_level_package_json_install().await?; } Ok(()) } diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index 73ac029df3..bd19ed26f9 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -1,6 +1,7 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use deno_core::serde_json; +use deno_core::serde_json::json; use deno_core::serde_json::Value; use pretty_assertions::assert_eq; use std::process::Stdio; @@ -1924,3 +1925,73 @@ pub fn node_modules_dir_config_file() { .run(); assert!(node_modules_dir.exists()); } + +#[test] +fn top_level_install_package_json_explicit_opt_in() { + let test_context = TestContextBuilder::for_npm().use_temp_cwd().build(); + let temp_dir = test_context.temp_dir(); + let node_modules_dir = temp_dir.path().join("node_modules"); + let rm_created_files = || { + std::fs::remove_dir_all(&node_modules_dir).unwrap(); + std::fs::remove_file(temp_dir.path().join("deno.lock")).unwrap(); + }; + + // when the node_modules_dir is explicitly opted into, we should always + // ensure a top level package.json install occurs + temp_dir.write("deno.json", "{ \"nodeModulesDir\": true }"); + temp_dir.write( + "package.json", + "{ \"dependencies\": { \"@denotest/esm-basic\": \"1.0\" }}", + ); + + 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:4545/npm/registry/@denotest/esm-basic\n", + "Download http://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz\n", + "Initialize @denotest/esm-basic@1.0.0\n", + ) + ); + + rm_created_files(); + let output = test_context + .new_command() + .args_vec(["eval", "console.log(5)"]) + .run(); + output.assert_matches_text(concat!( + "Initialize @denotest/esm-basic@1.0.0\n", + "5\n" + )); + + rm_created_files(); + let output = test_context + .new_command() + .args("run -") + .stdin("console.log(5)") + .run(); + output.assert_matches_text(concat!( + "Initialize @denotest/esm-basic@1.0.0\n", + "5\n" + )); + + // now ensure this is cached in the lsp + rm_created_files(); + let mut client = test_context.new_lsp_command().build(); + client.initialize_default(); + let file_uri = temp_dir.uri().join("file.ts").unwrap(); + client.did_open(json!({ + "textDocument": { + "uri": file_uri, + "languageId": "typescript", + "version": 1, + "text": "", + } + })); + client.write_request( + "deno/cache", + json!({ "referrer": { "uri": file_uri }, "uris": [] }), + ); + + assert!(node_modules_dir.join("@denotest").exists()); +} diff --git a/cli/tools/run.rs b/cli/tools/run.rs index 99312d5b90..4805ea704d 100644 --- a/cli/tools/run.rs +++ b/cli/tools/run.rs @@ -47,6 +47,8 @@ To grant permissions, set them before the script argument. For example: let main_module = cli_options.resolve_main_module()?; + maybe_npm_install(&factory).await?; + let permissions = PermissionsContainer::new(Permissions::from_options( &cli_options.permissions_options(), )?); @@ -63,9 +65,11 @@ pub async fn run_from_stdin(flags: Flags) -> Result { let factory = CliFactory::from_flags(flags).await?; let cli_options = factory.cli_options(); let main_module = cli_options.resolve_main_module()?; + + maybe_npm_install(&factory).await?; + let file_fetcher = factory.file_fetcher()?; let worker_factory = factory.create_cli_main_worker_factory().await?; - let permissions = PermissionsContainer::new(Permissions::from_options( &cli_options.permissions_options(), )?); @@ -103,9 +107,11 @@ async fn run_with_watch(flags: Flags) -> Result { let cli_options = factory.cli_options(); let clear_screen = !cli_options.no_clear_screen(); let main_module = cli_options.resolve_main_module()?; + + maybe_npm_install(&factory).await?; + let create_cli_main_worker_factory = factory.create_cli_main_worker_factory_func().await?; - let operation = |main_module: ModuleSpecifier| { file_watcher.reset(); let permissions = PermissionsContainer::new(Permissions::from_options( @@ -144,12 +150,10 @@ pub async fn eval_command( let factory = CliFactory::from_flags(flags).await?; let cli_options = factory.cli_options(); let file_fetcher = factory.file_fetcher()?; - let main_worker_factory = factory.create_cli_main_worker_factory().await?; - let main_module = cli_options.resolve_main_module()?; - let permissions = PermissionsContainer::new(Permissions::from_options( - &cli_options.permissions_options(), - )?); + + maybe_npm_install(&factory).await?; + // Create a dummy source file. let source_code = if eval_flags.print { format!("console.log({})", eval_flags.code) @@ -171,9 +175,26 @@ pub async fn eval_command( // to allow module access by TS compiler. file_fetcher.insert_cached(file); - let mut worker = main_worker_factory + let permissions = PermissionsContainer::new(Permissions::from_options( + &cli_options.permissions_options(), + )?); + let worker_factory = factory.create_cli_main_worker_factory().await?; + let mut worker = worker_factory .create_main_worker(main_module, permissions) .await?; let exit_code = worker.run().await?; Ok(exit_code) } + +async fn maybe_npm_install(factory: &CliFactory) -> Result<(), AnyError> { + // ensure an "npm install" is done if the user has explicitly + // opted into using a node_modules directory + if factory.cli_options().node_modules_dir_enablement() == Some(true) { + factory + .package_json_deps_installer() + .await? + .ensure_top_level_install() + .await?; + } + Ok(()) +}