Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(info): error instead of panic for npm specifiers when using byonm #25947

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1144,10 +1144,6 @@ impl CliOptions {
DenoSubcommand::Run(run_flags) => {
if run_flags.is_stdin() {
resolve_url_or_path("./$deno$stdin.ts", self.initial_cwd())?
} else if NpmPackageReqReference::from_str(&run_flags.script)
.is_ok()
{
ModuleSpecifier::parse(&run_flags.script)?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was useless. It's done in the else condition.

} else {
resolve_url_or_path(&run_flags.script, self.initial_cwd())?
}
Expand Down
7 changes: 7 additions & 0 deletions cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::tools::check::TypeChecker;
use crate::util::file_watcher::WatcherCommunicator;
use crate::util::fs::canonicalize_path;
use deno_config::workspace::JsrPackageConfig;
use deno_core::anyhow::bail;
use deno_graph::source::LoaderChecksum;
use deno_graph::FillFromLockfileOptions;
use deno_graph::JsrLoadError;
Expand Down Expand Up @@ -593,6 +594,12 @@ impl ModuleGraphBuilder {
let initial_package_deps_len = graph.packages.package_deps_sum();
let initial_package_mappings_len = graph.packages.mappings().len();

if roots.iter().any(|r| r.scheme() == "npm")
&& self.npm_resolver.as_byonm().is_some()
{
bail!("Resolving npm specifier entrypoints this way is currently not supported with \"nodeModules\": \"manual\". In the meantime, try with --node-modules-dir=auto instead");
}

graph.build(roots, loader, options).await;

let has_redirects_changed = graph.redirects.len() != initial_redirects_len;
Expand Down
11 changes: 11 additions & 0 deletions tests/specs/info/byonm/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"tempDir": true,
"steps": [{
"args": "install",
"output": "[WILDCARD]"
}, {
"args": "info npm:@denotest/add",
"output": "info.out",
"exitCode": 1
}]
}
6 changes: 6 additions & 0 deletions tests/specs/info/byonm/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"nodeModulesDir": "manual",
"imports": {
"chalk": "npm:@denotest/add"
}
}
1 change: 1 addition & 0 deletions tests/specs/info/byonm/info.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error: Resolving npm specifier entrypoints this way is currently not supported with "nodeModules": "manual". In the meantime, try with --node-modules-dir=auto instead
5 changes: 5 additions & 0 deletions tests/specs/info/byonm/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"@denotest/add": "*"
}
}