From 655464dcedded147d81959b12ca9565fd409a511 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Wed, 6 Nov 2024 22:15:31 +0100 Subject: [PATCH 1/7] fix: ban cjs and cts file extensions --- api/src/analysis.rs | 42 +++++++++++++++++++++- api/src/gcp.rs | 8 ++++- api/src/publish.rs | 10 ++++++ api/src/tarball.rs | 14 ++++++++ api/testdata/tarballs/cjs_import/jsr.json | 5 +++ api/testdata/tarballs/cjs_import/mod.ts | 3 ++ api/testdata/tarballs/cjs_import/other.cjs | 1 + 7 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 api/testdata/tarballs/cjs_import/jsr.json create mode 100644 api/testdata/tarballs/cjs_import/mod.ts create mode 100644 api/testdata/tarballs/cjs_import/other.cjs diff --git a/api/src/analysis.rs b/api/src/analysis.rs index c8d5b833..edc8b2fe 100644 --- a/api/src/analysis.rs +++ b/api/src/analysis.rs @@ -186,6 +186,7 @@ async fn analyze_package_inner( .analyzer .get_parsed_source(module.specifier()) { + check_for_banned_extensions(&parsed_source)?; check_for_banned_syntax(&parsed_source)?; check_for_banned_triple_slash_directives(&parsed_source)?; } @@ -802,6 +803,18 @@ fn collect_dependencies( Ok(dependencies) } +fn check_for_banned_extensions( + parsed_source: &ParsedSource, +) -> Result<(), PublishError> { + match parsed_source.media_type() { + deno_ast::MediaType::Cjs | deno_ast::MediaType::Cts => { + return Err(PublishError::BannedCommonJsExtension { specifier: parsed_source.specifier().to_string() } ); + }, + _ => { + return Ok(()) + } + }} + fn check_for_banned_syntax( parsed_source: &ParsedSource, ) -> Result<(), PublishError> { @@ -957,8 +970,15 @@ fn check_for_banned_triple_slash_directives( #[cfg(test)] mod tests { fn parse(source: &str) -> deno_ast::ParsedSource { - let specifier = deno_ast::ModuleSpecifier::parse("file:///mod.ts").unwrap(); let media_type = deno_ast::MediaType::TypeScript; + parse_with_media_type(source, media_type) + } + + fn parse_with_media_type( + source: &str, + media_type: deno_ast::MediaType, + ) -> deno_ast::ParsedSource { + let specifier = deno_ast::ModuleSpecifier::parse("file:///mod.ts").unwrap(); deno_ast::parse_module(deno_ast::ParseParams { specifier, text: source.into(), @@ -970,6 +990,26 @@ mod tests { .unwrap() } + #[test] + fn banned_extensions() { + let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::TypeScript); + assert!(super::check_for_banned_extensions(&x).is_ok()); + + let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::Cjs); + let err = super::check_for_banned_extensions(&x).unwrap_err(); + assert!( + matches!(err, super::PublishError::BannedCommonJsExtension { .. }), + "{err:?}", + ); + + let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::Cts); + let err = super::check_for_banned_extensions(&x).unwrap_err(); + assert!( + matches!(err, super::PublishError::BannedCommonJsExtension { .. }), + "{err:?}", + ); + } + #[test] fn banned_triple_slash_directives() { let x = parse("let x = 1;"); diff --git a/api/src/gcp.rs b/api/src/gcp.rs index c4415b68..34d14b6c 100644 --- a/api/src/gcp.rs +++ b/api/src/gcp.rs @@ -573,12 +573,18 @@ impl FakeGcsTester { assert!(self.proc.is_none()); - #[cfg(target_os = "macos")] + #[cfg(all(target_os = "macos", target_arch = "aarch64"))] let p = concat!( env!("CARGO_MANIFEST_DIR"), "/../tools/bin/darwin-arm64/fake-gcs-server" ); + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + let p = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../tools/bin/darwin-amd64/fake-gcs-server" + ); + #[cfg(target_os = "linux")] let p = concat!( env!("CARGO_MANIFEST_DIR"), diff --git a/api/src/publish.rs b/api/src/publish.rs index eab4b97f..06a19477 100644 --- a/api/src/publish.rs +++ b/api/src/publish.rs @@ -1308,6 +1308,16 @@ pub mod tests { assert_eq!(task.status, PublishingTaskStatus::Success, "{task:#?}"); } + #[tokio::test] + async fn cjs_import() { + let t = TestSetup::new().await; + let bytes = create_mock_tarball("cjs_import"); + let task = process_tarball_setup(&t, bytes).await; + assert_eq!(task.status, PublishingTaskStatus::Failure, "{task:#?}"); + let error = task.error.unwrap(); + assert_eq!(error.code, "bannedCommonJsExtension"); + } + #[tokio::test] async fn npm_tarball() { let t = TestSetup::new().await; diff --git a/api/src/tarball.rs b/api/src/tarball.rs index 58238c49..e03b1422 100644 --- a/api/src/tarball.rs +++ b/api/src/tarball.rs @@ -158,6 +158,12 @@ pub async fn process_tarball( }); } + if path.ends_with(".cjs") | path.ends_with(".cts") { + return Err(PublishError::BannedCommonJsExtension { + specifier: path.to_string(), + }); + } + let size = header.size().map_err(PublishError::UntarError)?; if size > max_file_size { return Err(PublishError::FileTooLarge { @@ -517,6 +523,11 @@ pub enum PublishError { column: usize, }, + #[error("CommonJS is not allowed {specifier}")] + BannedCommonJsExtension { + specifier: String, + }, + #[error("triple slash directives that modify globals (for example, '/// ' or '/// ') are not allowed. Instead instruct the user of your package to specify these directives. {specifier}:{line}:{column}")] BannedTripleSlashDirectives { specifier: String, @@ -636,6 +647,9 @@ impl PublishError { Some("globalTypeAugmentation") } PublishError::CommonJs { .. } => Some("commonJs"), + PublishError::BannedCommonJsExtension { .. } => { + Some("bannedCommonJsExtension") + }, PublishError::BannedTripleSlashDirectives { .. } => { Some("bannedTripleSlashDirectives") } diff --git a/api/testdata/tarballs/cjs_import/jsr.json b/api/testdata/tarballs/cjs_import/jsr.json new file mode 100644 index 00000000..94c22c0e --- /dev/null +++ b/api/testdata/tarballs/cjs_import/jsr.json @@ -0,0 +1,5 @@ +{ + "name": "@scope/foo", + "version": "1.2.3", + "exports": "./mod.ts" +} diff --git a/api/testdata/tarballs/cjs_import/mod.ts b/api/testdata/tarballs/cjs_import/mod.ts new file mode 100644 index 00000000..5455679a --- /dev/null +++ b/api/testdata/tarballs/cjs_import/mod.ts @@ -0,0 +1,3 @@ +import { test } from "./other.cjs"; // bad + +export const hello = `Hello, ${test}!`; diff --git a/api/testdata/tarballs/cjs_import/other.cjs b/api/testdata/tarballs/cjs_import/other.cjs new file mode 100644 index 00000000..e89508f4 --- /dev/null +++ b/api/testdata/tarballs/cjs_import/other.cjs @@ -0,0 +1 @@ +exports.test = "test"; From 9612dd374e79391561c0832dc51bfe8015848c58 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Wed, 6 Nov 2024 22:45:03 +0100 Subject: [PATCH 2/7] chore: lint --- api/src/analysis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/analysis.rs b/api/src/analysis.rs index edc8b2fe..13500cf7 100644 --- a/api/src/analysis.rs +++ b/api/src/analysis.rs @@ -811,7 +811,7 @@ fn check_for_banned_extensions( return Err(PublishError::BannedCommonJsExtension { specifier: parsed_source.specifier().to_string() } ); }, _ => { - return Ok(()) + Ok(()) } }} From 0f760b8f18ee07eedcbf482ab47220942b3c5864 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Wed, 6 Nov 2024 23:37:24 +0100 Subject: [PATCH 3/7] chore: format --- api/src/analysis.rs | 14 ++++++++------ api/src/tarball.rs | 6 ++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/src/analysis.rs b/api/src/analysis.rs index 13500cf7..26633fe6 100644 --- a/api/src/analysis.rs +++ b/api/src/analysis.rs @@ -808,12 +808,13 @@ fn check_for_banned_extensions( ) -> Result<(), PublishError> { match parsed_source.media_type() { deno_ast::MediaType::Cjs | deno_ast::MediaType::Cts => { - return Err(PublishError::BannedCommonJsExtension { specifier: parsed_source.specifier().to_string() } ); - }, - _ => { - Ok(()) + return Err(PublishError::BannedCommonJsExtension { + specifier: parsed_source.specifier().to_string(), + }); } - }} + _ => Ok(()), + } +} fn check_for_banned_syntax( parsed_source: &ParsedSource, @@ -992,7 +993,8 @@ mod tests { #[test] fn banned_extensions() { - let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::TypeScript); + let x = + parse_with_media_type("let x = 1;", deno_ast::MediaType::TypeScript); assert!(super::check_for_banned_extensions(&x).is_ok()); let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::Cjs); diff --git a/api/src/tarball.rs b/api/src/tarball.rs index e03b1422..cd2cac3a 100644 --- a/api/src/tarball.rs +++ b/api/src/tarball.rs @@ -524,9 +524,7 @@ pub enum PublishError { }, #[error("CommonJS is not allowed {specifier}")] - BannedCommonJsExtension { - specifier: String, - }, + BannedCommonJsExtension { specifier: String }, #[error("triple slash directives that modify globals (for example, '/// ' or '/// ') are not allowed. Instead instruct the user of your package to specify these directives. {specifier}:{line}:{column}")] BannedTripleSlashDirectives { @@ -649,7 +647,7 @@ impl PublishError { PublishError::CommonJs { .. } => Some("commonJs"), PublishError::BannedCommonJsExtension { .. } => { Some("bannedCommonJsExtension") - }, + } PublishError::BannedTripleSlashDirectives { .. } => { Some("bannedTripleSlashDirectives") } From 9b553364c8e3b005ecc88c916d061768879f10a2 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Wed, 6 Nov 2024 23:57:40 +0100 Subject: [PATCH 4/7] chore: appease ts --- frontend/islands/GlobalSearch.tsx | 1 + frontend/routes/packages.tsx | 1 + 2 files changed, 2 insertions(+) diff --git a/frontend/islands/GlobalSearch.tsx b/frontend/islands/GlobalSearch.tsx index 96b484b7..56d56553 100644 --- a/frontend/islands/GlobalSearch.tsx +++ b/frontend/islands/GlobalSearch.tsx @@ -136,6 +136,7 @@ export function GlobalSearch( searchNRef.current.displayed = searchN; batch(() => { selectionIdx.value = -1; + // @ts-ignore orama's return type doesn't match our type suggestions.value = res?.hits.map((hit) => hit.document) ?? []; }); } else if (kind === "packages") { diff --git a/frontend/routes/packages.tsx b/frontend/routes/packages.tsx index a654746e..0666cc16 100644 --- a/frontend/routes/packages.tsx +++ b/frontend/routes/packages.tsx @@ -109,6 +109,7 @@ export const handler: Handlers = { } return ctx.render({ + // @ts-ignore orama's return type doesn't match our type packages, query: search, page, From 68db9e18af60c0d1a929279d70c5709e355632a5 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Fri, 8 Nov 2024 18:31:11 +0100 Subject: [PATCH 5/7] chore: implement review remarks --- api/src/analysis.rs | 8 +++++--- api/src/publish.rs | 10 ---------- api/src/tarball.rs | 12 ------------ api/testdata/tarballs/cjs_import/jsr.json | 5 ----- api/testdata/tarballs/cjs_import/mod.ts | 3 --- api/testdata/tarballs/cjs_import/other.cjs | 1 - frontend/docs/troubleshooting.md | 5 +++-- 7 files changed, 8 insertions(+), 36 deletions(-) delete mode 100644 api/testdata/tarballs/cjs_import/jsr.json delete mode 100644 api/testdata/tarballs/cjs_import/mod.ts delete mode 100644 api/testdata/tarballs/cjs_import/other.cjs diff --git a/api/src/analysis.rs b/api/src/analysis.rs index 26633fe6..0368d342 100644 --- a/api/src/analysis.rs +++ b/api/src/analysis.rs @@ -808,8 +808,10 @@ fn check_for_banned_extensions( ) -> Result<(), PublishError> { match parsed_source.media_type() { deno_ast::MediaType::Cjs | deno_ast::MediaType::Cts => { - return Err(PublishError::BannedCommonJsExtension { + return Err(PublishError::CommonJs { specifier: parsed_source.specifier().to_string(), + line: 0, + column: 0, }); } _ => Ok(()), @@ -1000,14 +1002,14 @@ mod tests { let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::Cjs); let err = super::check_for_banned_extensions(&x).unwrap_err(); assert!( - matches!(err, super::PublishError::BannedCommonJsExtension { .. }), + matches!(err, super::PublishError::CommonJs { .. }), "{err:?}", ); let x = parse_with_media_type("let x = 1;", deno_ast::MediaType::Cts); let err = super::check_for_banned_extensions(&x).unwrap_err(); assert!( - matches!(err, super::PublishError::BannedCommonJsExtension { .. }), + matches!(err, super::PublishError::CommonJs { .. }), "{err:?}", ); } diff --git a/api/src/publish.rs b/api/src/publish.rs index 06a19477..eab4b97f 100644 --- a/api/src/publish.rs +++ b/api/src/publish.rs @@ -1308,16 +1308,6 @@ pub mod tests { assert_eq!(task.status, PublishingTaskStatus::Success, "{task:#?}"); } - #[tokio::test] - async fn cjs_import() { - let t = TestSetup::new().await; - let bytes = create_mock_tarball("cjs_import"); - let task = process_tarball_setup(&t, bytes).await; - assert_eq!(task.status, PublishingTaskStatus::Failure, "{task:#?}"); - let error = task.error.unwrap(); - assert_eq!(error.code, "bannedCommonJsExtension"); - } - #[tokio::test] async fn npm_tarball() { let t = TestSetup::new().await; diff --git a/api/src/tarball.rs b/api/src/tarball.rs index cd2cac3a..58238c49 100644 --- a/api/src/tarball.rs +++ b/api/src/tarball.rs @@ -158,12 +158,6 @@ pub async fn process_tarball( }); } - if path.ends_with(".cjs") | path.ends_with(".cts") { - return Err(PublishError::BannedCommonJsExtension { - specifier: path.to_string(), - }); - } - let size = header.size().map_err(PublishError::UntarError)?; if size > max_file_size { return Err(PublishError::FileTooLarge { @@ -523,9 +517,6 @@ pub enum PublishError { column: usize, }, - #[error("CommonJS is not allowed {specifier}")] - BannedCommonJsExtension { specifier: String }, - #[error("triple slash directives that modify globals (for example, '/// ' or '/// ') are not allowed. Instead instruct the user of your package to specify these directives. {specifier}:{line}:{column}")] BannedTripleSlashDirectives { specifier: String, @@ -645,9 +636,6 @@ impl PublishError { Some("globalTypeAugmentation") } PublishError::CommonJs { .. } => Some("commonJs"), - PublishError::BannedCommonJsExtension { .. } => { - Some("bannedCommonJsExtension") - } PublishError::BannedTripleSlashDirectives { .. } => { Some("bannedTripleSlashDirectives") } diff --git a/api/testdata/tarballs/cjs_import/jsr.json b/api/testdata/tarballs/cjs_import/jsr.json deleted file mode 100644 index 94c22c0e..00000000 --- a/api/testdata/tarballs/cjs_import/jsr.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "name": "@scope/foo", - "version": "1.2.3", - "exports": "./mod.ts" -} diff --git a/api/testdata/tarballs/cjs_import/mod.ts b/api/testdata/tarballs/cjs_import/mod.ts deleted file mode 100644 index 5455679a..00000000 --- a/api/testdata/tarballs/cjs_import/mod.ts +++ /dev/null @@ -1,3 +0,0 @@ -import { test } from "./other.cjs"; // bad - -export const hello = `Hello, ${test}!`; diff --git a/api/testdata/tarballs/cjs_import/other.cjs b/api/testdata/tarballs/cjs_import/other.cjs deleted file mode 100644 index e89508f4..00000000 --- a/api/testdata/tarballs/cjs_import/other.cjs +++ /dev/null @@ -1 +0,0 @@ -exports.test = "test"; diff --git a/frontend/docs/troubleshooting.md b/frontend/docs/troubleshooting.md index 5e07e445..4adf704f 100644 --- a/frontend/docs/troubleshooting.md +++ b/frontend/docs/troubleshooting.md @@ -91,8 +91,9 @@ source. ### `commonJs` -The package being published contains CommonJS code like `require()`. This is -disallowed because JSR is ESM only. +The package being published imports code from CommonJS files with a `.cjs` or +`.cts` extension or contains CommonJS code like `require()`. This is disallowed +because JSR is ESM only. You can fix this error by removing the CommonJS code from your source. From f476c641b0ab60edab0eced4dfcd2eabab9723a1 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Sat, 9 Nov 2024 13:48:00 +0100 Subject: [PATCH 6/7] chore: re-introduce updated e2e test --- api/src/publish.rs | 10 ++++++++++ api/testdata/tarballs/graph_error/jsr.json | 5 +++++ api/testdata/tarballs/graph_error/mod.ts | 3 +++ api/testdata/tarballs/graph_error/other.cjs | 1 + 4 files changed, 19 insertions(+) create mode 100644 api/testdata/tarballs/graph_error/jsr.json create mode 100644 api/testdata/tarballs/graph_error/mod.ts create mode 100644 api/testdata/tarballs/graph_error/other.cjs diff --git a/api/src/publish.rs b/api/src/publish.rs index eab4b97f..2cd7e279 100644 --- a/api/src/publish.rs +++ b/api/src/publish.rs @@ -1308,6 +1308,16 @@ pub mod tests { assert_eq!(task.status, PublishingTaskStatus::Success, "{task:#?}"); } + #[tokio::test] + async fn graph_error() { + let t = TestSetup::new().await; + let bytes = create_mock_tarball("graph_error"); + let task = process_tarball_setup(&t, bytes).await; + assert_eq!(task.status, PublishingTaskStatus::Failure, "{task:#?}"); + let error = task.error.unwrap(); + assert_eq!(error.code, "graphError"); + } + #[tokio::test] async fn npm_tarball() { let t = TestSetup::new().await; diff --git a/api/testdata/tarballs/graph_error/jsr.json b/api/testdata/tarballs/graph_error/jsr.json new file mode 100644 index 00000000..35b9ed05 --- /dev/null +++ b/api/testdata/tarballs/graph_error/jsr.json @@ -0,0 +1,5 @@ +{ + "name": "@scope/foo", + "version": "1.2.3", + "exports": "./mod.ts" +} \ No newline at end of file diff --git a/api/testdata/tarballs/graph_error/mod.ts b/api/testdata/tarballs/graph_error/mod.ts new file mode 100644 index 00000000..5455679a --- /dev/null +++ b/api/testdata/tarballs/graph_error/mod.ts @@ -0,0 +1,3 @@ +import { test } from "./other.cjs"; // bad + +export const hello = `Hello, ${test}!`; diff --git a/api/testdata/tarballs/graph_error/other.cjs b/api/testdata/tarballs/graph_error/other.cjs new file mode 100644 index 00000000..e89508f4 --- /dev/null +++ b/api/testdata/tarballs/graph_error/other.cjs @@ -0,0 +1 @@ +exports.test = "test"; From f82cbdda95edb583550b575552fd6393fdf3a7a1 Mon Sep 17 00:00:00 2001 From: Michael Herzner Date: Sun, 17 Nov 2024 22:07:32 +0100 Subject: [PATCH 7/7] chore: update test case after merge from main --- api/src/publish.rs | 6 +++--- api/testdata/tarballs/{graph_error => cjs_import}/jsr.json | 0 api/testdata/tarballs/{graph_error => cjs_import}/mod.ts | 0 api/testdata/tarballs/{graph_error => cjs_import}/other.cjs | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename api/testdata/tarballs/{graph_error => cjs_import}/jsr.json (100%) rename api/testdata/tarballs/{graph_error => cjs_import}/mod.ts (100%) rename api/testdata/tarballs/{graph_error => cjs_import}/other.cjs (100%) diff --git a/api/src/publish.rs b/api/src/publish.rs index 3ed34f84..0c80a045 100644 --- a/api/src/publish.rs +++ b/api/src/publish.rs @@ -1310,13 +1310,13 @@ pub mod tests { } #[tokio::test] - async fn graph_error() { + async fn cjs_import() { let t = TestSetup::new().await; - let bytes = create_mock_tarball("graph_error"); + let bytes = create_mock_tarball("cjs_import"); let task = process_tarball_setup(&t, bytes).await; assert_eq!(task.status, PublishingTaskStatus::Failure, "{task:#?}"); let error = task.error.unwrap(); - assert_eq!(error.code, "graphError"); + assert_eq!(error.code, "commonJs"); } #[tokio::test] diff --git a/api/testdata/tarballs/graph_error/jsr.json b/api/testdata/tarballs/cjs_import/jsr.json similarity index 100% rename from api/testdata/tarballs/graph_error/jsr.json rename to api/testdata/tarballs/cjs_import/jsr.json diff --git a/api/testdata/tarballs/graph_error/mod.ts b/api/testdata/tarballs/cjs_import/mod.ts similarity index 100% rename from api/testdata/tarballs/graph_error/mod.ts rename to api/testdata/tarballs/cjs_import/mod.ts diff --git a/api/testdata/tarballs/graph_error/other.cjs b/api/testdata/tarballs/cjs_import/other.cjs similarity index 100% rename from api/testdata/tarballs/graph_error/other.cjs rename to api/testdata/tarballs/cjs_import/other.cjs