FUTURE: override byonm with nodeModulesDir setting (#23222)

Makes the `"nodeModulesDir"` setting take precedence over byonm when
using `DENO_FUTURE`.

cli/args/mod.rs

@@ -1102,15 +1102,11 @@ impl CliOptions {
   }
 
   pub fn has_node_modules_dir(&self) -> bool {
-    if self.enable_future_features() {
-      self.maybe_node_modules_folder.is_some()
-    } else {
-      self.maybe_node_modules_folder.is_some() || self.unstable_byonm()
-    }
+    self.maybe_node_modules_folder.is_some()
   }
 
-  pub fn node_modules_dir_path(&self) -> Option<PathBuf> {
-    self.maybe_node_modules_folder.clone()
+  pub fn node_modules_dir_path(&self) -> Option<&PathBuf> {
+    self.maybe_node_modules_folder.as_ref()
   }
 
   pub fn with_node_modules_dir_path(&self, path: PathBuf) -> Self {
@@ -1595,10 +1591,14 @@ impl CliOptions {
   }
 
   pub fn use_byonm(&self) -> bool {
-    self.enable_future_features()
-  }
+    if self.enable_future_features()
+      && self.node_modules_dir_enablement().is_none()
+      && self.maybe_package_json.is_some()
+    {
+      return true;
+    }
 
-  pub fn unstable_byonm(&self) -> bool {
+    // check if enabled via unstable
     self.flags.unstable_config.byonm
       || NPM_PROCESS_STATE
         .as_ref()

cli/factory.rs

@@ -402,11 +402,11 @@ impl CliFactory {
       .npm_resolver
       .get_or_try_init_async(async {
         let fs = self.fs();
-        create_cli_npm_resolver(if self.options.use_byonm() || self.options.unstable_byonm() {
+        create_cli_npm_resolver(if self.options.use_byonm() {
           CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
             fs: fs.clone(),
-            root_node_modules_dir: match self.options.node_modules_dir_path().clone() {
-              Some(node_modules_path) => node_modules_path,
+            root_node_modules_dir: match self.options.node_modules_dir_path() {
+              Some(node_modules_path) => node_modules_path.to_path_buf(),
               // path needs to be canonicalized for node resolution
               // (node_modules_dir_path above is already canonicalized)
               None => canonicalize_path_maybe_not_exists(self.options.initial_cwd())?
@@ -434,7 +434,7 @@ impl CliFactory {
             npm_global_cache_dir: self.deno_dir()?.npm_folder_path(),
             cache_setting: self.options.cache_setting(),
             text_only_progress_bar: self.text_only_progress_bar().clone(),
-            maybe_node_modules_path: self.options.node_modules_dir_path(),
+            maybe_node_modules_path: self.options.node_modules_dir_path().cloned(),
             package_json_installer:
               CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall(
                 self.package_json_deps_provider().clone(),

cli/standalone/binary.rs

@@ -635,7 +635,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
       unstable_config: UnstableConfig {
         legacy_flag_enabled: cli_options.legacy_unstable_flag(),
         bare_node_builtins: cli_options.unstable_bare_node_builtins(),
-        byonm: cli_options.use_byonm() || cli_options.unstable_byonm(),
+        byonm: cli_options.use_byonm(),
         sloppy_imports: cli_options.unstable_sloppy_imports(),
         features: cli_options.unstable_features(),
       },

cli/tools/vendor/mod.rs

@@ -99,13 +99,14 @@ pub async fn vendor(
 
   // 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"))
-    });
+    let node_modules_path =
+      cli_options.node_modules_dir_path().cloned().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 {
       let cli_options =
         cli_options.with_node_modules_dir_path(node_modules_path);

tests/specs/npm/future_auto_install_no_package_json/__test__.jsonc

@@ -0,0 +1,10 @@
+{
+  "envs": {
+    "DENO_FUTURE": "1"
+  },
+  "steps": [{
+    // should auto-install because no package.json
+    "args": "run --quiet main.ts",
+    "output": "main.out"
+  }]
+}

tests/specs/npm/future_auto_install_no_package_json/main.out

@@ -0,0 +1 @@
+2

tests/specs/npm/future_auto_install_no_package_json/main.ts

@@ -0,0 +1,4 @@
+import { getValue, setValue } from "npm:@denotest/esm-basic";
+
+setValue(2);
+console.log(getValue());

tests/specs/npm/future_node_modules_dir_setting/__test__.jsonc

@@ -0,0 +1,20 @@
+{
+  "envs": {
+    "DENO_FUTURE": "1"
+  },
+  "tempDir": true,
+  "steps": [{
+    // byonm where this fails be
+    "args": "run main.ts",
+    "output": "failed.out",
+    "exitCode": 1
+  }, {
+    // this should override byonm
+    "args": "run --node-modules-dir=false --quiet main.ts",
+    "output": "main.out"
+  }, {
+    // same with this
+    "args": "run --node-modules-dir --quiet main.ts",
+    "output": "main.out"
+  }]
+}

tests/specs/npm/future_node_modules_dir_setting/failed.out

@@ -0,0 +1 @@
+error: Could not resolve "@denotest/esm-basic", but found it in a package.json.[WILDCARD]

tests/specs/npm/future_node_modules_dir_setting/main.out

@@ -0,0 +1 @@
+2

tests/specs/npm/future_node_modules_dir_setting/main.ts

@@ -0,0 +1,4 @@
+import { getValue, setValue } from "@denotest/esm-basic";
+
+setValue(2);
+console.log(getValue());

tests/specs/npm/future_node_modules_dir_setting/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "@denotest/esm-basic": "*"
+  }
+}