Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(install/global): do not error if path is an npm pkg and relative file #26975

Merged
merged 2 commits into from
Nov 21, 2024
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
31 changes: 17 additions & 14 deletions cli/tools/installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ async fn install_global(
// ensure the module is cached
let factory = CliFactory::from_flags(flags.clone());

let cli_options = factory.cli_options()?;
let http_client = factory.http_client_provider();
let deps_http_cache = factory.global_http_cache()?;
let mut deps_file_fetcher = FileFetcher::new(
Expand All @@ -381,20 +382,22 @@ async fn install_global(
));

let entry_text = install_flags_global.module_url.as_str();
let req = super::registry::AddRmPackageReq::parse(entry_text);

// found a package requirement but missing the prefix
if let Ok(Err(package_req)) = req {
if jsr_resolver.req_to_nv(&package_req).await.is_some() {
bail!(
"{entry_text} is missing a prefix. Did you mean `{}`?",
crate::colors::yellow(format!("deno install -g jsr:{package_req}"))
);
} else if npm_resolver.req_to_nv(&package_req).await.is_some() {
bail!(
"{entry_text} is missing a prefix. Did you mean `{}`?",
crate::colors::yellow(format!("deno install -g npm:{package_req}"))
);
if !cli_options.initial_cwd().join(entry_text).exists() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this going to hit the file system twice when the file exists? That seems like a redundant file operation to me

Copy link
Member Author

@dsherret dsherret Nov 21, 2024

Choose a reason for hiding this comment

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

It's fine. There won't be any noticable perf impact from one file system stat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright

// check for package requirement missing prefix
if let Ok(Err(package_req)) =
super::registry::AddRmPackageReq::parse(entry_text)
{
if jsr_resolver.req_to_nv(&package_req).await.is_some() {
bail!(
"{entry_text} is missing a prefix. Did you mean `{}`?",
crate::colors::yellow(format!("deno install -g jsr:{package_req}"))
);
} else if npm_resolver.req_to_nv(&package_req).await.is_some() {
bail!(
"{entry_text} is missing a prefix. Did you mean `{}`?",
crate::colors::yellow(format!("deno install -g npm:{package_req}"))
);
}
}
}

Expand Down
32 changes: 32 additions & 0 deletions tests/registry/npm/cli.ts/registry.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"name": "cli.ts",
"description": "Package with name like a file path",
"dist-tags": {
"latest": "2.2.0"
},
"versions": {
"2.2.0": {
"name": "cli.ts",
"version": "2.2.0",
"description": "Package with name like a file path",
"license": "MIT",
"author": {},
"engines": {
"node": ">=6"
},
"gitHead": "91440c5a1615354fb9419354650937c434eb9f49",
"_id": "[email protected]",
"_nodeVersion": "10.16.0",
"_npmVersion": "6.9.0",
"dist": {
"integrity": "sha512-4diC9HaTE+KRAMWhDhrGOECgWZxoevMc5TlkObMqNSsVU62PYzXZ/SMTjzyGAFF1YusgxGcSWTEXBhp0CPwQ1A==",
"shasum": "ee8472dbb129e727b31e8a10a427dee9dfe4008b",
"tarball": "http://localhost:4260/cli.ts/cli.ts-2.2.0.tgz",
"fileCount": 5,
"unpackedSize": 5508
},
"directories": {},
"_hasShrinkwrap": false
}
}
}
20 changes: 20 additions & 0 deletions tests/specs/install/global/pkg_name_same_as_file/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"tempDir": true,
"tests": {
"file_exists": {
"steps": [{
"args": ["eval", "Deno.writeTextFileSync('cli.ts', '')"],
"output": "[WILDCARD]"
}, {
// success
"args": "install --root ./bins -g --name my-cli cli.ts",
"output": "[WILDCARD]"
}]
},
"file_not_exists": {
"args": "install --root ./bins -g --name my-cli cli.ts",
"output": "file_not_exists.out",
"exitCode": 1
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error: cli.ts is missing a prefix. Did you mean `deno install -g npm:cli.ts`?