From ac04787c30b67ba9c7fb800eae8361ba419e7f4e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 14 Dec 2023 16:09:05 +0100 Subject: [PATCH] fix(node): support resolving a package.json import to a builtin node module (#21576) Closes https://github.com/denoland/deno/issues/21501 --- .../testdata/npm/imports_package_json/main.js | 3 + .../npm/imports_package_json/main.out | 3 + .../imports-package-json/1.0.0/main.js | 6 + .../imports-package-json/1.0.0/package.json | 7 +- ext/node/analyze.rs | 24 +- ext/node/errors.rs | 2 +- ext/node/ops/require.rs | 18 +- ext/node/path.rs | 10 + ext/node/polyfill.rs | 14 + ext/node/resolution.rs | 242 ++++++++++-------- 10 files changed, 205 insertions(+), 124 deletions(-) diff --git a/cli/tests/testdata/npm/imports_package_json/main.js b/cli/tests/testdata/npm/imports_package_json/main.js index cd7bbf8e91..53090dd948 100644 --- a/cli/tests/testdata/npm/imports_package_json/main.js +++ b/cli/tests/testdata/npm/imports_package_json/main.js @@ -2,3 +2,6 @@ import data from "@denotest/imports-package-json"; console.log(data.hi); console.log(data.bye); +console.log(typeof data.fs.readFile); +console.log(typeof data.path.join); +console.log(typeof data.fs2.writeFile); diff --git a/cli/tests/testdata/npm/imports_package_json/main.out b/cli/tests/testdata/npm/imports_package_json/main.out index f006f18d54..9f702b96b6 100644 --- a/cli/tests/testdata/npm/imports_package_json/main.out +++ b/cli/tests/testdata/npm/imports_package_json/main.out @@ -2,3 +2,6 @@ Download http://localhost:4545/npm/registry/@denotest/imports-package-json Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz hi bye +function +function +function diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js index 46625479eb..9e7c247b78 100644 --- a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js @@ -1,7 +1,13 @@ import hi from "#hi"; import bye from "./sub_path/main.js"; +import fs from "#fs"; +import path from "#path"; +import fs2 from "#fs2"; export default { hi, bye, + fs, + path, + fs2, }; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json index a4fbc88895..2c294e680e 100644 --- a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json @@ -11,6 +11,11 @@ "./sub-path-import-not-defined": "./sub_path/import_not_defined.js" }, "imports": { - "#hi": "./hi.js" + "#hi": "./hi.js", + "#fs": "fs", + "#path": "node:path", + "#fs2": { + "node": "fs" + } } } diff --git a/ext/node/analyze.rs b/ext/node/analyze.rs index 173ce53852..fc3fee0cb1 100644 --- a/ext/node/analyze.rs +++ b/ext/node/analyze.rs @@ -11,6 +11,7 @@ use once_cell::sync::Lazy; use deno_core::error::AnyError; +use crate::path::to_file_specifier; use crate::resolution::NodeResolverRc; use crate::NodeModuleKind; use crate::NodePermissions; @@ -106,7 +107,7 @@ impl NodeCodeTranslator { handled_reexports.insert(reexport.to_string()); // First, resolve the reexport specifier - let resolved_reexport = self.resolve( + let reexport_specifier = self.resolve( &reexport, &referrer, // FIXME(bartlomieju): check if these conditions are okay, probably @@ -117,8 +118,6 @@ impl NodeCodeTranslator { )?; // Second, resolve its exports and re-exports - let reexport_specifier = - ModuleSpecifier::from_file_path(&resolved_reexport).unwrap(); let analysis = self .cjs_code_analyzer .analyze_cjs(&reexport_specifier, None) @@ -177,7 +176,7 @@ impl NodeCodeTranslator { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result { + ) -> Result { if specifier.starts_with('/') { todo!(); } @@ -186,7 +185,8 @@ impl NodeCodeTranslator { if specifier.starts_with("./") || specifier.starts_with("../") { if let Some(parent) = referrer_path.parent() { return self - .file_extension_probe(parent.join(specifier), &referrer_path); + .file_extension_probe(parent.join(specifier), &referrer_path) + .map(|p| to_file_specifier(&p)); } else { todo!(); } @@ -238,17 +238,19 @@ impl NodeCodeTranslator { )?; if package_json.exists { if let Some(main) = package_json.main(NodeModuleKind::Cjs) { - return Ok(d.join(main).clean()); + return Ok(to_file_specifier(&d.join(main).clean())); } } - return Ok(d.join("index.js").clean()); + return Ok(to_file_specifier(&d.join("index.js").clean())); } - return self.file_extension_probe(d, &referrer_path); + return self + .file_extension_probe(d, &referrer_path) + .map(|p| to_file_specifier(&p)); } else if let Some(main) = package_json.main(NodeModuleKind::Cjs) { - return Ok(module_dir.join(main).clean()); + return Ok(to_file_specifier(&module_dir.join(main).clean())); } else { - return Ok(module_dir.join("index.js").clean()); + return Ok(to_file_specifier(&module_dir.join("index.js").clean())); } } @@ -264,7 +266,7 @@ impl NodeCodeTranslator { parent.join("node_modules").join(specifier) }; if let Ok(path) = self.file_extension_probe(path, &referrer_path) { - return Ok(path); + return Ok(to_file_specifier(&path)); } last = parent; } diff --git a/ext/node/errors.rs b/ext/node/errors.rs index f34707c66c..27c4024bc5 100644 --- a/ext/node/errors.rs +++ b/ext/node/errors.rs @@ -54,7 +54,7 @@ pub fn err_module_not_found(path: &str, base: &str, typ: &str) -> AnyError { pub fn err_invalid_package_target( pkg_path: &str, key: &str, - target: String, + target: &str, is_import: bool, maybe_referrer: Option, ) -> AnyError { diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 8010558c5c..ada123686a 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -431,7 +431,13 @@ where NodeResolutionMode::Execution, permissions, ) - .map(|r| Some(r.to_string_lossy().to_string())) + .map(|r| { + Some(if r.scheme() == "file" { + r.to_file_path().unwrap().to_string_lossy().to_string() + } else { + r.to_string() + }) + }) } else { Ok(None) } @@ -515,7 +521,13 @@ where NodeResolutionMode::Execution, permissions, ) - .map(|r| Some(r.to_string_lossy().to_string())) + .map(|r| { + Some(if r.scheme() == "file" { + r.to_file_path().unwrap().to_string_lossy().to_string() + } else { + r.to_string() + }) + }) } else { Ok(None) } @@ -588,7 +600,7 @@ where NodeResolutionMode::Execution, permissions, ) - .map(|r| Some(Url::from_file_path(r).unwrap().to_string())) + .map(|r| Some(r.to_string())) } else { Ok(None) } diff --git a/ext/node/path.rs b/ext/node/path.rs index 71cc0741e3..e20555a2c1 100644 --- a/ext/node/path.rs +++ b/ext/node/path.rs @@ -1,8 +1,11 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use std::path::Component; +use std::path::Path; use std::path::PathBuf; +use deno_core::ModuleSpecifier; + /// Extension to path_clean::PathClean pub trait PathClean { fn clean(&self) -> T; @@ -38,3 +41,10 @@ impl PathClean for PathBuf { } } } + +pub(crate) fn to_file_specifier(path: &Path) -> ModuleSpecifier { + match ModuleSpecifier::from_file_path(path) { + Ok(url) => url, + Err(_) => panic!("Invalid path: {}", path.display()), + } +} diff --git a/ext/node/polyfill.rs b/ext/node/polyfill.rs index 7772e3a16a..a4c87bef71 100644 --- a/ext/node/polyfill.rs +++ b/ext/node/polyfill.rs @@ -1,5 +1,7 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use deno_core::ModuleSpecifier; + /// e.g. `is_builtin_node_module("assert")` pub fn is_builtin_node_module(module_name: &str) -> bool { SUPPORTED_BUILTIN_NODE_MODULES @@ -7,6 +9,18 @@ pub fn is_builtin_node_module(module_name: &str) -> bool { .any(|m| *m == module_name) } +/// Ex. returns `fs` for `node:fs` +pub fn get_module_name_from_builtin_node_module_specifier( + specifier: &ModuleSpecifier, +) -> Option<&str> { + if specifier.scheme() != "node" { + return None; + } + + let (_, specifier) = specifier.as_str().split_once(':')?; + Some(specifier) +} + macro_rules! generate_builtin_node_module_lists { ($( $module_name:literal ,)+) => { pub static SUPPORTED_BUILTIN_NODE_MODULES: &[&str] = &[ diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index 65c2e1bcdc..e3c8855fa1 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -17,6 +17,9 @@ use deno_fs::FileSystemRc; use deno_media_type::MediaType; use crate::errors; +use crate::is_builtin_node_module; +use crate::path::to_file_specifier; +use crate::polyfill::get_module_name_from_builtin_node_module_specifier; use crate::AllowAllNodePermissions; use crate::NodePermissions; use crate::NpmResolverRc; @@ -164,17 +167,14 @@ impl NodeResolver { return Ok(Some(NodeResolution::Esm(url))); } - let protocol = url.scheme(); - - if protocol == "node" { - let split_specifier = url.as_str().split(':'); - let specifier = split_specifier.skip(1).collect::(); - - if crate::is_builtin_node_module(&specifier) { - return Ok(Some(NodeResolution::BuiltIn(specifier))); - } + if let Some(module_name) = + get_module_name_from_builtin_node_module_specifier(&url) + { + return Ok(Some(NodeResolution::BuiltIn(module_name.to_string()))); } + let protocol = url.scheme(); + if protocol != "file" && protocol != "data" { return Err(errors::err_unsupported_esm_url_scheme(&url)); } @@ -203,12 +203,10 @@ impl NodeResolver { let path = url.to_file_path().unwrap(); // todo(16370): the module kind is not correct here. I think we need // typescript to tell us if the referrer is esm or cjs - let path = - match self.path_to_declaration_path(path, NodeModuleKind::Esm) { - Some(path) => path, - None => return Ok(None), - }; - ModuleSpecifier::from_file_path(path).unwrap() + match self.path_to_declaration_path(path, NodeModuleKind::Esm) { + Some(path) => to_file_specifier(&path), + None => return Ok(None), + } } }; @@ -235,38 +233,31 @@ impl NodeResolver { // should use the value provided by typescript instead let declaration_path = self.path_to_declaration_path(file_path, NodeModuleKind::Esm); - declaration_path.map(|declaration_path| { - ModuleSpecifier::from_file_path(declaration_path).unwrap() - }) + declaration_path + .map(|declaration_path| to_file_specifier(&declaration_path)) } else { Some(resolved_specifier) } } else if specifier.starts_with('#') { - Some( - self - .package_imports_resolve( - specifier, - referrer, - NodeModuleKind::Esm, - conditions, - mode, - permissions, - ) - .map(|p| ModuleSpecifier::from_file_path(p).unwrap())?, - ) + Some(self.package_imports_resolve( + specifier, + referrer, + NodeModuleKind::Esm, + conditions, + mode, + permissions, + )?) } else if let Ok(resolved) = Url::parse(specifier) { Some(resolved) } else { - self - .package_resolve( - specifier, - referrer, - NodeModuleKind::Esm, - conditions, - mode, - permissions, - )? - .map(|p| ModuleSpecifier::from_file_path(p).unwrap()) + self.package_resolve( + specifier, + referrer, + NodeModuleKind::Esm, + conditions, + mode, + permissions, + )? }; Ok(match url { Some(url) => Some(self.finalize_resolution(url, referrer)?), @@ -289,6 +280,10 @@ impl NodeResolver { )); } + if resolved.scheme() == "node" { + return Ok(resolved); + } + let path = to_file_path(&resolved); // TODO(bartlomieju): currently not supported @@ -340,7 +335,7 @@ impl NodeResolver { let package_subpath = package_subpath .map(|s| format!("./{s}")) .unwrap_or_else(|| ".".to_string()); - let maybe_resolved_path = self + let maybe_resolved_url = self .resolve_package_subpath( &package_json, &package_subpath, @@ -357,21 +352,25 @@ impl NodeResolver { package_json.path.display() ) })?; - let resolved_path = match maybe_resolved_path { + let resolved_url = match maybe_resolved_url { Some(resolved_path) => resolved_path, None => return Ok(None), }; - let resolved_path = match mode { - NodeResolutionMode::Execution => resolved_path, + let resolved_url = match mode { + NodeResolutionMode::Execution => resolved_url, NodeResolutionMode::Types => { - match self.path_to_declaration_path(resolved_path, node_module_kind) { - Some(path) => path, - None => return Ok(None), + if resolved_url.scheme() == "file" { + let path = resolved_url.to_file_path().unwrap(); + match self.path_to_declaration_path(path, node_module_kind) { + Some(path) => to_file_specifier(&path), + None => return Ok(None), + } + } else { + resolved_url } } }; - let url = ModuleSpecifier::from_file_path(resolved_path).unwrap(); - let resolve_response = self.url_to_node_resolution(url)?; + let resolve_response = self.url_to_node_resolution(resolved_url)?; // TODO(bartlomieju): skipped checking errors for commonJS resolution and // "preserveSymlinksMain"/"preserveSymlinks" options. Ok(Some(resolve_response)) @@ -408,8 +407,7 @@ impl NodeResolver { let package_json = self .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?; let bin_entry = resolve_bin_entry_value(&package_json, sub_path)?; - let url = - ModuleSpecifier::from_file_path(package_folder.join(bin_entry)).unwrap(); + let url = to_file_specifier(&package_folder.join(bin_entry)); let resolve_response = self.url_to_node_resolution(url)?; // TODO(bartlomieju): skipped checking errors for commonJS resolution and @@ -531,7 +529,7 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result { + ) -> Result { if name == "#" || name.starts_with("#/") || name.ends_with('/') { let reason = "is not a valid internal imports specifier name"; return Err(errors::err_invalid_module_specifier( @@ -549,9 +547,10 @@ impl NodeResolver { package_json_path = Some(package_config.path.clone()); if let Some(imports) = &package_config.imports { if imports.contains_key(name) && !name.contains('*') { + let target = imports.get(name).unwrap(); let maybe_resolved = self.resolve_package_target( package_json_path.as_ref().unwrap(), - imports.get(name).unwrap().to_owned(), + target, "", name, referrer, @@ -591,7 +590,7 @@ impl NodeResolver { } if !best_match.is_empty() { - let target = imports.get(best_match).unwrap().to_owned(); + let target = imports.get(best_match).unwrap(); let maybe_resolved = self.resolve_package_target( package_json_path.as_ref().unwrap(), target, @@ -624,7 +623,7 @@ impl NodeResolver { #[allow(clippy::too_many_arguments)] fn resolve_package_target_string( &self, - target: String, + target: &str, subpath: &str, match_: &str, package_json_path: &Path, @@ -635,7 +634,7 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result { + ) -> Result { if !subpath.is_empty() && !pattern && !target.ends_with('/') { return Err(throw_invalid_package_target( match_, @@ -650,29 +649,51 @@ impl NodeResolver { let pattern_re = lazy_regex::regex!(r"\*"); if !target.starts_with("./") { if internal && !target.starts_with("../") && !target.starts_with('/') { - let is_url = Url::parse(&target).is_ok(); - if !is_url { - let export_target = if pattern { - pattern_re - .replace(&target, |_caps: ®ex::Captures| subpath) - .to_string() - } else { - format!("{target}{subpath}") - }; - let package_json_url = - ModuleSpecifier::from_file_path(package_json_path).unwrap(); - return match self.package_resolve( - &export_target, - &package_json_url, - referrer_kind, - conditions, - mode, - permissions, - ) { - Ok(Some(path)) => Ok(path), - Ok(None) => Err(generic_error("not found")), - Err(err) => Err(err), - }; + let target_url = Url::parse(target); + match target_url { + Ok(url) => { + if get_module_name_from_builtin_node_module_specifier(&url) + .is_some() + { + return Ok(url); + } + } + Err(_) => { + let export_target = if pattern { + pattern_re + .replace(target, |_caps: ®ex::Captures| subpath) + .to_string() + } else { + format!("{target}{subpath}") + }; + let package_json_url = to_file_specifier(package_json_path); + let result = match self.package_resolve( + &export_target, + &package_json_url, + referrer_kind, + conditions, + mode, + permissions, + ) { + Ok(Some(url)) => Ok(url), + Ok(None) => Err(generic_error("not found")), + Err(err) => Err(err), + }; + + return match result { + Ok(url) => Ok(url), + Err(err) => { + if is_builtin_node_module(target) { + Ok( + ModuleSpecifier::parse(&format!("node:{}", target)) + .unwrap(), + ) + } else { + Err(err) + } + } + }; + } } } return Err(throw_invalid_package_target( @@ -693,7 +714,7 @@ impl NodeResolver { )); } let package_path = package_json_path.parent().unwrap(); - let resolved_path = package_path.join(&target).clean(); + let resolved_path = package_path.join(target).clean(); if !resolved_path.starts_with(package_path) { return Err(throw_invalid_package_target( match_, @@ -704,7 +725,7 @@ impl NodeResolver { )); } if subpath.is_empty() { - return Ok(resolved_path); + return Ok(to_file_specifier(&resolved_path)); } if invalid_segment_re.is_match(subpath) { let request = if pattern { @@ -723,16 +744,16 @@ impl NodeResolver { let resolved_path_str = resolved_path.to_string_lossy(); let replaced = pattern_re .replace(&resolved_path_str, |_caps: ®ex::Captures| subpath); - return Ok(PathBuf::from(replaced.to_string())); + return Ok(to_file_specifier(&PathBuf::from(replaced.to_string()))); } - Ok(resolved_path.join(subpath).clean()) + Ok(to_file_specifier(&resolved_path.join(subpath).clean())) } #[allow(clippy::too_many_arguments)] fn resolve_package_target( &self, package_json_path: &Path, - target: Value, + target: &Value, subpath: &str, package_subpath: &str, referrer: &ModuleSpecifier, @@ -742,11 +763,11 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result, AnyError> { if let Some(target) = target.as_str() { return self .resolve_package_target_string( - target.to_string(), + target, subpath, package_subpath, package_json_path, @@ -758,11 +779,14 @@ impl NodeResolver { mode, permissions, ) - .map(|path| { - if mode.is_types() { - self.path_to_declaration_path(path, referrer_kind) + .map(|url| { + if mode.is_types() && url.scheme() == "file" { + let path = url.to_file_path().unwrap(); + self + .path_to_declaration_path(path, referrer_kind) + .map(|path| to_file_specifier(&path)) } else { - Some(path) + Some(url) } }); } else if let Some(target_arr) = target.as_array() { @@ -774,7 +798,7 @@ impl NodeResolver { for target_item in target_arr { let resolved_result = self.resolve_package_target( package_json_path, - target_item.to_owned(), + target_item, subpath, package_subpath, referrer, @@ -819,7 +843,7 @@ impl NodeResolver { || conditions.contains(&key.as_str()) || mode.is_types() && key.as_str() == "types" { - let condition_target = target_obj.get(key).unwrap().to_owned(); + let condition_target = target_obj.get(key).unwrap(); let resolved = self.resolve_package_target( package_json_path, @@ -848,7 +872,7 @@ impl NodeResolver { Err(throw_invalid_package_target( package_subpath, - target.to_string(), + &target.to_string(), package_json_path, internal, referrer, @@ -866,12 +890,12 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result { + ) -> Result { if package_exports.contains_key(package_subpath) && package_subpath.find('*').is_none() && !package_subpath.ends_with('/') { - let target = package_exports.get(package_subpath).unwrap().to_owned(); + let target = package_exports.get(package_subpath).unwrap(); let resolved = self.resolve_package_target( package_json_path, target, @@ -885,15 +909,15 @@ impl NodeResolver { mode, permissions, )?; - if resolved.is_none() { - return Err(throw_exports_not_found( + return match resolved { + Some(resolved) => Ok(resolved), + None => Err(throw_exports_not_found( package_subpath, package_json_path, referrer, mode, - )); - } - return Ok(resolved.unwrap()); + )), + }; } let mut best_match = ""; @@ -931,7 +955,7 @@ impl NodeResolver { } if !best_match.is_empty() { - let target = package_exports.get(best_match).unwrap().to_owned(); + let target = package_exports.get(best_match).unwrap(); let maybe_resolved = self.resolve_package_target( package_json_path, target, @@ -973,7 +997,7 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result, AnyError> { let (package_name, package_subpath, _is_scoped) = parse_npm_pkg_name(specifier, referrer)?; @@ -1044,7 +1068,7 @@ impl NodeResolver { conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result, AnyError> { if let Some(exports) = &package_json.exports { let result = self.package_exports_resolve( &package_json.path, @@ -1063,7 +1087,7 @@ impl NodeResolver { if let Ok(Some(path)) = self.legacy_main_resolve(package_json, referrer_kind, mode) { - return Ok(Some(path)); + return Ok(Some(to_file_specifier(&path))); } else { return Ok(None); } @@ -1073,16 +1097,18 @@ impl NodeResolver { } } if package_subpath == "." { - return self.legacy_main_resolve(package_json, referrer_kind, mode); + return self + .legacy_main_resolve(package_json, referrer_kind, mode) + .map(|maybe_resolved| maybe_resolved.map(|p| to_file_specifier(&p))); } let file_path = package_json.path.parent().unwrap().join(package_subpath); if mode.is_types() { let maybe_declaration_path = self.path_to_declaration_path(file_path, referrer_kind); - Ok(maybe_declaration_path) + Ok(maybe_declaration_path.map(|p| to_file_specifier(&p))) } else { - Ok(Some(file_path)) + Ok(Some(to_file_specifier(&file_path))) } } @@ -1416,7 +1442,7 @@ fn throw_import_not_defined( fn throw_invalid_package_target( subpath: &str, - target: String, + target: &str, package_json_path: &Path, internal: bool, referrer: &ModuleSpecifier,