From 552436938656f1aade722b2ac9f2051fa9c64e78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 22 Nov 2022 20:09:30 +0100 Subject: [PATCH] fix: Make npm packages works with import maps (#16754) Co-authored-by: David Sherret --- cli/npm/resolution/specifier.rs | 69 ++++++++++++++++++- .../testdata/npm/import_map/import_map.json | 3 +- cli/tests/testdata/npm/import_map/main.js | 3 + cli/tests/testdata/npm/import_map/main.out | 3 + cli/tsc/00_typescript.js | 8 ++- 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/cli/npm/resolution/specifier.rs b/cli/npm/resolution/specifier.rs index 1a1590a2f9..b832540bd9 100644 --- a/cli/npm/resolution/specifier.rs +++ b/cli/npm/resolution/specifier.rs @@ -32,8 +32,12 @@ impl NpmPackageReference { } pub fn from_str(specifier: &str) -> Result { + let original_text = specifier; let specifier = match specifier.strip_prefix("npm:") { - Some(s) => s, + Some(s) => { + // Strip leading slash, which might come from import map + s.strip_prefix('/').unwrap_or(s) + } None => { // don't allocate a string here and instead use a static string // because this is hit a lot when a url is not an npm specifier @@ -65,7 +69,12 @@ impl NpmPackageReference { let sub_path = if parts.len() == name_parts.len() { None } else { - Some(parts[name_part_len..].join("/")) + let sub_path = parts[name_part_len..].join("/"); + if sub_path.is_empty() { + None + } else { + Some(sub_path) + } }; if let Some(sub_path) = &sub_path { @@ -79,6 +88,14 @@ impl NpmPackageReference { } } + if name.is_empty() { + let msg = format!( + "Invalid npm specifier '{}'. Did not contain a package name.", + original_text + ); + return Err(generic_error(msg)); + } + Ok(NpmPackageReference { req: NpmPackageReq { name, version_req }, sub_path, @@ -631,6 +648,54 @@ mod tests { .to_string(), "Not a valid package: @package" ); + + // should parse leading slash + assert_eq!( + NpmPackageReference::from_str("npm:/@package/test/sub_path").unwrap(), + NpmPackageReference { + req: NpmPackageReq { + name: "@package/test".to_string(), + version_req: None, + }, + sub_path: Some("sub_path".to_string()), + } + ); + assert_eq!( + NpmPackageReference::from_str("npm:/test").unwrap(), + NpmPackageReference { + req: NpmPackageReq { + name: "test".to_string(), + version_req: None, + }, + sub_path: None, + } + ); + assert_eq!( + NpmPackageReference::from_str("npm:/test/").unwrap(), + NpmPackageReference { + req: NpmPackageReq { + name: "test".to_string(), + version_req: None, + }, + sub_path: None, + } + ); + + // should error for no name + assert_eq!( + NpmPackageReference::from_str("npm:/") + .err() + .unwrap() + .to_string(), + "Invalid npm specifier 'npm:/'. Did not contain a package name." + ); + assert_eq!( + NpmPackageReference::from_str("npm://test") + .err() + .unwrap() + .to_string(), + "Invalid npm specifier 'npm://test'. Did not contain a package name." + ); } #[test] diff --git a/cli/tests/testdata/npm/import_map/import_map.json b/cli/tests/testdata/npm/import_map/import_map.json index a7ed13b82e..1c3baacd1e 100644 --- a/cli/tests/testdata/npm/import_map/import_map.json +++ b/cli/tests/testdata/npm/import_map/import_map.json @@ -1,5 +1,6 @@ { "imports": { - "chalk": "npm:chalk@5" + "chalk": "npm:chalk@5", + "@denotest/": "npm:/@denotest/dual-cjs-esm/" } } diff --git a/cli/tests/testdata/npm/import_map/main.js b/cli/tests/testdata/npm/import_map/main.js index fe7ef549aa..e354b7e92c 100644 --- a/cli/tests/testdata/npm/import_map/main.js +++ b/cli/tests/testdata/npm/import_map/main.js @@ -1,7 +1,10 @@ import chalk from "chalk"; +import { getSubPathKind } from "@denotest/subpath/main.mjs"; console.log(chalk.green("chalk import map loads")); export function test(value) { return chalk.red(value); } + +console.log(getSubPathKind()); diff --git a/cli/tests/testdata/npm/import_map/main.out b/cli/tests/testdata/npm/import_map/main.out index ef3f4e22b2..b5b67651ef 100644 --- a/cli/tests/testdata/npm/import_map/main.out +++ b/cli/tests/testdata/npm/import_map/main.out @@ -1,3 +1,6 @@ +Download http://localhost:4545/npm/registry/@denotest/dual-cjs-esm Download http://localhost:4545/npm/registry/chalk +Download http://localhost:4545/npm/registry/@denotest/dual-cjs-esm/1.0.0.tgz Download http://localhost:4545/npm/registry/chalk/chalk-5.0.1.tgz chalk import map loads +esm diff --git a/cli/tsc/00_typescript.js b/cli/tsc/00_typescript.js index 01377c8b5d..4f2a0c4675 100644 --- a/cli/tsc/00_typescript.js +++ b/cli/tsc/00_typescript.js @@ -90846,7 +90846,7 @@ var ts; if (!text.startsWith("npm:")) { throw new Error("Not an npm specifier: ".concat(text)); } - text = text.replace(/^npm:/, ""); + text = text.replace(/^npm:\/?/, ""); var parts = text.split("/"); var namePartLen = text.startsWith("@") ? 2 : 1; if (parts.length < namePartLen) { @@ -90860,8 +90860,12 @@ var ts; versionReq = lastNamePart.substring(lastAtIndex + 1); nameParts[nameParts.length - 1] = lastNamePart.substring(0, lastAtIndex); } + var name = nameParts.join("/"); + if (name.length === 0) { + throw new Error("Npm specifier did not have a name: ".concat(text)); + } return { - name: nameParts.join("/"), + name: name, versionReq: versionReq, subPath: parts.length > nameParts.length ? parts.slice(nameParts.length).join("/") : undefined, };