feat(vendor): support for npm specifiers (#19186)

We never properly added support for this. This fixes vendoring when it
has npm or node specifiers. Vendoring occurs by adding a
`"nodeModulesDir": true` property to deno.json then it uses a local
node_modules directory. This can be opted out by setting
`"nodeModulesDir": false` or running with `--node-modules-dir=false`.

Closes #18090
Closes #17210
Closes #17619
Closes #16778

cli/args/flags.rs

@@ -1343,7 +1343,7 @@ TypeScript compiler cache: Subdirectory containing TS compiler output.",
     .arg(lock_arg())
     .arg(config_arg())
     .arg(import_map_arg())
-    .arg(local_npm_arg())
+    .arg(node_modules_dir_arg())
     .arg(
       Arg::new("json")
         .long("json")
@@ -1862,6 +1862,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(reload_arg())
     .arg(ca_file_arg())
 }
@@ -1875,7 +1876,7 @@ fn compile_args_without_check_args(app: Command) -> Command {
     .arg(import_map_arg())
     .arg(no_remote_arg())
     .arg(no_npm_arg())
-    .arg(local_npm_arg())
+    .arg(node_modules_dir_arg())
     .arg(config_arg())
     .arg(no_config_arg())
     .arg(reload_arg())
@@ -2424,7 +2425,7 @@ fn no_npm_arg() -> Arg {
     .help("Do not resolve npm modules")
 }
 
-fn local_npm_arg() -> Arg {
+fn node_modules_dir_arg() -> Arg {
   Arg::new("node-modules-dir")
     .long("node-modules-dir")
     .num_args(0..=1)
@@ -2719,7 +2720,7 @@ fn info_parse(flags: &mut Flags, matches: &mut ArgMatches) {
   import_map_arg_parse(flags, matches);
   location_arg_parse(flags, matches);
   ca_file_arg_parse(flags, matches);
-  local_npm_args_parse(flags, matches);
+  node_modules_dir_arg_parse(flags, matches);
   lock_arg_parse(flags, matches);
   no_lock_arg_parse(flags, matches);
   no_remote_arg_parse(flags, matches);
@@ -2975,6 +2976,7 @@ fn vendor_parse(flags: &mut Flags, matches: &mut ArgMatches) {
   config_args_parse(flags, matches);
   import_map_arg_parse(flags, matches);
   lock_arg_parse(flags, matches);
+  node_modules_dir_arg_parse(flags, matches);
   reload_arg_parse(flags, matches);
 
   flags.subcommand = DenoSubcommand::Vendor(VendorFlags {
@@ -3000,7 +3002,7 @@ fn compile_args_without_check_parse(
   import_map_arg_parse(flags, matches);
   no_remote_arg_parse(flags, matches);
   no_npm_arg_parse(flags, matches);
-  local_npm_args_parse(flags, matches);
+  node_modules_dir_arg_parse(flags, matches);
   config_args_parse(flags, matches);
   reload_arg_parse(flags, matches);
   lock_args_parse(flags, matches);
@@ -3254,7 +3256,7 @@ fn no_npm_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) {
   }
 }
 
-fn local_npm_args_parse(flags: &mut Flags, matches: &mut ArgMatches) {
+fn node_modules_dir_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) {
   flags.node_modules_dir = matches.remove_one::<bool>("node-modules-dir");
 }
 

cli/args/mod.rs

@@ -881,6 +881,15 @@ impl CliOptions {
     self.maybe_node_modules_folder.clone()
   }
 
+  pub fn node_modules_dir_enablement(&self) -> Option<bool> {
+    self.flags.node_modules_dir.or_else(|| {
+      self
+        .maybe_config_file
+        .as_ref()
+        .and_then(|c| c.node_modules_dir())
+    })
+  }
+
   pub fn node_modules_dir_specifier(&self) -> Option<ModuleSpecifier> {
     self
       .maybe_node_modules_folder

cli/factory.rs

@@ -29,6 +29,7 @@ use crate::npm::create_npm_fs_resolver;
 use crate::npm::CliNpmRegistryApi;
 use crate::npm::CliNpmResolver;
 use crate::npm::NpmCache;
+use crate::npm::NpmPackageFsResolver;
 use crate::npm::NpmResolution;
 use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
@@ -325,6 +326,23 @@ impl CliFactory {
       .await
   }
 
+  pub async fn create_node_modules_npm_fs_resolver(
+    &self,
+    node_modules_dir_path: PathBuf,
+  ) -> Result<Arc<dyn NpmPackageFsResolver>, AnyError> {
+    Ok(create_npm_fs_resolver(
+      self.fs().clone(),
+      self.npm_cache()?.clone(),
+      self.text_only_progress_bar(),
+      CliNpmRegistryApi::default_url().to_owned(),
+      self.npm_resolution().await?.clone(),
+      // when an explicit path is provided here, it will create the
+      // local node_modules variant of an npm fs resolver
+      Some(node_modules_dir_path),
+      self.options.npm_system_info(),
+    ))
+  }
+
   pub fn package_json_deps_provider(&self) -> &Arc<PackageJsonDepsProvider> {
     self.services.package_json_deps_provider.get_or_init(|| {
       Arc::new(PackageJsonDepsProvider::new(

cli/npm/mod.rs

@@ -14,4 +14,5 @@ pub use registry::CliNpmRegistryApi;
 pub use resolution::NpmResolution;
 pub use resolvers::create_npm_fs_resolver;
 pub use resolvers::CliNpmResolver;
+pub use resolvers::NpmPackageFsResolver;
 pub use resolvers::NpmProcessState;

cli/npm/resolvers/mod.rs

@@ -36,11 +36,12 @@ use crate::args::Lockfile;
 use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs;
 use crate::util::progress_bar::ProgressBar;
 
-use self::common::NpmPackageFsResolver;
 use self::local::LocalNpmPackageResolver;
 use super::resolution::NpmResolution;
 use super::NpmCache;
 
+pub use self::common::NpmPackageFsResolver;
+
 /// State provided to the process via an environment variable.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct NpmProcessState {

cli/tests/integration/vendor_tests.rs

@@ -10,6 +10,7 @@ use test_util as util;
 use test_util::TempDir;
 use util::http_server;
 use util::new_deno_dir;
+use util::TestContextBuilder;
 
 #[test]
 fn output_dir_exists() {
@@ -186,15 +187,13 @@ fn import_map_output_dir() {
     String::from_utf8_lossy(&output.stderr).trim(),
     format!(
       concat!(
-        "Ignoring import map. Specifying an import map file ({}) in the deno ",
+        "{}\n",
-        "vendor output directory is not supported. If you wish to use an ",
-        "import map while vendoring, please specify one located outside this ",
-        "directory.\n",
         "Download http://localhost:4545/vendor/logger.ts\n",
-        "{}",
+        "{}\n\n{}",
       ),
-      PathBuf::from("vendor").join("import_map.json").display(),
+      ignoring_import_map_text(),
-      success_text_updated_deno_json("1 module", "vendor/"),
+      vendored_text("1 module", "vendor/"),
+      success_text_updated_deno_json("vendor/"),
     )
   );
   assert!(output.status.success());
@@ -511,8 +510,9 @@ fn update_existing_config_test() {
   assert_eq!(
     String::from_utf8_lossy(&output.stderr).trim(),
     format!(
-      "Download http://localhost:4545/vendor/logger.ts\n{}",
+      "Download http://localhost:4545/vendor/logger.ts\n{}\n\n{}",
-      success_text_updated_deno_json("1 module", "vendor2",)
+      vendored_text("1 module", "vendor2"),
+      success_text_updated_deno_json("vendor2",)
     )
   );
   assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), "");
@@ -537,12 +537,124 @@ fn update_existing_config_test() {
   assert!(output.status.success());
 }
 
+#[test]
+fn vendor_npm_node_specifiers() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write(
+    "my_app.ts",
+    concat!(
+      "import { path, getValue, setValue } from 'http://localhost:4545/vendor/npm_and_node_specifier.ts';\n",
+      "setValue(5);\n",
+      "console.log(path.isAbsolute(Deno.cwd()), getValue());",
+    ),
+  );
+  temp_dir.write("deno.json", "{}");
+
+  let output = context.new_command().args("vendor my_app.ts").run();
+  output.assert_matches_text(
+    format!(
+      concat!(
+        "Download http://localhost:4545/vendor/npm_and_node_specifier.ts\n",
+        "Download http://localhost:4545/npm/registry/@denotest/esm-basic\n",
+        "Download http://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz\n",
+        "{}\n",
+        "Initialize @denotest/esm-basic@1.0.0\n",
+        "{}\n\n",
+        "{}\n",
+      ),
+      vendored_text("1 module", "vendor/"),
+      vendored_npm_package_text("1 npm package"),
+      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");
+  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",
+    ignoring_import_map_text(),
+    vendored_text("1 module", "vendor/"),
+    success_text_updated_deno_json("vendor/"),
+  ));
+
+  // delete the node_modules folder
+  temp_dir.remove_dir_all("node_modules");
+
+  // vendor with --node-modules-dir=false
+  let output = context
+    .new_command()
+    .args("vendor --node-modules-dir=false --force my_app.ts")
+    .run();
+  output.assert_matches_text(format!(
+    "{}\n{}\n\n{}\n",
+    ignoring_import_map_text(),
+    vendored_text("1 module", "vendor/"),
+    success_text_updated_deno_json("vendor/")
+  ));
+  assert!(!temp_dir.path().join("node_modules").exists());
+
+  // delete the deno.json
+  temp_dir.remove_file("deno.json");
+
+  // vendor with --node-modules-dir
+  let output = context
+    .new_command()
+    .args("vendor --node-modules-dir --force my_app.ts")
+    .run();
+  output.assert_matches_text(format!(
+    "Initialize @denotest/esm-basic@1.0.0\n{}\n\n{}\n",
+    vendored_text("1 module", "vendor/"),
+    use_import_map_text("vendor/")
+  ));
+}
+
+#[test]
+fn vendor_only_npm_specifiers() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write(
+    "my_app.ts",
+    concat!(
+      "import { getValue, setValue } from 'npm:@denotest/esm-basic';\n",
+      "setValue(5);\n",
+      "console.log(path.isAbsolute(Deno.cwd()), getValue());",
+    ),
+  );
+  temp_dir.write("deno.json", "{}");
+
+  let output = context.new_command().args("vendor my_app.ts").run();
+  output.assert_matches_text(
+    format!(
+      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",
+        "{}\n",
+        "Initialize @denotest/esm-basic@1.0.0\n",
+        "{}\n",
+      ),
+      vendored_text("0 modules", "vendor/"),
+      vendored_npm_package_text("1 npm package"),
+    )
+  );
+}
+
 fn success_text(module_count: &str, dir: &str, has_import_map: bool) -> String {
   let mut text = format!("Vendored {module_count} into {dir} directory.");
   if has_import_map {
-    let f = format!(
+    write!(text, "\n\n{}", use_import_map_text(dir)).unwrap();
+  }
+  text
+}
+
+fn use_import_map_text(dir: &str) -> String {
+  format!(
     concat!(
-        "\n\nTo use vendored modules, specify the `--import-map {}import_map.json` flag when ",
+      "To use vendored modules, specify the `--import-map {}import_map.json` flag when ",
       r#"invoking Deno subcommands or add an `"importMap": "<path_to_vendored_import_map>"` "#,
       "entry to a deno.json file.",
     ),
@@ -551,24 +663,32 @@ fn success_text(module_count: &str, dir: &str, has_import_map: bool) -> String {
     } else {
       dir.to_string()
     }
-    );
+  )
-    write!(text, "{f}").unwrap();
-  }
-  text
 }
 
-fn success_text_updated_deno_json(module_count: &str, dir: &str) -> String {
+fn vendored_text(module_count: &str, dir: &str) -> String {
+  format!("Vendored {} into {} directory.", module_count, dir)
+}
+
+fn vendored_npm_package_text(package_count: &str) -> String {
+  format!(
+    concat!(
+     "Vendored {} into node_modules directory. Set `nodeModulesDir: false` ",
+     "in the Deno configuration file to disable vendoring npm packages in the future.",
+    ),
+    package_count
+  )
+}
+
+fn success_text_updated_deno_json(dir: &str) -> String {
   format!(
     concat!(
-      "Vendored {} into {} directory.\n\n",
       "Updated your local Deno configuration file with a reference to the ",
       "new vendored import map at {}import_map.json. Invoking Deno subcommands will ",
       "now automatically resolve using the vendored modules. You may override ",
       "this by providing the `--import-map <other-import-map>` flag or by ",
       "manually editing your Deno configuration file.",
     ),
-    module_count,
-    dir,
     if dir != "vendor/" {
       format!(
         "{}{}",
@@ -580,3 +700,15 @@ fn success_text_updated_deno_json(module_count: &str, dir: &str) -> String {
     }
   )
 }
+
+fn ignoring_import_map_text() -> String {
+  format!(
+    concat!(
+      "Ignoring import map. Specifying an import map file ({}) in the deno ",
+      "vendor output directory is not supported. If you wish to use an ",
+      "import map while vendoring, please specify one located outside this ",
+      "directory.",
+    ),
+    PathBuf::from("vendor").join("import_map.json").display(),
+  )
+}

cli/tests/testdata/vendor/npm_and_node_specifier.ts

@@ -0,0 +1,2 @@
+export { default as path } from "node:path";
+export { getValue, setValue } from "npm:@denotest/esm-basic";

cli/tools/vendor/import_map.rs

@@ -304,7 +304,7 @@ fn handle_dep_specifier(
       referrer,
       mappings,
     )
-  } else {
+  } else if specifier.scheme() == "file" {
     handle_local_dep_specifier(
       text,
       unresolved_specifier,

cli/tools/vendor/mod.rs

@@ -5,6 +5,7 @@ use std::path::PathBuf;
 use std::sync::Arc;
 
 use deno_ast::ModuleSpecifier;
+use deno_ast::TextChange;
 use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
@@ -12,6 +13,7 @@ use deno_core::resolve_url_or_path;
 use log::warn;
 
 use crate::args::CliOptions;
+use crate::args::ConfigFile;
 use crate::args::Flags;
 use crate::args::FmtOptionsConfig;
 use crate::args::VendorFlags;
@@ -51,6 +53,9 @@ pub async fn vendor(
     cli_options.initial_cwd(),
   )
   .await?;
+  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);
   let vendored_count = build::build(
     graph,
     factory.parsed_source_cache()?,
@@ -70,9 +75,48 @@ pub async fn vendor(
     },
     raw_output_dir.display(),
   );
+
+  let try_add_import_map = vendored_count > 0;
+  let modified_result = maybe_update_config_file(
+    &output_dir,
+    cli_options,
+    try_add_import_map,
+    try_add_node_modules_dir,
+  );
+
+  // cache the node_modules folder when it's been added to the config file
+  if modified_result.added_node_modules_dir {
+    let node_modules_path = cli_options.node_modules_dir_path().or_else(|| {
+      cli_options
+        .maybe_config_file_specifier()
+        .filter(|c| c.scheme() == "file")
+        .and_then(|c| c.to_file_path().ok())
+        .map(|config_path| config_path.parent().unwrap().join("node_modules"))
+    });
+    if let Some(node_modules_path) = node_modules_path {
+      factory
+        .create_node_modules_npm_fs_resolver(node_modules_path)
+        .await?
+        .cache_packages()
+        .await?;
+    }
+    log::info!(
+      concat!(
+        "Vendored {} npm {} into node_modules directory. Set `nodeModulesDir: false` ",
+        "in the Deno configuration file to disable vendoring npm packages in the future.",
+      ),
+      npm_package_count,
+      if npm_package_count == 1 {
+        "package"
+      } else {
+        "packages"
+      },
+    );
+  }
+
   if vendored_count > 0 {
     let import_map_path = raw_output_dir.join("import_map.json");
-    if maybe_update_config_file(&output_dir, cli_options) {
+    if modified_result.updated_import_map {
       log::info!(
         concat!(
           "\nUpdated your local Deno configuration file with a reference to the ",
@@ -154,107 +198,156 @@ fn validate_options(
   Ok(())
 }
 
-fn maybe_update_config_file(output_dir: &Path, options: &CliOptions) -> bool {
+fn maybe_update_config_file(
+  output_dir: &Path,
+  options: &CliOptions,
+  try_add_import_map: bool,
+  try_add_node_modules_dir: bool,
+) -> ModifiedResult {
   assert!(output_dir.is_absolute());
-  let config_file_specifier = match options.maybe_config_file_specifier() {
+  let config_file = match options.maybe_config_file() {
-    Some(f) => f,
+    Some(config_file) => config_file,
-    None => return false,
+    None => return ModifiedResult::default(),
   };
+  if config_file.specifier.scheme() != "file" {
+    return ModifiedResult::default();
+  }
 
-  let fmt_config = options
+  let fmt_config = config_file
-    .maybe_config_file()
+    .to_fmt_config()
-    .as_ref()
+    .ok()
-    .and_then(|config| config.to_fmt_config().ok())
     .unwrap_or_default()
     .unwrap_or_default();
   let result = update_config_file(
-    &config_file_specifier,
+    config_file,
-    &ModuleSpecifier::from_file_path(output_dir.join("import_map.json"))
-      .unwrap(),
     &fmt_config.options,
+    if try_add_import_map {
+      Some(
+        ModuleSpecifier::from_file_path(output_dir.join("import_map.json"))
+          .unwrap(),
+      )
+    } else {
+      None
+    },
+    try_add_node_modules_dir,
   );
   match result {
-    Ok(()) => true,
+    Ok(modified_result) => modified_result,
     Err(err) => {
       warn!("Error updating config file. {:#}", err);
-      false
+      ModifiedResult::default()
     }
   }
 }
 
 fn update_config_file(
-  config_specifier: &ModuleSpecifier,
+  config_file: &ConfigFile,
-  import_map_specifier: &ModuleSpecifier,
   fmt_options: &FmtOptionsConfig,
-) -> Result<(), AnyError> {
+  import_map_specifier: Option<ModuleSpecifier>,
-  if config_specifier.scheme() != "file" {
+  try_add_node_modules_dir: bool,
-    return Ok(());
+) -> Result<ModifiedResult, AnyError> {
-  }
+  let config_path = specifier_to_file_path(&config_file.specifier)?;
-
-  let config_path = specifier_to_file_path(config_specifier)?;
   let config_text = std::fs::read_to_string(&config_path)?;
-  let relative_text =
+  let import_map_specifier =
-    match relative_specifier(config_specifier, import_map_specifier) {
+    import_map_specifier.and_then(|import_map_specifier| {
-      Some(text) => text,
+      relative_specifier(&config_file.specifier, &import_map_specifier)
-      None => return Ok(()), // ignore
+    });
-    };
+  let modified_result = update_config_text(
-  if let Some(new_text) =
+    &config_text,
-    update_config_text(&config_text, &relative_text, fmt_options)
+    fmt_options,
-  {
+    import_map_specifier.as_deref(),
+    try_add_node_modules_dir,
+  )?;
+  if let Some(new_text) = &modified_result.new_text {
     std::fs::write(config_path, new_text)?;
   }
+  Ok(modified_result)
+}
 
-  Ok(())
+#[derive(Default)]
+struct ModifiedResult {
+  updated_import_map: bool,
+  added_node_modules_dir: bool,
+  new_text: Option<String>,
 }
 
 fn update_config_text(
   text: &str,
-  import_map_specifier: &str,
   fmt_options: &FmtOptionsConfig,
-) -> Option<String> {
+  import_map_specifier: Option<&str>,
+  try_add_node_modules_dir: bool,
+) -> Result<ModifiedResult, AnyError> {
   use jsonc_parser::ast::ObjectProp;
   use jsonc_parser::ast::Value;
   let ast =
-    jsonc_parser::parse_to_ast(text, &Default::default(), &Default::default())
+    jsonc_parser::parse_to_ast(text, &Default::default(), &Default::default())?;
-      .ok()?;
   let obj = match ast.value {
     Some(Value::Object(obj)) => obj,
-    _ => return None, // shouldn't happen, so ignore
+    _ => bail!("Failed updating config file due to no object."),
   };
-  let import_map_specifier = import_map_specifier.replace('\"', "\\\"");
+  let mut modified_result = ModifiedResult::default();
+  let mut text_changes = Vec::new();
+  let mut should_format = false;
 
+  if try_add_node_modules_dir {
+    // Only modify the nodeModulesDir property if it's not set
+    // as this allows people to opt-out of this when vendoring
+    // by specifying `nodeModulesDir: false`
+    if obj.get("nodeModulesDir").is_none() {
+      let insert_position = obj.range.end - 1;
+      text_changes.push(TextChange {
+        range: insert_position..insert_position,
+        new_text: r#""nodeModulesDir": true"#.to_string(),
+      });
+      should_format = true;
+      modified_result.added_node_modules_dir = true;
+    }
+  }
+
+  if let Some(import_map_specifier) = import_map_specifier {
+    let import_map_specifier = import_map_specifier.replace('\"', "\\\"");
     match obj.get("importMap") {
       Some(ObjectProp {
         value: Value::StringLit(lit),
         ..
-    }) => Some(format!(
+      }) => {
-      "{}{}{}",
+        text_changes.push(TextChange {
-      &text[..lit.range.start + 1],
+          range: lit.range.start..lit.range.end,
-      import_map_specifier,
+          new_text: format!("\"{}\"", import_map_specifier),
-      &text[lit.range.end - 1..],
+        });
-    )),
+        modified_result.updated_import_map = true;
+      }
       None => {
         // insert it crudely at a position that won't cause any issues
         // with comments and format after to make it look nice
         let insert_position = obj.range.end - 1;
-      let insert_text = format!(
+        text_changes.push(TextChange {
-        r#"{}"importMap": "{}""#,
+          range: insert_position..insert_position,
-        if obj.properties.is_empty() { "" } else { "," },
+          new_text: format!(r#""importMap": "{}""#, import_map_specifier),
-        import_map_specifier
+        });
-      );
+        should_format = true;
-      let new_text = format!(
+        modified_result.updated_import_map = true;
-        "{}{}{}",
+      }
-        &text[..insert_position],
+      // shouldn't happen
-        insert_text,
+      Some(_) => {
-        &text[insert_position..],
+        bail!("Failed updating importMap in config file due to invalid type.")
-      );
+      }
+    }
+  }
+
+  if text_changes.is_empty() {
+    return Ok(modified_result);
+  }
+
+  let new_text = deno_ast::apply_text_changes(text, text_changes);
+  modified_result.new_text = if should_format {
     format_json(&new_text, fmt_options)
       .ok()
       .map(|formatted_text| formatted_text.unwrap_or(new_text))
-    }
+  } else {
-    // shouldn't happen, so ignore
+    Some(new_text)
-    Some(_) => None,
+  };
-  }
+  Ok(modified_result)
 }
 
 fn is_dir_empty(dir_path: &Path) -> Result<bool, AnyError> {
@@ -288,36 +381,94 @@ mod internal_test {
 
   #[test]
   fn update_config_text_no_existing_props_add_prop() {
-    let text = update_config_text(
+    let result = update_config_text(
       "{\n}",
-      "./vendor/import_map.json",
       &Default::default(),
+      Some("./vendor/import_map.json"),
+      false,
     )
     .unwrap();
+    assert!(result.updated_import_map);
+    assert!(!result.added_node_modules_dir);
     assert_eq!(
-      text,
+      result.new_text.unwrap(),
       r#"{
   "importMap": "./vendor/import_map.json"
 }
+"#
+    );
+
+    let result = update_config_text(
+      "{\n}",
+      &Default::default(),
+      Some("./vendor/import_map.json"),
+      true,
+    )
+    .unwrap();
+    assert!(result.updated_import_map);
+    assert!(result.added_node_modules_dir);
+    assert_eq!(
+      result.new_text.unwrap(),
+      r#"{
+  "nodeModulesDir": true,
+  "importMap": "./vendor/import_map.json"
+}
+"#
+    );
+
+    let result =
+      update_config_text("{\n}", &Default::default(), None, true).unwrap();
+    assert!(!result.updated_import_map);
+    assert!(result.added_node_modules_dir);
+    assert_eq!(
+      result.new_text.unwrap(),
+      r#"{
+  "nodeModulesDir": true
+}
 "#
     );
   }
 
   #[test]
   fn update_config_text_existing_props_add_prop() {
-    let text = update_config_text(
+    let result = update_config_text(
       r#"{
   "tasks": {
     "task1": "other"
   }
 }
 "#,
-      "./vendor/import_map.json",
       &Default::default(),
+      Some("./vendor/import_map.json"),
+      false,
     )
     .unwrap();
     assert_eq!(
-      text,
+      result.new_text.unwrap(),
+      r#"{
+  "tasks": {
+    "task1": "other"
+  },
+  "importMap": "./vendor/import_map.json"
+}
+"#
+    );
+
+    // trailing comma
+    let result = update_config_text(
+      r#"{
+  "tasks": {
+    "task1": "other"
+  },
+}
+"#,
+      &Default::default(),
+      Some("./vendor/import_map.json"),
+      false,
+    )
+    .unwrap();
+    assert_eq!(
+      result.new_text.unwrap(),
       r#"{
   "tasks": {
     "task1": "other"
@@ -330,21 +481,54 @@ mod internal_test {
 
   #[test]
   fn update_config_text_update_prop() {
-    let text = update_config_text(
+    let result = update_config_text(
       r#"{
   "importMap": "./local.json"
 }
 "#,
-      "./vendor/import_map.json",
       &Default::default(),
+      Some("./vendor/import_map.json"),
+      false,
     )
     .unwrap();
     assert_eq!(
-      text,
+      result.new_text.unwrap(),
       r#"{
   "importMap": "./vendor/import_map.json"
 }
 "#
     );
   }
+
+  #[test]
+  fn no_update_node_modules_dir() {
+    // will not update if this is already set (even if it's false)
+    let result = update_config_text(
+      r#"{
+  "nodeModulesDir": false
+}
+"#,
+      &Default::default(),
+      None,
+      true,
+    )
+    .unwrap();
+    assert!(!result.added_node_modules_dir);
+    assert!(!result.updated_import_map);
+    assert_eq!(result.new_text, None);
+
+    let result = update_config_text(
+      r#"{
+  "nodeModulesDir": true
+}
+"#,
+      &Default::default(),
+      None,
+      true,
+    )
+    .unwrap();
+    assert!(!result.added_node_modules_dir);
+    assert!(!result.updated_import_map);
+    assert_eq!(result.new_text, None);
+  }
 }

cli/tools/vendor/specifiers.rs

@@ -65,7 +65,7 @@ pub fn make_url_relative(
 }
 
 pub fn is_remote_specifier(specifier: &ModuleSpecifier) -> bool {
-  specifier.scheme().to_lowercase().starts_with("http")
+  matches!(specifier.scheme().to_lowercase().as_str(), "http" | "https")
 }
 
 pub fn is_remote_specifier_text(text: &str) -> bool {

test_util/src/temp_dir.rs

@@ -58,6 +58,10 @@ impl TempDir {
     fs::create_dir_all(self.path().join(path)).unwrap();
   }
 
+  pub fn remove_file(&self, path: impl AsRef<Path>) {
+    fs::remove_file(self.path().join(path)).unwrap();
+  }
+
   pub fn remove_dir_all(&self, path: impl AsRef<Path>) {
     fs::remove_dir_all(self.path().join(path)).unwrap();
   }