fix(compile): handle CJS and native addons in --bundle#34529
Conversation
Bundles the entrypoint with esbuild before embedding it in the compiled binary, instead of shipping the entire node_modules tree. Tree-shaken output and a `createRequire(import.meta.url)` shim let CJS-style `require()` keep working in the standalone runtime. Prints a warning since this is the first cut and a number of cases (native .node addons, worker entrypoints, dynamic require/import) aren't handled yet.
Stacks on top of #34527. Two things to make `compile --bundle` usable with anything beyond pure-ESM trees. First, the bundler's CJS-from-ESM wrapper hardcodes the absolute path of each CJS module it imports. At runtime the VFS is mounted at a temp dir under $TMPDIR, so those paths miss. Rewrite each literal in the bundle output to a runtime-relative resolution against `import.meta.url` so it survives relocation. Second, when those rewritten requires hit something at runtime — either a CJS dep loaded through the wrapper or a `.node` native addon — the npm tree has to ship in the binary. Detect both signals (the rewrite fired; or `find_native_addon_packages` found a package with a `.node` in the resolved snapshot) and embed the full snapshot in that case. Pure-ESM bundles still skip everything npm and stay tiny. Tested with chalk (pure ESM, ~17 KB user payload), lodash (CJS, full npm), and `@node-rs/crc32` (native addon, full npm — addon loads and runs). Spec test under `tests/specs/compile/bundle/native_addon/` exercises the addon path against the local `@denotest/node-addon` fixture.
The bundle path-rewriter only matched POSIX absolute paths (a leading `/`), so on Windows the drive-letter paths esbuild emits for the CJS-from-ESM wrapper (e.g. "C:\\Users\\...\\index.js", with JS-escaped backslashes) were never rewritten. They stayed as build-time absolute paths, so the compiled binary's require() pointed at a location that doesn't exist at runtime, failing with "NotFound: path not found" in loadMaybeCjs. This broke compile/bundle/native_addon on Windows. Broaden the regex to also match Windows drive-letter paths with either separator style, collapse JS-escaped backslashes before touching the filesystem, and factor the core out so it can be unit-tested with synthetic paths on every platform.
The compile-only import.meta.main transform parsed each module with its original media type. A CJS module imported from ESM reaches the transform as the ESM facade the module loader generates (createRequire + export default), but MediaType::Cjs makes deno_ast parse in script mode, which rejects the facade's import/export statements with a parse error. deno run and standalone deno bundle don't run this transform, so they accepted it. Map Cjs to JavaScript before parsing so program mode auto-detects module vs script, handling both the facade and genuine CJS scripts. Closes the .cjs-in-static-imports blocker reported against the compile bundle stack (affects jiti and anything depending on it).
…ative-addons # Conflicts: # cli/standalone/binary.rs # cli/tools/bundle/mod.rs # cli/tools/compile.rs
CJS npm packages with a default export are now inlined into the bundle rather than externalized as a stripped runtime require(). The failure mode shifted from a NotCapable read-access error to a TypeError on the non-callable default import. Update run.out and the explanatory comment to lock in the new (still exit-1) behavior.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Self-review. Solid PR overall — the rewrite-to-relative + embed-npm-only-when-needed approach is sound and well-commented, and the experimental warning sets expectations. Gates pass (title format, focused scope, internal contributor). Confirm CI is green before merge — no checks are showing yet.
Findings inline, ordered by significance. None are blocking for an experimental feature; the regex-breadth (compile.rs) and the comment/impl mismatch (binary.rs) are the two I'd most want addressed before this stops being experimental.
Test coverage is good: unit tests for the rewrite (native path shape + skips-missing) and a real round-trip via cjs_import_from_esm. Two gaps to consider — no unit test asserting the rewrite is skipped for relative __commonJS-style keys (guards the regex-breadth concern), and the BYONM path is stubbed with a TODO and no test documenting that native addons under BYONM + bundle currently won't embed. The npm_cjs_unsupported .out flip from NotCapable to TypeError is legitimate — it reflects the real behavior change from the main merge and locks in a known failure mode rather than masking a regression.
Document the regex rewrite assumption (absolute literals only appear at external require call sites) and the VFS cwd-relative layout invariant, reword the native-addon prose to clarify only .node imports stay external, and flag the snapshot re-walk in fill_bundle_native_addon_vfs.
find_native_addon_packages returned empty for BYONM, so a pure-ESM bundle that imports a native addon under BYONM embedded nothing and the .node failed to resolve at runtime. It only worked by accident when a CJS dep also forced the wholesale node_modules embed. Now walk the workspace node_modules trees (the same set fill_npm_vfs embeds wholesale) and report folders that ship a .node so the embed fires. Hoisted vs nested layout is irrelevant because the BYONM embed is wholesale, so a plain existence scan is correct. Also add the skip-relative-key rewrite unit test flagged in self-review, asserting esbuild's relative __commonJS keys are left untouched.
|
Followed up on the two test/coverage gaps from the self-review (commit cf3a9a6). 1. Skip-relative-key rewrite test. Added 2. BYONM native addons. Implemented the detection rather than leaving it stubbed, and replaced the TODO. The key realization is that the TODO's stated blocker (hoisted-vs-nested monorepo layout) only matters for precise per-package embedding, which BYONM does not do: Without this, a pure-ESM bundle that imports a native addon under BYONM embedded nothing and the addon failed to resolve at runtime; it only worked by accident when a CJS dep also forced the wholesale embed. Covered by two unit tests ( |
bartlomieju
changed the title
Resolves conflicts from the squashed landing of the prior stacked PRs (#34527 --bundle flag, #34529 CJS/native addons). Keeps the worker bundling logic (BundleForCompileResult, discover_worker_urls, rewrite_worker_urls, per-worker bundling) while adopting main's refactors: the testable rewrite_absolute_bundle_paths_inner with Windows-path support, the up-front CleanupGuard, and the BYONM native-addon scan (workspace_root parameter).
Two things to make `compile --bundle` usable with anything beyond pure-ESM trees. First, the bundler's CJS-from-ESM wrapper hardcodes the absolute path of each CJS module it imports. At runtime the VFS is mounted at a temp dir under `$TMPDIR`, so those paths miss. Rewrite each literal in the bundle output to a runtime-relative resolution against `import.meta.url` so it survives the relocation. Second, when those rewritten requires hit something at runtime — either a CJS dep loaded through the wrapper or a `.node` native addon — the npm tree has to ship in the binary. Detect both signals (the rewrite fired; or `find_native_addon_packages` found a package with a `.node` in the resolved snapshot) and embed the full snapshot in that case. Pure-ESM bundles still skip everything npm and stay tiny. Smoke-tested against chalk (pure ESM, ~17 KB user payload), lodash (CJS, full npm), and `@node-rs/crc32` (native addon, full npm; the addon loads and runs). A spec test under `tests/specs/compile/bundle/native_addon/` exercises the addon path against the existing `@denotest/node-addon` fixture, alongside PR 1's basic test.
1 participant
Stacks on top of #34527. Two things to make
compile --bundleusablewith anything beyond pure-ESM trees.
First, the bundler's CJS-from-ESM wrapper hardcodes the absolute path
of each CJS module it imports. At runtime the VFS is mounted at a temp
dir under
$TMPDIR, so those paths miss. Rewrite each literal in thebundle output to a runtime-relative resolution against
import.meta.urlso it survives the relocation.Second, when those rewritten requires hit something at runtime —
either a CJS dep loaded through the wrapper or a
.nodenative addon— the npm tree has to ship in the binary. Detect both signals (the
rewrite fired; or
find_native_addon_packagesfound a package with a.nodein the resolved snapshot) and embed the full snapshot in thatcase. Pure-ESM bundles still skip everything npm and stay tiny.
Smoke-tested against chalk (pure ESM, ~17 KB user payload), lodash
(CJS, full npm), and
@node-rs/crc32(native addon, full npm; theaddon loads and runs). A spec test under
tests/specs/compile/bundle/native_addon/exercises the addon pathagainst the existing
@denotest/node-addonfixture, alongside PR 1'sbasic test.