fix(check): CJS types importing dual ESM/CJS package should prefer CJS types (#24492)

Closes #16370
This commit is contained in:
David Sherret 2024-07-10 14:46:25 -04:00 committed by GitHub
parent 4d2d764816
commit a49d0bd10b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
20 changed files with 246 additions and 88 deletions

View file

@ -409,13 +409,13 @@ impl CliFactory {
create_cli_npm_resolver(if self.options.use_byonm() && !matches!(self.options.sub_command(), DenoSubcommand::Install(_)) {
CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
fs: fs.clone(),
root_node_modules_dir: match self.options.node_modules_dir_path() {
root_node_modules_dir: Some(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())?
.join("node_modules"),
},
}),
})
} else {
CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
@ -732,7 +732,7 @@ impl CliFactory {
.cli_node_resolver
.get_or_try_init_async(async {
Ok(Arc::new(CliNodeResolver::new(
Some(self.cjs_resolutions().clone()),
self.cjs_resolutions().clone(),
self.fs().clone(),
self.node_resolver().await?.clone(),
self.npm_resolver().await?.clone(),

View file

@ -4,6 +4,7 @@ use crate::args::create_default_npmrc;
use crate::args::CacheSetting;
use crate::args::CliLockfile;
use crate::args::PackageJsonInstallDepsProvider;
use crate::args::DENO_FUTURE;
use crate::graph_util::CliJsrUrlProvider;
use crate::http_util::HttpClientProvider;
use crate::lsp::config::Config;
@ -16,6 +17,7 @@ use crate::npm::CliNpmResolverCreateOptions;
use crate::npm::CliNpmResolverManagedCreateOptions;
use crate::npm::CliNpmResolverManagedSnapshotOption;
use crate::npm::ManagedCliNpmResolver;
use crate::resolver::CjsResolutionStore;
use crate::resolver::CliGraphResolver;
use crate::resolver::CliGraphResolverOptions;
use crate::resolver::CliNodeResolver;
@ -38,6 +40,7 @@ use deno_runtime::deno_node::errors::ClosestPkgJsonError;
use deno_runtime::deno_node::NodeResolution;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_runtime::deno_node::NodeResolver;
use deno_runtime::deno_node::NpmResolver;
use deno_runtime::deno_node::PackageJson;
use deno_runtime::fs_util::specifier_to_file_path;
use deno_semver::jsr::JsrPackageReqReference;
@ -343,11 +346,29 @@ impl LspResolver {
}
pub fn in_node_modules(&self, specifier: &ModuleSpecifier) -> bool {
let resolver = self.get_scope_resolver(Some(specifier));
if let Some(npm_resolver) = &resolver.npm_resolver {
return npm_resolver.in_npm_package(specifier);
fn has_node_modules_dir(specifier: &ModuleSpecifier) -> bool {
// consider any /node_modules/ directory as being in the node_modules
// folder for the LSP because it's pretty complicated to deal with multiple scopes
specifier.scheme() == "file"
&& specifier
.path()
.to_ascii_lowercase()
.contains("/node_modules/")
}
false
let global_npm_resolver = self
.get_scope_resolver(Some(specifier))
.npm_resolver
.as_ref()
.and_then(|npm_resolver| npm_resolver.as_managed())
.filter(|r| r.root_node_modules_path().is_none());
if let Some(npm_resolver) = &global_npm_resolver {
if npm_resolver.in_npm_package(specifier) {
return true;
}
}
has_node_modules_dir(specifier)
}
pub fn node_media_type(
@ -422,20 +443,17 @@ async fn create_npm_resolver(
cache: &LspCache,
http_client_provider: &Arc<HttpClientProvider>,
) -> Option<Arc<dyn CliNpmResolver>> {
let mut byonm_dir = None;
if let Some(config_data) = config_data {
if config_data.byonm {
byonm_dir = Some(config_data.node_modules_dir.clone().or_else(|| {
specifier_to_file_path(&config_data.scope)
.ok()
.map(|p| p.join("node_modules/"))
})?)
}
}
let options = if let Some(byonm_dir) = byonm_dir {
let enable_byonm = config_data.map(|d| d.byonm).unwrap_or(*DENO_FUTURE);
let options = if enable_byonm {
CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
fs: Arc::new(deno_fs::RealFs),
root_node_modules_dir: byonm_dir,
root_node_modules_dir: config_data.and_then(|config_data| {
config_data.node_modules_dir.clone().or_else(|| {
specifier_to_file_path(&config_data.scope)
.ok()
.map(|p| p.join("node_modules/"))
})
}),
})
} else {
CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
@ -478,6 +496,13 @@ async fn create_npm_resolver(
fn create_node_resolver(
npm_resolver: Option<&Arc<dyn CliNpmResolver>>,
) -> Option<Arc<CliNodeResolver>> {
use once_cell::sync::Lazy;
// it's not ideal to share this across all scopes and to
// never clear it, but it's fine for the time being
static CJS_RESOLUTIONS: Lazy<Arc<CjsResolutionStore>> =
Lazy::new(Default::default);
let npm_resolver = npm_resolver?;
let fs = Arc::new(deno_fs::RealFs);
let node_resolver_inner = Arc::new(NodeResolver::new(
@ -485,7 +510,7 @@ fn create_node_resolver(
npm_resolver.clone().into_npm_resolver(),
));
Some(Arc::new(CliNodeResolver::new(
None,
CJS_RESOLUTIONS.clone(),
fs,
node_resolver_inner,
npm_resolver.clone(),

View file

@ -4347,9 +4347,17 @@ fn op_release(
fn op_resolve(
state: &mut OpState,
#[string] base: String,
is_base_cjs: bool,
#[serde] specifiers: Vec<String>,
) -> Result<Vec<Option<(String, String)>>, AnyError> {
op_resolve_inner(state, ResolveArgs { base, specifiers })
op_resolve_inner(
state,
ResolveArgs {
base,
is_base_cjs,
specifiers,
},
)
}
struct TscRequestArray {
@ -6287,6 +6295,7 @@ mod tests {
&mut state,
ResolveArgs {
base: "file:///a.ts".to_string(),
is_base_cjs: false,
specifiers: vec!["./b.ts".to_string()],
},
)

View file

@ -29,7 +29,8 @@ use super::InnerCliNpmResolverRef;
pub struct CliNpmResolverByonmCreateOptions {
pub fs: Arc<dyn FileSystem>,
pub root_node_modules_dir: PathBuf,
// todo(dsherret): investigate removing this
pub root_node_modules_dir: Option<PathBuf>,
}
pub fn create_byonm_npm_resolver(
@ -44,7 +45,7 @@ pub fn create_byonm_npm_resolver(
#[derive(Debug)]
pub struct ByonmCliNpmResolver {
fs: Arc<dyn FileSystem>,
root_node_modules_dir: PathBuf,
root_node_modules_dir: Option<PathBuf>,
}
impl ByonmCliNpmResolver {
@ -131,16 +132,16 @@ impl ByonmCliNpmResolver {
}
// otherwise, fall fallback to the project's package.json
let root_pkg_json_path = self
.root_node_modules_dir
.parent()
.unwrap()
.join("package.json");
if let Some(pkg_json) =
load_pkg_json(self.fs.as_ref(), &root_pkg_json_path)?
{
if let Some(alias) = resolve_alias_from_pkg_json(req, pkg_json.as_ref()) {
return Ok((pkg_json, alias));
if let Some(root_node_modules_dir) = &self.root_node_modules_dir {
let root_pkg_json_path =
root_node_modules_dir.parent().unwrap().join("package.json");
if let Some(pkg_json) =
load_pkg_json(self.fs.as_ref(), &root_pkg_json_path)?
{
if let Some(alias) = resolve_alias_from_pkg_json(req, pkg_json.as_ref())
{
return Ok((pkg_json, alias));
}
}
}
@ -159,9 +160,10 @@ impl NpmResolver for ByonmCliNpmResolver {
fn get_npm_process_state(&self) -> String {
serde_json::to_string(&NpmProcessState {
kind: NpmProcessStateKind::Byonm,
local_node_modules_path: Some(
self.root_node_modules_dir.to_string_lossy().to_string(),
),
local_node_modules_path: self
.root_node_modules_dir
.as_ref()
.map(|p| p.to_string_lossy().to_string()),
})
.unwrap()
}
@ -256,7 +258,7 @@ impl CliNpmResolver for ByonmCliNpmResolver {
}
fn root_node_modules_path(&self) -> Option<&PathBuf> {
Some(&self.root_node_modules_dir)
self.root_node_modules_dir.as_ref()
}
fn resolve_pkg_folder_from_deno_module_req(

View file

@ -27,6 +27,7 @@ use deno_runtime::deno_node::errors::ClosestPkgJsonError;
use deno_runtime::deno_node::errors::UrlToNodeResolutionError;
use deno_runtime::deno_node::is_builtin_node_module;
use deno_runtime::deno_node::parse_npm_pkg_name;
use deno_runtime::deno_node::NodeModuleKind;
use deno_runtime::deno_node::NodeResolution;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_runtime::deno_node::NodeResolver;
@ -66,8 +67,7 @@ pub struct ModuleCodeStringSource {
#[derive(Debug)]
pub struct CliNodeResolver {
// not used in the LSP
cjs_resolutions: Option<Arc<CjsResolutionStore>>,
cjs_resolutions: Arc<CjsResolutionStore>,
fs: Arc<dyn deno_fs::FileSystem>,
node_resolver: Arc<NodeResolver>,
// todo(dsherret): remove this pub(crate)
@ -76,7 +76,7 @@ pub struct CliNodeResolver {
impl CliNodeResolver {
pub fn new(
cjs_resolutions: Option<Arc<CjsResolutionStore>>,
cjs_resolutions: Arc<CjsResolutionStore>,
fs: Arc<dyn deno_fs::FileSystem>,
node_resolver: Arc<NodeResolver>,
npm_resolver: Arc<dyn CliNpmResolver>,
@ -120,10 +120,16 @@ impl CliNodeResolver {
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<Option<NodeResolution>, AnyError> {
let referrer_kind = if self.cjs_resolutions.contains(referrer) {
NodeModuleKind::Cjs
} else {
NodeModuleKind::Esm
};
self.handle_node_resolve_result(
self
.node_resolver
.resolve(specifier, referrer, mode)
.resolve(specifier, referrer, referrer_kind, mode)
.map_err(AnyError::from),
)
}
@ -241,16 +247,12 @@ impl CliNodeResolver {
let specifier =
crate::node::resolve_specifier_into_node_modules(&specifier);
if self.in_npm_package(&specifier) {
if let Some(cjs_resolutions) = &self.cjs_resolutions {
let resolution =
self.node_resolver.url_to_node_resolution(specifier)?;
if let NodeResolution::CommonJs(specifier) = &resolution {
cjs_resolutions.insert(specifier.clone());
}
return Ok(resolution.into_url());
} else {
return Ok(specifier);
let resolution =
self.node_resolver.url_to_node_resolution(specifier)?;
if let NodeResolution::CommonJs(specifier) = &resolution {
self.cjs_resolutions.insert(specifier.clone());
}
return Ok(resolution.into_url());
}
}
@ -272,9 +274,7 @@ impl CliNodeResolver {
Some(response) => {
if let NodeResolution::CommonJs(specifier) = &response {
// remember that this was a common js resolution
if let Some(cjs_resolutions) = &self.cjs_resolutions {
cjs_resolutions.insert(specifier.clone());
}
self.cjs_resolutions.insert(specifier.clone());
}
Ok(Some(response))
}

View file

@ -69,7 +69,7 @@ pub enum NodeModules {
node_modules_dir: Option<String>,
},
Byonm {
root_node_modules_dir: String,
root_node_modules_dir: Option<String>,
},
}
@ -572,15 +572,16 @@ impl<'a> DenoCompileBinaryWriter<'a> {
Some(root_dir),
files,
Some(NodeModules::Byonm {
root_node_modules_dir: root_dir_url
.specifier_key(
&ModuleSpecifier::from_directory_path(
// will always be set for byonm
resolver.root_node_modules_path().unwrap(),
)
.unwrap(),
)
.into_owned(),
root_node_modules_dir: resolver.root_node_modules_path().map(
|node_modules_dir| {
root_dir_url
.specifier_key(
&ModuleSpecifier::from_directory_path(node_modules_dir)
.unwrap(),
)
.into_owned()
},
),
}),
)
}

View file

@ -490,7 +490,8 @@ pub async fn run(
let vfs_root_dir_path = root_path.clone();
let vfs = load_npm_vfs(vfs_root_dir_path.clone())
.context("Failed to load vfs.")?;
let root_node_modules_dir = vfs.root().join(root_node_modules_dir);
let root_node_modules_dir =
root_node_modules_dir.map(|p| vfs.root().join(p));
let fs = Arc::new(DenoCompileFileSystem::new(vfs))
as Arc<dyn deno_fs::FileSystem>;
let npm_resolver = create_cli_npm_resolver(
@ -584,7 +585,7 @@ pub async fn run(
)
};
let cli_node_resolver = Arc::new(CliNodeResolver::new(
Some(cjs_resolutions.clone()),
cjs_resolutions.clone(),
fs.clone(),
node_resolver.clone(),
npm_resolver.clone(),

View file

@ -731,6 +731,7 @@ delete Object.prototype.__proto__;
/** @type {[string, ts.Extension] | undefined} */
const resolved = ops.op_resolve(
containingFilePath,
isCjsCache.has(containingFilePath),
[fileReference.fileName],
)?.[0];
if (resolved) {
@ -764,6 +765,7 @@ delete Object.prototype.__proto__;
/** @type {Array<[string, ts.Extension] | undefined>} */
const resolved = ops.op_resolve(
base,
isCjsCache.has(base),
specifiers,
);
if (resolved) {

View file

@ -30,6 +30,7 @@ use deno_graph::GraphKind;
use deno_graph::Module;
use deno_graph::ModuleGraph;
use deno_graph::ResolutionResolved;
use deno_runtime::deno_node::NodeModuleKind;
use deno_runtime::deno_node::NodeResolution;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_runtime::deno_node::NodeResolver;
@ -585,6 +586,8 @@ pub struct ResolveArgs {
/// The base specifier that the supplied specifier strings should be resolved
/// relative to.
pub base: String,
/// If the base is cjs.
pub is_base_cjs: bool,
/// A list of specifiers that should be resolved.
pub specifiers: Vec<String>,
}
@ -594,9 +597,17 @@ pub struct ResolveArgs {
fn op_resolve(
state: &mut OpState,
#[string] base: String,
is_base_cjs: bool,
#[serde] specifiers: Vec<String>,
) -> Result<Vec<(String, String)>, AnyError> {
op_resolve_inner(state, ResolveArgs { base, specifiers })
op_resolve_inner(
state,
ResolveArgs {
base,
is_base_cjs,
specifiers,
},
)
}
#[inline]
@ -607,6 +618,11 @@ fn op_resolve_inner(
let state = state.borrow_mut::<State>();
let mut resolved: Vec<(String, String)> =
Vec::with_capacity(args.specifiers.len());
let referrer_kind = if args.is_base_cjs {
NodeModuleKind::Cjs
} else {
NodeModuleKind::Esm
};
let referrer = if let Some(remapped_specifier) =
state.remapped_specifiers.get(&args.base)
{
@ -646,7 +662,12 @@ fn op_resolve_inner(
Some(ResolutionResolved { specifier, .. }) => {
resolve_graph_specifier_types(specifier, &referrer, state)?
}
_ => resolve_non_graph_specifier_types(&specifier, &referrer, state)?,
_ => resolve_non_graph_specifier_types(
&specifier,
&referrer,
referrer_kind,
state,
)?,
};
let result = match maybe_result {
Some((specifier, media_type)) => {
@ -766,6 +787,7 @@ fn resolve_graph_specifier_types(
fn resolve_non_graph_specifier_types(
specifier: &str,
referrer: &ModuleSpecifier,
referrer_kind: NodeModuleKind,
state: &State,
) -> Result<Option<(ModuleSpecifier, MediaType)>, AnyError> {
let npm = match state.maybe_npm.as_ref() {
@ -777,11 +799,17 @@ fn resolve_non_graph_specifier_types(
// we're in an npm package, so use node resolution
Ok(Some(NodeResolution::into_specifier_and_media_type(
node_resolver
.resolve(specifier, referrer, NodeResolutionMode::Types)
.resolve(
specifier,
referrer,
referrer_kind,
NodeResolutionMode::Types,
)
.ok()
.flatten(),
)))
} else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(specifier) {
debug_assert_eq!(referrer_kind, NodeModuleKind::Esm);
// todo(dsherret): add support for injecting this in the graph so
// we don't need this special code here.
// This could occur when resolving npm:@types/node when it is
@ -1184,6 +1212,7 @@ mod tests {
&mut state,
ResolveArgs {
base: "https://deno.land/x/a.ts".to_string(),
is_base_cjs: false,
specifiers: vec!["./b.ts".to_string()],
},
)
@ -1206,6 +1235,7 @@ mod tests {
&mut state,
ResolveArgs {
base: "https://deno.land/x/a.ts".to_string(),
is_base_cjs: false,
specifiers: vec!["./bad.ts".to_string()],
},
)

View file

@ -172,6 +172,7 @@ impl NodeResolver {
&self,
specifier: &str,
referrer: &ModuleSpecifier,
referrer_kind: NodeModuleKind,
mode: NodeResolutionMode,
) -> Result<Option<NodeResolution>, NodeResolveError> {
// Note: if we are here, then the referrer is an esm module
@ -212,8 +213,16 @@ impl NodeResolver {
}
}
let maybe_url =
self.module_resolve(specifier, referrer, DEFAULT_CONDITIONS, mode)?;
let maybe_url = self.module_resolve(
specifier,
referrer,
referrer_kind,
match referrer_kind {
NodeModuleKind::Esm => DEFAULT_CONDITIONS,
NodeModuleKind::Cjs => REQUIRE_CONDITIONS,
},
mode,
)?;
let url = match maybe_url {
Some(url) => url,
None => return Ok(None),
@ -229,6 +238,7 @@ impl NodeResolver {
&self,
specifier: &str,
referrer: &ModuleSpecifier,
referrer_kind: NodeModuleKind,
conditions: &[&str],
mode: NodeResolutionMode,
) -> Result<Option<ModuleSpecifier>, NodeResolveError> {
@ -250,7 +260,7 @@ impl NodeResolver {
Some(self.package_imports_resolve(
specifier,
Some(referrer),
NodeModuleKind::Esm,
referrer_kind,
pkg_config.as_deref(),
conditions,
mode,
@ -261,7 +271,7 @@ impl NodeResolver {
self.package_resolve(
specifier,
referrer,
NodeModuleKind::Esm,
referrer_kind,
conditions,
mode,
)?
@ -273,13 +283,7 @@ impl NodeResolver {
let maybe_url = if mode.is_types() {
let file_path = to_file_path(&url);
// todo(16370): the referrer module kind is not correct here. I think we need
// typescript to tell us if the referrer is esm or cjs
self.path_to_declaration_url(
file_path,
Some(referrer),
NodeModuleKind::Esm,
)?
self.path_to_declaration_url(file_path, Some(referrer), referrer_kind)?
} else {
Some(url)
};
@ -1319,13 +1323,7 @@ impl NodeResolver {
source: source.into_io_error(),
},
)?);
let mut current_dir = current_dir.as_path();
let package_json_path = current_dir.join("package.json");
if let Some(pkg_json) = self.load_package_json(&package_json_path)? {
return Ok(Some(pkg_json));
}
while let Some(parent) = current_dir.parent() {
current_dir = parent;
for current_dir in current_dir.ancestors() {
let package_json_path = current_dir.join("package.json");
if let Some(pkg_json) = self.load_package_json(&package_json_path)? {
return Ok(Some(pkg_json));

View file

@ -14500,6 +14500,55 @@ fn lsp_cjs_internal_types_default_export() {
assert_eq!(json!(diagnostics.all()), json!([]));
}
#[test]
fn lsp_cjs_import_dual() {
let context = TestContextBuilder::new()
.use_http_server()
.use_temp_cwd()
.add_npm_env_vars()
.env("DENO_FUTURE", "1")
.build();
let temp_dir = context.temp_dir();
temp_dir.write("deno.json", r#"{}"#);
temp_dir.write(
"package.json",
r#"{
"dependencies": {
"@denotest/cjs-import-dual": "1"
}
}"#,
);
context.run_npm("install");
let mut client = context.new_lsp_command().build();
client.initialize_default();
let main_url = temp_dir.path().join("main.ts").uri_file();
let diagnostics = client.did_open(
json!({
"textDocument": {
"uri": main_url,
"languageId": "typescript",
"version": 1,
// getKind() should resolve as "cjs" and cause a type checker error
"text": "import { getKind } from 'npm:@denotest/cjs-import-dual@1';\nconst kind: 'esm' = getKind(); console.log(kind);",
}
}),
);
assert_eq!(
json!(diagnostics.all()),
json!([{
"range": {
"start": { "line": 1, "character": 6, },
"end": { "line": 1, "character": 10, },
},
"severity": 1,
"code": 2322,
"source": "deno-ts",
"message": "Type '\"cjs\"' is not assignable to type '\"esm\"'.",
}])
);
}
#[test]
fn lsp_ts_code_fix_any_param() {
let context = TestContextBuilder::new().use_temp_cwd().build();

View file

@ -0,0 +1,2 @@
// it should pick up the cjs types
export { getKind } from "@denotest/dual-cjs-esm";

View file

@ -0,0 +1 @@
module.exports.getKind = require("@denotest/dual-cjs-esm").getKind;

View file

@ -0,0 +1,7 @@
{
"name": "@denotest/cjs-import-dual",
"version": "1.0.0",
"dependencies": {
"@denotest/dual-cjs-esm": "1"
}
}

View file

@ -1 +1 @@
export function getKind(): string;
export function getKind(): "cjs";

View file

@ -1 +1 @@
export function getKind(): string;
export function getKind(): "esm";

View file

@ -0,0 +1,13 @@
{
"tests": {
"run": {
"args": "run main.ts",
"output": "run.out"
},
"check": {
"args": "check --all main.ts",
"exitCode": 1,
"output": "check.out"
}
}
}

View file

@ -0,0 +1,9 @@
Download http://localhost:4260/@denotest/cjs-import-dual
Download http://localhost:4260/@denotest/dual-cjs-esm
Download http://localhost:4260/@denotest/cjs-import-dual/1.0.0.tgz
Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz
Check file:///[WILDLINE]/cjs_import_dual/main.ts
error: TS2322 [ERROR]: Type '"cjs"' is not assignable to type '"esm"'.
const kind: "esm" = getKind(); // should cause a type error
~~~~
at file:///[WILDLINE]/cjs_import_dual/main.ts:3:7

View file

@ -0,0 +1,4 @@
import { getKind } from "npm:@denotest/cjs-import-dual@1";
const kind: "esm" = getKind(); // should cause a type error
console.log(kind);

View file

@ -0,0 +1,5 @@
Download http://localhost:4260/@denotest/cjs-import-dual
Download http://localhost:4260/@denotest/dual-cjs-esm
Download http://localhost:4260/@denotest/cjs-import-dual/1.0.0.tgz
Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz
cjs