diff --git a/changelog.d/11072-cjs-require-export-condition.md b/changelog.d/11072-cjs-require-export-condition.md new file mode 100644 index 0000000000..cd2d6fb3cc --- /dev/null +++ b/changelog.d/11072-cjs-require-export-condition.md @@ -0,0 +1,3 @@ +### Fixed: compiled CommonJS packages now honor `exports.require` (#11047, PR #11072) + +Perry's CommonJS wrapper previously converted `require("pkg")` into an ordinary ESM import before package resolution. Dual-entry packages therefore selected `exports.import` instead of `exports.require`; for `ws`, this loaded `wrapper.mjs`, whose default `WebSocket` class intentionally lacks the CommonJS-only `.Server` attachment, and `new WebSocket.Server()` failed with `undefined is not a constructor`. Bare requires for packages selected by `compilePackages` now resolve their require-condition entry while the call-site context is available and emit that target as a relative import. Packages outside the compile allowlist keep their bare specifier and remain behind the existing native compilation trust boundary. diff --git a/crates/perry/src/commands/compile/cjs_wrap/mod.rs b/crates/perry/src/commands/compile/cjs_wrap/mod.rs index 115b8b5898..dfac9fd946 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/mod.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/mod.rs @@ -49,6 +49,8 @@ mod issue_10662_tests; #[cfg(test)] mod issue_6585_tests; #[cfg(test)] +mod package_resolution_tests; +#[cfg(test)] mod parcel_watcher_tests; #[cfg(test)] mod preamble_canary_tests; diff --git a/crates/perry/src/commands/compile/cjs_wrap/package_resolution_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/package_resolution_tests.rs new file mode 100644 index 0000000000..81fe16fd7a --- /dev/null +++ b/crates/perry/src/commands/compile/cjs_wrap/package_resolution_tests.rs @@ -0,0 +1,72 @@ +use super::wrap::wrap_commonjs_for_target; +use std::collections::HashSet; +use std::fs; + +/// Issue #11047: once a CommonJS `require("pkg")` is wrapped as an ESM import, +/// package resolution must retain require-call semantics. `ws` exposes an ESM +/// default under `exports.import`, but only its `exports.require` entry adds +/// the legacy `WebSocket.Server` property. +#[test] +fn cjs_bare_require_uses_package_require_export_condition() { + let dir = tempfile::tempdir().expect("tempdir"); + let package = dir.path().join("node_modules/dual-entry"); + fs::create_dir_all(&package).expect("create package"); + fs::write( + package.join("package.json"), + r#"{ + "name": "dual-entry", + "exports": { + ".": { + "import": "./wrapper.mjs", + "require": "./index.js" + } + } +}"#, + ) + .expect("write package.json"); + fs::write( + package.join("wrapper.mjs"), + "export default class WebSocket {}\n", + ) + .expect("write ESM entry"); + fs::write( + package.join("index.js"), + "module.exports = class WebSocket {};\n", + ) + .expect("write CJS entry"); + + let entry = dir.path().join("entry.js"); + let compile_packages = HashSet::from(["dual-entry".to_string()]); + let wrapped = wrap_commonjs_for_target( + "const WebSocket = require('dual-entry');\nmodule.exports = WebSocket;\n", + &entry, + None, + false, + Some(&compile_packages), + ); + let expected = "node_modules/dual-entry/index.js\";"; + assert!( + wrapped.contains(expected), + "expected require-condition entry ending in `{expected}`, got:\n{wrapped}" + ); + assert!( + !wrapped.contains("wrapper.mjs"), + "CommonJS require must not select the import-condition entry:\n{wrapped}" + ); + + let wrapped_unapproved = wrap_commonjs_for_target( + "module.exports = require('dual-entry');\n", + &entry, + None, + false, + Some(&HashSet::new()), + ); + assert!( + wrapped_unapproved.contains("from 'dual-entry';"), + "a package outside compilePackages must retain its bare specifier:\n{wrapped_unapproved}" + ); + assert!( + !wrapped_unapproved.contains("node_modules/dual-entry/index.js"), + "require-condition resolution must not pull an unapproved package into native compilation:\n{wrapped_unapproved}" + ); +} diff --git a/crates/perry/src/commands/compile/cjs_wrap/parcel_watcher_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/parcel_watcher_tests.rs index 5fb96888a8..3f8ea04b0c 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/parcel_watcher_tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/parcel_watcher_tests.rs @@ -15,6 +15,7 @@ module.exports = binding &PathBuf::from("/tmp/node_modules/opencode/watcher.js"), Some("linux-x86_64-musl"), false, + None, ); assert!( wrapped.contains("from '@parcel/watcher-linux-x64-musl'") diff --git a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs index d8e3b7d87e..fe5fa97eb6 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs @@ -50,7 +50,7 @@ exports.compute = compute; fn wrap_and_lower(body: &str) -> perry_hir::Module { let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); - let wrapped = wrap_commonjs_for_target(body, path, None, false); + let wrapped = wrap_commonjs_for_target(body, path, None, false, None); let ast = perry_parser::parse_typescript(&wrapped, "index.js") .expect("the wrap template must produce parseable ESM"); perry_hir::lower_module(&ast, "dep", &path.to_string_lossy()) @@ -63,7 +63,7 @@ fn wrap_and_lower(body: &str) -> perry_hir::Module { #[test] fn cjs_preamble_does_not_arm_the_ptr_shape_module_barrier() { let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); - let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false); + let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false, None); // Anti-vacuity, and the more precise failure of the two: assert the // preamble still HAS the site the recogniser is written for. Without this @@ -129,7 +129,7 @@ const EXPECTED_PREAMBLE_ALLOC_STMTS: usize = 2; #[test] fn the_cjs_preamble_is_still_recognised_as_scaffolding_allocation() { let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); - let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false); + let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false, None); // Anti-vacuity on the template, one assertion per recogniser conjunct, so // a template edit names the conjunct it broke rather than failing as an @@ -197,7 +197,7 @@ fn a_module_that_was_never_cjs_wrapped_has_no_preamble() { fn path_module_wrap_publishes_partial_then_final_exports_and_tracks_undefined() { let path = Path::new("/tmp/perry-canary/.next/server/chunks/lazy.js"); let marker = "exports.ready = true;"; - let wrapped = wrap_commonjs_for_target(marker, path, None, false); + let wrapped = wrap_commonjs_for_target(marker, path, None, false, None); let partial = wrapped .find("__perry_register_path_module_partial(") @@ -247,7 +247,7 @@ fn path_module_wrap_publishes_partial_then_final_exports_and_tracks_undefined() #[test] fn computed_relative_requires_are_joined_against_the_module_dir() { let path = Path::new("/tmp/perry-canary/.next/server/webpack-runtime.js"); - let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false); + let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false, None); // Anti-vacuity: if the wrap stops consulting the registry at all, the // assertions below would be about a branch that no longer exists. @@ -290,7 +290,7 @@ fn computed_relative_requires_are_joined_against_the_module_dir() { fn the_wrap_still_binds_the_local_the_cjs_entry_recogniser_keys_on() { let local = perry_codegen::cjs_wrap_create_require_local(); let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); - let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false); + let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None, false, None); // Anti-vacuity: the template must still emit the binding at all. assert!( diff --git a/crates/perry/src/commands/compile/cjs_wrap/tests.rs b/crates/perry/src/commands/compile/cjs_wrap/tests.rs index 8cf8a8f98f..5af40e6262 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/tests.rs @@ -28,7 +28,7 @@ fn cjs_wrap_body_offset_maps_back_to_original_line() { // line `L - prefix_line_count`. let original = "function f() {\n return new Nope();\n}\nmodule.exports = f;\n"; let path = PathBuf::from("/tmp/x/index.js"); - let (wrapped, body_off) = wrap_commonjs_with_body_offset(original, &path, None, false); + let (wrapped, body_off) = wrap_commonjs_with_body_offset(original, &path, None, false, None); let body_off = body_off.expect("body should be locatable in wrapped output"); // Prefix line count = newlines before the body in the wrapped output. let prefix_lines = wrapped.as_bytes()[..body_off] @@ -749,6 +749,7 @@ exports.spawn = function spawn() { return terminalCtor; }; &PathBuf::from("/tmp/node_modules/node-pty/lib/index.js"), Some("windows"), false, + None, ); assert!( wrapped.contains("import _lazyreq_0 from './windowsTerminal';"), @@ -784,6 +785,7 @@ exports.spawn = function spawn() { return terminalCtor; }; &PathBuf::from("/tmp/node_modules/node-pty/lib/index.js"), Some("linux"), false, + None, ); assert!( wrapped.contains("from './unixTerminal'"), diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index 6b8060127a..302aa3cb40 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -3,7 +3,34 @@ use super::*; use std::borrow::Cow; -use std::path::Path; +use std::collections::HashSet; +use std::path::{Component, Path, PathBuf}; + +fn relative_import_specifier(from: &Path, to: &Path) -> Option { + let from: Vec> = from.components().collect(); + let to: Vec> = to.components().collect(); + if from.first() != to.first() { + return None; + } + let common = from + .iter() + .zip(&to) + .take_while(|(left, right)| left == right) + .count(); + let mut relative = PathBuf::new(); + for _ in common..from.len() { + relative.push(".."); + } + for component in &to[common..] { + relative.push(component.as_os_str()); + } + let relative = relative.to_string_lossy().replace('\\', "/"); + if relative.starts_with('.') { + Some(relative) + } else { + Some(format!("./{relative}")) + } +} fn resolved_native_addon( source_path: &Path, @@ -105,7 +132,7 @@ pub(in crate::commands::compile) fn wrap_commonjs(source: &str, source_path: &Pa // here, which is correct for the overwhelming majority of CJS-wrapped // files (dependencies). The real per-module entry status is threaded // explicitly from `collect_modules.rs`, the only place that knows it. - wrap_commonjs_for_target(source, source_path, None, false) + wrap_commonjs_for_target(source, source_path, None, false, None) } pub(in crate::commands::compile) fn wrap_commonjs_for_target( @@ -113,8 +140,16 @@ pub(in crate::commands::compile) fn wrap_commonjs_for_target( source_path: &Path, target: Option<&str>, is_entry_module: bool, + compile_packages: Option<&HashSet>, ) -> String { - wrap_commonjs_with_body_offset(source, source_path, target, is_entry_module).0 + wrap_commonjs_with_body_offset( + source, + source_path, + target, + is_entry_module, + compile_packages, + ) + .0 } /// Like [`wrap_commonjs_for_target`], but also returns the byte offset within @@ -129,6 +164,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( source_path: &Path, target: Option<&str>, is_entry_module: bool, + compile_packages: Option<&HashSet>, ) -> (String, Option) { let mut source_cow = Cow::Borrowed(source); @@ -424,7 +460,59 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( | "_http_server" => "http", other => other, }; - format!("import {} from '{}';", local, import_spec) + // #11047: this import represents a CommonJS `require`, so bare + // package specifiers must use the package's `require` export + // condition. Leaving the original specifier here sends it through + // the ordinary ESM resolver after wrapping, which prefers + // `exports.import`. For `ws`, that selected `wrapper.mjs` instead + // of `index.js`; the ESM default is WebSocket but intentionally + // lacks the CommonJS-only `.Server` attachment. + // + // Resolve the require entry while the original call-site context + // is still known. Keep relative imports spelled as written so + // their existing cycle/deferred-module handling remains intact. + let resolved_require_spec = if import_spec == spec + && !spec.starts_with("./") + && !spec.starts_with("../") + && !std::path::Path::new(spec).is_absolute() + && compile_packages.is_none_or(|packages| { + let (package_name, _) = + super::super::resolve::parse_package_specifier(spec); + packages.contains("*") || packages.contains(&package_name) + }) + { + source_path + .parent() + .and_then(|module_dir| { + super::super::collect_modules::static_require_transform::resolve_static_require( + module_dir, + spec, + None, + ) + }) + // Keep the resolved target relative to the importing + // module. Absolute node_modules imports are classified as + // ordinary runtime JS by the general resolver because the + // original package name (and therefore compilePackages + // opt-in) is no longer visible there. + .and_then(|path| { + source_path + .parent() + .and_then(|module_dir| relative_import_specifier(module_dir, &path)) + }) + } else { + None + }; + if let Some(resolved) = resolved_require_spec { + format!( + "import {} from {};", + local, + serde_json::to_string(&resolved) + .expect("CJS import specifier is JSON encodable") + ) + } else { + format!("import {} from '{}';", local, import_spec) + } }) .collect::>() .join("\n"); diff --git a/crates/perry/src/commands/compile/collect_modules.rs b/crates/perry/src/commands/compile/collect_modules.rs index da168bcbdd..5953e87bea 100644 --- a/crates/perry/src/commands/compile/collect_modules.rs +++ b/crates/perry/src/commands/compile/collect_modules.rs @@ -43,7 +43,7 @@ mod native_addon; mod parse_error; pub(crate) mod reexport_prune; mod script_string; -mod static_require_transform; +pub(super) mod static_require_transform; #[cfg(test)] mod tests; mod walk; @@ -423,6 +423,7 @@ fn collect_module_one( &canonical, target, cjs_is_entry_module, + Some(&ctx.compile_packages), ); // Newlines before the original body in the wrapped output = the // wrapper prefix line count. Recorded only when the body was @@ -441,6 +442,7 @@ fn collect_module_one( &canonical, target, cjs_is_entry_module, + Some(&ctx.compile_packages), ) } } else { diff --git a/crates/perry/src/commands/compile/collect_modules/static_require_transform.rs b/crates/perry/src/commands/compile/collect_modules/static_require_transform.rs index 67a61c1169..f473b8c4f0 100644 --- a/crates/perry/src/commands/compile/collect_modules/static_require_transform.rs +++ b/crates/perry/src/commands/compile/collect_modules/static_require_transform.rs @@ -207,7 +207,7 @@ fn is_bare_call(masked_source: &str, start: usize, end: usize) -> bool { .all(|b| b.is_ascii_whitespace()) } -pub(super) fn resolve_static_require( +pub(in crate::commands::compile) fn resolve_static_require( module_dir: &Path, specifier: &str, bunfs_root: Option<&Path>,