From 5de30c53239ac74843725d981afc0bb8c45bdf16 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 23 May 2024 12:31:05 -0700 Subject: [PATCH] fix(cli): Support deno.lock with only package.json present + fix DENO_FUTURE install interactions with lockfile (#23918) Fixes #23571. Previously, we required a `deno.json` to be present (or the `--lock` flag) in order for us to resolve a `deno.lock` file. This meant that if you were using deno in an npm-first project deno wouldn't use a lockfile. Additionally, while I was fixing that, I discovered there were a couple bugs keeping the future `install` command from using a lockfile. With this PR, `install` will actually resolve the lockfile (or create one if not present), and update it if it's not up-to-date. This also speeds up `deno install`, as we can use the lockfile to skip work during npm resolution. --- cli/args/lockfile.rs | 16 ++++- cli/args/mod.rs | 7 +- cli/tools/installer.rs | 4 ++ tests/integration/npm_tests.rs | 11 ++-- .../future_install_local_deno/__test__.jsonc | 8 +++ .../future_install_local_deno/deno.lock.out | 22 +++++++ .../__test__.jsonc | 65 +++++++++++++++---- .../future_install_node_modules/corrupt.js | 5 ++ .../future_install_node_modules/corrupted.out | 3 + .../future_install_node_modules/deno.lock.out | 19 ++++++ .../lockfile/only_package_json/__test__.jsonc | 16 +++++ .../lockfile/only_package_json/cache.out | 3 + .../lockfile/only_package_json/deno.lock.out | 19 ++++++ .../specs/lockfile/only_package_json/index.js | 1 + .../lockfile/only_package_json/package.json | 7 ++ 15 files changed, 184 insertions(+), 22 deletions(-) create mode 100644 tests/specs/install/future_install_local_deno/deno.lock.out create mode 100644 tests/specs/install/future_install_node_modules/corrupt.js create mode 100644 tests/specs/install/future_install_node_modules/corrupted.out create mode 100644 tests/specs/install/future_install_node_modules/deno.lock.out create mode 100644 tests/specs/lockfile/only_package_json/__test__.jsonc create mode 100644 tests/specs/lockfile/only_package_json/cache.out create mode 100644 tests/specs/lockfile/only_package_json/deno.lock.out create mode 100644 tests/specs/lockfile/only_package_json/index.js create mode 100644 tests/specs/lockfile/only_package_json/package.json diff --git a/cli/args/lockfile.rs b/cli/args/lockfile.rs index 84cad98d4f..d5ab14432d 100644 --- a/cli/args/lockfile.rs +++ b/cli/args/lockfile.rs @@ -3,11 +3,14 @@ use std::path::PathBuf; use deno_core::error::AnyError; +use deno_runtime::deno_node::PackageJson; use crate::args::ConfigFile; use crate::Flags; use super::DenoSubcommand; +use super::InstallFlags; +use super::InstallKind; pub use deno_lockfile::Lockfile; pub use deno_lockfile::LockfileError; @@ -15,11 +18,15 @@ pub use deno_lockfile::LockfileError; pub fn discover( flags: &Flags, maybe_config_file: Option<&ConfigFile>, + maybe_package_json: Option<&PackageJson>, ) -> Result, AnyError> { if flags.no_lock || matches!( flags.subcommand, - DenoSubcommand::Install(_) | DenoSubcommand::Uninstall(_) + DenoSubcommand::Install(InstallFlags { + kind: InstallKind::Global(..), + .. + }) | DenoSubcommand::Uninstall(_) ) { return Ok(None); @@ -38,7 +45,12 @@ pub fn discover( return Ok(None); } } - None => return Ok(None), + None => match maybe_package_json { + Some(package_json) => { + package_json.path.parent().unwrap().join("deno.lock") + } + None => return Ok(None), + }, }, }; diff --git a/cli/args/mod.rs b/cli/args/mod.rs index b3d508e18a..03a6357aa5 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -852,8 +852,11 @@ impl CliOptions { maybe_package_json = discover_package_json(&flags, None, &initial_cwd)?; } - let maybe_lock_file = - lockfile::discover(&flags, maybe_config_file.as_ref())?; + let maybe_lock_file = lockfile::discover( + &flags, + maybe_config_file.as_ref(), + maybe_package_json.as_ref(), + )?; Self::new( flags, initial_cwd, diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index b13dea6fd9..82a44de16a 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -265,6 +265,10 @@ async fn install_local( let factory = CliFactory::from_flags(flags)?; crate::module_loader::load_top_level_deps(&factory).await?; + if let Some(lockfile) = factory.cli_options().maybe_lockfile() { + lockfile.lock().write()?; + } + Ok(()) } diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index 4c4868c652..ab00c7b56f 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -2982,7 +2982,8 @@ fn cjs_export_analysis_import_cjs_directly_relative_import() { } itest!(imports_package_json { - args: "run --node-modules-dir=false npm/imports_package_json/main.js", + args: + "run --no-lock --node-modules-dir=false npm/imports_package_json/main.js", output: "npm/imports_package_json/main.out", envs: env_vars_for_npm_tests(), http_server: true, @@ -2990,7 +2991,7 @@ itest!(imports_package_json { itest!(imports_package_json_import_not_defined { args: - "run --node-modules-dir=false npm/imports_package_json/import_not_defined.js", + "run --no-lock --node-modules-dir=false npm/imports_package_json/import_not_defined.js", output: "npm/imports_package_json/import_not_defined.out", envs: env_vars_for_npm_tests(), exit_code: 1, @@ -2999,7 +3000,7 @@ itest!(imports_package_json_import_not_defined { itest!(imports_package_json_sub_path_import_not_defined { args: - "run --node-modules-dir=false npm/imports_package_json/sub_path_import_not_defined.js", + "run --no-lock --node-modules-dir=false npm/imports_package_json/sub_path_import_not_defined.js", output: "npm/imports_package_json/sub_path_import_not_defined.out", envs: env_vars_for_npm_tests(), exit_code: 1, @@ -3007,7 +3008,7 @@ itest!(imports_package_json_sub_path_import_not_defined { }); itest!(different_nested_dep_node_modules_dir_false { - args: "run --quiet --node-modules-dir=false npm/different_nested_dep/main.js", + args: "run --quiet --no-lock --node-modules-dir=false npm/different_nested_dep/main.js", output: "npm/different_nested_dep/main.out", envs: env_vars_for_npm_tests(), exit_code: 0, @@ -3015,7 +3016,7 @@ itest!(different_nested_dep_node_modules_dir_false { }); itest!(different_nested_dep_node_modules_dir_true { - args: "run --quiet --node-modules-dir=true main.js", + args: "run --no-lock --quiet --node-modules-dir=true main.js", output: "npm/different_nested_dep/main.out", copy_temp_dir: Some("npm/different_nested_dep/"), cwd: Some("npm/different_nested_dep/"), diff --git a/tests/specs/install/future_install_local_deno/__test__.jsonc b/tests/specs/install/future_install_local_deno/__test__.jsonc index eb780d9624..928030699c 100644 --- a/tests/specs/install/future_install_local_deno/__test__.jsonc +++ b/tests/specs/install/future_install_local_deno/__test__.jsonc @@ -12,6 +12,14 @@ // ensure deps are actually cached "args": "run --cached-only main.js", "output": "" + }, + { + // check for lockfile + "args": [ + "eval", + "console.log(Deno.readTextFileSync('./deno.lock').trim())" + ], + "output": "deno.lock.out" } ] } diff --git a/tests/specs/install/future_install_local_deno/deno.lock.out b/tests/specs/install/future_install_local_deno/deno.lock.out new file mode 100644 index 0000000000..fd3d52e428 --- /dev/null +++ b/tests/specs/install/future_install_local_deno/deno.lock.out @@ -0,0 +1,22 @@ +{ + "version": "3", + "packages": { + "specifiers": { + "jsr:@denotest/add": "jsr:@denotest/add@1.0.0", + "npm:@denotest/esm-basic@^1.0.0": "npm:@denotest/esm-basic@1.0.0" + }, + "jsr": { + "@denotest/add@1.0.0": [WILDCARD] + }, + "npm": { + "@denotest/esm-basic@1.0.0": [WILDCARD] + } + }, + "remote": [WILDCARD], + "workspace": { + "dependencies": [ + "jsr:@denotest/add", + "npm:@denotest/esm-basic@^1.0.0" + ] + } +} diff --git a/tests/specs/install/future_install_node_modules/__test__.jsonc b/tests/specs/install/future_install_node_modules/__test__.jsonc index eb780d9624..5712165570 100644 --- a/tests/specs/install/future_install_node_modules/__test__.jsonc +++ b/tests/specs/install/future_install_node_modules/__test__.jsonc @@ -1,17 +1,56 @@ { - "tempDir": true, - "envs": { - "DENO_FUTURE": "1" - }, - "steps": [ - { - "args": "install", - "output": "install.out" + "tests": { + "install_sets_up_node_modules": { + "tempDir": true, + "envs": { + "DENO_FUTURE": "1" + }, + "steps": [ + { + "args": "install", + "output": "install.out" + }, + { + // ensure deps are actually cached + "args": "run --cached-only main.js", + "output": "" + }, + { + // check for lockfile + "args": [ + "eval", + "console.log(Deno.readTextFileSync('./deno.lock').trim())" + ], + "output": "deno.lock.out" + } + ] }, - { - // ensure deps are actually cached - "args": "run --cached-only main.js", - "output": "" + "install_errors_corrupted_lockfile": { + "tempDir": true, + "envs": { + "DENO_FUTURE": "1" + }, + "steps": [ + { + "args": "install", + "output": "install.out" + }, + { + // Mess up the lockfile + "args": [ + "run", + "-A", + "corrupt.js" + ], + "output": "" + }, + { + // Run the install again + "args": "install", + "output": "corrupted.out", + "exitCode": 10 + } + ] } - ] + } } diff --git a/tests/specs/install/future_install_node_modules/corrupt.js b/tests/specs/install/future_install_node_modules/corrupt.js new file mode 100644 index 0000000000..fcc146081a --- /dev/null +++ b/tests/specs/install/future_install_node_modules/corrupt.js @@ -0,0 +1,5 @@ +const lock = JSON.parse(Deno.readTextFileSync("./deno.lock")); +const pkg = lock.packages.npm["@denotest/esm-basic@1.0.0"]; +// Corrupt the integrity hash +pkg.integrity = pkg.integrity.slice(0, -1); +Deno.writeTextFileSync("./deno.lock", JSON.stringify(lock)); diff --git a/tests/specs/install/future_install_node_modules/corrupted.out b/tests/specs/install/future_install_node_modules/corrupted.out new file mode 100644 index 0000000000..89578cbe26 --- /dev/null +++ b/tests/specs/install/future_install_node_modules/corrupted.out @@ -0,0 +1,3 @@ +[WILDCARD] +error: Integrity check failed for package: "npm:@denotest/esm-basic@1.0.0".[WILDCARD] +Use "--lock-write" flag to regenerate the lockfile at [WILDCARD] \ No newline at end of file diff --git a/tests/specs/install/future_install_node_modules/deno.lock.out b/tests/specs/install/future_install_node_modules/deno.lock.out new file mode 100644 index 0000000000..b302329967 --- /dev/null +++ b/tests/specs/install/future_install_node_modules/deno.lock.out @@ -0,0 +1,19 @@ +{ + "version": "3", + "packages": { + "specifiers": { + "npm:@denotest/esm-basic": "npm:@denotest/esm-basic@1.0.0" + }, + "npm": { + "@denotest/esm-basic@1.0.0": [WILDCARD] + } + }, + "remote": {}, + "workspace": { + "packageJson": { + "dependencies": [ + "npm:@denotest/esm-basic" + ] + } + } +} diff --git a/tests/specs/lockfile/only_package_json/__test__.jsonc b/tests/specs/lockfile/only_package_json/__test__.jsonc new file mode 100644 index 0000000000..6b28a7a924 --- /dev/null +++ b/tests/specs/lockfile/only_package_json/__test__.jsonc @@ -0,0 +1,16 @@ +{ + "tempDir": true, + "steps": [ + { + "args": "cache index.js", + "output": "cache.out" + }, + { + "args": [ + "eval", + "console.log(Deno.readTextFileSync('./deno.lock').trim())" + ], + "output": "deno.lock.out" + } + ] +} diff --git a/tests/specs/lockfile/only_package_json/cache.out b/tests/specs/lockfile/only_package_json/cache.out new file mode 100644 index 0000000000..b8114c12a0 --- /dev/null +++ b/tests/specs/lockfile/only_package_json/cache.out @@ -0,0 +1,3 @@ +Download http://localhost:4260/@denotest/esm-basic +Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz +Initialize @denotest/esm-basic@1.0.0 diff --git a/tests/specs/lockfile/only_package_json/deno.lock.out b/tests/specs/lockfile/only_package_json/deno.lock.out new file mode 100644 index 0000000000..b302329967 --- /dev/null +++ b/tests/specs/lockfile/only_package_json/deno.lock.out @@ -0,0 +1,19 @@ +{ + "version": "3", + "packages": { + "specifiers": { + "npm:@denotest/esm-basic": "npm:@denotest/esm-basic@1.0.0" + }, + "npm": { + "@denotest/esm-basic@1.0.0": [WILDCARD] + } + }, + "remote": {}, + "workspace": { + "packageJson": { + "dependencies": [ + "npm:@denotest/esm-basic" + ] + } + } +} diff --git a/tests/specs/lockfile/only_package_json/index.js b/tests/specs/lockfile/only_package_json/index.js new file mode 100644 index 0000000000..2600083e2f --- /dev/null +++ b/tests/specs/lockfile/only_package_json/index.js @@ -0,0 +1 @@ +import * as basic from "@denotest/esm-basic"; diff --git a/tests/specs/lockfile/only_package_json/package.json b/tests/specs/lockfile/only_package_json/package.json new file mode 100644 index 0000000000..6611f206fc --- /dev/null +++ b/tests/specs/lockfile/only_package_json/package.json @@ -0,0 +1,7 @@ +{ + "name": "lockfile_only_package_json", + "dependencies": { + "@denotest/esm-basic": "*" + }, + "type": "module" +}