From 55b85d4992a9cdbde23f1b53d18444fde8a161b5 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 12 Sep 2022 15:47:54 -0400 Subject: [PATCH] fix(npm): support cjs resolution of package subpath with package.json (#15855) --- cli/node/mod.rs | 38 ++++++++++++++++--- cli/tests/testdata/npm/dual_cjs_esm/main.out | 2 + cli/tests/testdata/npm/dual_cjs_esm/main.ts | 3 ++ .../@denotest/dual-cjs-esm/1.0.0/cjs/main.cjs | 10 +++++ .../dual-cjs-esm/1.0.0/cjs/package.json | 3 ++ .../1.0.0/{index.cjs => main.cjs} | 0 .../1.0.0/{index.mjs => main.mjs} | 0 .../@denotest/dual-cjs-esm/1.0.0/package.json | 4 +- .../dual-cjs-esm/1.0.0/subpath/main.cjs | 3 ++ .../dual-cjs-esm/1.0.0/subpath/main.mjs | 3 ++ .../dual-cjs-esm/1.0.0/subpath/package.json | 5 +++ ext/node/02_require.js | 12 +++--- ext/node/lib.rs | 9 ++--- ext/node/package_json.rs | 14 ++++++- ext/node/resolution.rs | 7 +--- 15 files changed, 85 insertions(+), 28 deletions(-) create mode 100644 cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/main.cjs create mode 100644 cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/package.json rename cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/{index.cjs => main.cjs} (100%) rename cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/{index.mjs => main.mjs} (100%) create mode 100644 cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.cjs create mode 100644 cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.mjs create mode 100644 cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/package.json diff --git a/cli/node/mod.rs b/cli/node/mod.rs index 75fdfcd7d5..690a211818 100644 --- a/cli/node/mod.rs +++ b/cli/node/mod.rs @@ -9,6 +9,7 @@ use crate::deno_std::CURRENT_STD_URL; use deno_ast::CjsAnalysis; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; +use deno_core::anyhow::anyhow; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::generic_error; @@ -534,9 +535,7 @@ fn package_config_resolve( referrer_kind: NodeModuleKind, ) -> Result { let package_json_path = package_dir.join("package.json"); - let referrer = - ModuleSpecifier::from_directory_path(package_json_path.parent().unwrap()) - .unwrap(); + let referrer = ModuleSpecifier::from_directory_path(package_dir).unwrap(); let package_config = PackageJson::load(npm_resolver, package_json_path.clone())?; if let Some(exports) = &package_config.exports { @@ -770,7 +769,16 @@ pub fn translate_cjs_to_esm( let reexport_specifier = ModuleSpecifier::from_file_path(&resolved_reexport).unwrap(); // Second, read the source code from disk - let reexport_file = file_fetcher.get_source(&reexport_specifier).unwrap(); + let reexport_file = file_fetcher + .get_source(&reexport_specifier) + .ok_or_else(|| { + anyhow!( + "Could not find '{}' ({}) referenced from {}", + reexport, + reexport_specifier, + referrer + ) + })?; { let analysis = perform_cjs_analysis( @@ -873,11 +881,21 @@ fn resolve( let d = module_dir.join(package_subpath); if let Ok(m) = d.metadata() { if m.is_dir() { + // subdir might have a package.json that specifies the entrypoint + let package_json_path = d.join("package.json"); + if package_json_path.exists() { + let package_json = + PackageJson::load(npm_resolver, package_json_path)?; + if let Some(main) = package_json.main(NodeModuleKind::Cjs) { + return Ok(d.join(main).clean()); + } + } + return Ok(d.join("index.js").clean()); } } return file_extension_probe(d, &referrer_path); - } else if let Some(main) = package_json.main { + } else if let Some(main) = package_json.main(NodeModuleKind::Cjs) { return Ok(module_dir.join(main).clean()); } else { return Ok(module_dir.join("index.js").clean()); @@ -895,7 +913,7 @@ fn parse_specifier(specifier: &str) -> Option<(String, String)> { } else if specifier.starts_with('@') { // is_scoped = true; if let Some(index) = separator_index { - separator_index = specifier[index + 1..].find('/'); + separator_index = specifier[index + 1..].find('/').map(|i| i + index + 1); } else { valid_package_name = false; } @@ -1027,4 +1045,12 @@ mod tests { ] ) } + + #[test] + fn test_parse_specifier() { + assert_eq!( + parse_specifier("@some-package/core/actions"), + Some(("@some-package/core".to_string(), "./actions".to_string())) + ); + } } diff --git a/cli/tests/testdata/npm/dual_cjs_esm/main.out b/cli/tests/testdata/npm/dual_cjs_esm/main.out index 3d329be7a4..32e232f11c 100644 --- a/cli/tests/testdata/npm/dual_cjs_esm/main.out +++ b/cli/tests/testdata/npm/dual_cjs_esm/main.out @@ -1 +1,3 @@ esm +cjs +cjs diff --git a/cli/tests/testdata/npm/dual_cjs_esm/main.ts b/cli/tests/testdata/npm/dual_cjs_esm/main.ts index 3e80402c6d..6d973affb8 100644 --- a/cli/tests/testdata/npm/dual_cjs_esm/main.ts +++ b/cli/tests/testdata/npm/dual_cjs_esm/main.ts @@ -1,3 +1,6 @@ import { getKind } from "npm:@denotest/dual-cjs-esm"; +import * as cjs from "npm:@denotest/dual-cjs-esm/cjs/main.cjs"; console.log(getKind()); +console.log(cjs.getKind()); +console.log(cjs.getSubPathKind()); diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/main.cjs b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/main.cjs new file mode 100644 index 0000000000..51d32ff89c --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/main.cjs @@ -0,0 +1,10 @@ +const root = require("../"); +const subPath = require("../subpath"); + +module.exports.getKind = function() { + return root.getKind(); +}; + +module.exports.getSubPathKind = function() { + return subPath.getSubPathKind(); +}; diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/package.json b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/package.json new file mode 100644 index 0000000000..73847e365b --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/cjs/package.json @@ -0,0 +1,3 @@ +{ + "main": "./main.cjs" +} \ No newline at end of file diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/index.cjs b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/main.cjs similarity index 100% rename from cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/index.cjs rename to cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/main.cjs diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/index.mjs b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/main.mjs similarity index 100% rename from cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/index.mjs rename to cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/main.mjs diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/package.json index e0315b7f67..18b72e97a9 100644 --- a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/package.json +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/package.json @@ -2,6 +2,6 @@ "name": "@denotest/dual-cjs-esm", "version": "1.0.0", "type": "module", - "main": "./index.cjs", - "module": "./index.mjs" + "main": "./main.cjs", + "module": "./main.mjs" } diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.cjs b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.cjs new file mode 100644 index 0000000000..18a22e6f19 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.cjs @@ -0,0 +1,3 @@ +exports.getSubPathKind = function() { + return "cjs"; +}; diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.mjs b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.mjs new file mode 100644 index 0000000000..47e8cd5162 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/main.mjs @@ -0,0 +1,3 @@ +export function getSubPathKind() { + return "esm"; +} diff --git a/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/package.json b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/package.json new file mode 100644 index 0000000000..149ce36a3a --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/dual-cjs-esm/1.0.0/subpath/package.json @@ -0,0 +1,5 @@ +{ + "type": "module", + "main": "./main.cjs", + "module": "./main.mjs" +} \ No newline at end of file diff --git a/ext/node/02_require.js b/ext/node/02_require.js index f3ed266c3e..578d8e8739 100644 --- a/ext/node/02_require.js +++ b/ext/node/02_require.js @@ -98,7 +98,11 @@ } function tryPackage(requestPath, exts, isMain, originalPath) { - const pkg = core.ops.op_require_read_package_scope(requestPath).main; + const packageJsonPath = pathResolve( + requestPath, + "package.json", + ); + const pkg = core.ops.op_require_read_package_scope(packageJsonPath).main; if (!pkg) { return tryExtensions( pathResolve(requestPath, "index"), @@ -135,12 +139,8 @@ err.requestPath = originalPath; throw err; } else { - const jsonPath = pathResolve( - requestPath, - "package.json", - ); node.globalThis.process.emitWarning( - `Invalid 'main' field in '${jsonPath}' of '${pkg}'. ` + + `Invalid 'main' field in '${packageJsonPath}' of '${pkg}'. ` + "Please either fix that or report it to the module author", "DeprecationWarning", "DEP0128", diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 753a11b5de..db4fe31786 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -604,15 +604,12 @@ where #[op] fn op_require_read_package_scope( state: &mut OpState, - filename: String, + package_json_path: String, ) -> Option { check_unstable(state); let resolver = state.borrow::>().clone(); - resolution::get_package_scope_config( - &Url::from_file_path(filename).unwrap(), - &*resolver, - ) - .ok() + let package_json_path = PathBuf::from(package_json_path); + PackageJson::load(&*resolver, package_json_path).ok() } #[op] diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index e5f9214fb8..ced64a1b40 100644 --- a/ext/node/package_json.rs +++ b/ext/node/package_json.rs @@ -1,5 +1,7 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. +use crate::NodeModuleKind; + use super::DenoDirNpmResolver; use deno_core::anyhow; use deno_core::anyhow::bail; @@ -17,8 +19,8 @@ pub struct PackageJson { pub exports: Option>, pub imports: Option>, pub bin: Option, - pub main: Option, - pub module: Option, + main: Option, // use .main(...) + module: Option, // use .main(...) pub name: Option, pub path: PathBuf, pub typ: String, @@ -123,6 +125,14 @@ impl PackageJson { }; Ok(package_json) } + + pub fn main(&self, referrer_kind: NodeModuleKind) -> Option<&String> { + if referrer_kind == NodeModuleKind::Esm && self.typ == "module" { + self.module.as_ref().or(self.main.as_ref()) + } else { + self.main.as_ref() + } + } } fn is_conditional_exports_main_sugar(exports: &Value) -> bool { diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index d6734066cf..a7428fe03a 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -707,12 +707,7 @@ pub fn legacy_main_resolve( package_json: &PackageJson, referrer_kind: NodeModuleKind, ) -> Result { - let maybe_main = - if referrer_kind == NodeModuleKind::Esm && package_json.typ == "module" { - package_json.module.as_ref().or(package_json.main.as_ref()) - } else { - package_json.main.as_ref() - }; + let maybe_main = package_json.main(referrer_kind); let mut guess; if let Some(main) = maybe_main {