fix: bundled npm compile output naming#35459
Conversation
|
Pushed a follow-up commit adding the required reason to the |
|
Pushed |
bartlomieju
left a comment
There was a problem hiding this comment.
Reviewed the diff plus how needs_npm_embed flows into the binary writer. The naming fix itself is clean and well-tested, but the needs_npm_embed change looks like it can drop npm embedding for managed-npm bundles. Flagging as a comment rather than approving.
🔴 path_is_in_node_modules misses managed-npm (global cache) paths
run_bundle_for_compile now sets needs_npm_embed only when a referenced path contains a literal node_modules component (cli/tools/compile.rs:319-322, 345-348, helper at :528), replacing the old main_rewrite.rewrote_paths.
But for managed npm without a node_modules dir (the default), packages resolve from the global cache:
<DENO_DIR>/npm/registry.npmjs.org/<name>/<version>/...
— no node_modules segment. This is exactly why pkg_folder_node_modules_root() has the .unwrap_or(folder.as_path()) fallback in cli/standalone/binary.rs:1258.
So a bundle whose CJS-from-ESM wrapper require()s an npm package resolved from the global cache now computes needs_npm_embed = false and silently skips npm embedding → runtime require() failure. The old rewrote_paths flag fired for these and embedded correctly. This is the headline deno compile --bundle npm:<pkg> flow (no package.json).
Why the existing tests don't catch it: cjs_import_from_esm, native_addon, and cjs_deep_graph all ship a package.json, which auto-enables a node_modules dir, so their referenced paths do contain node_modules. The new npm_entrypoint_default_name test uses a pure-JS package (correctly no embed). Nothing exercises the global-cache + external-require combination.
Suggested fix: decide embedding based on whether a referenced path maps to an npm package (reuse the resolver / npm-cache-root logic already in collect_bundle_required_packages), not a literal node_modules string match — and add a test for compile --bundle npm:<cjs-or-native-pkg> with no node_modules dir.
nit: duplicated logic
path_is_in_node_modules (cli/tools/compile.rs:528) duplicates the inline path.components().any(|c| c.as_os_str() == "node_modules") at cli/tools/compile.rs:247-248. Use the helper in both places. (Note that inline check shares the same global-cache blind spot when deciding whether to add a path to the include set, though that part is pre-existing.)
nit: scope — tools/lint.js
Serializing the workflow YAML generators is unrelated to compile output naming and touches CI tooling. Fine if it's needed to unblock CI, but per repo convention CI tooling changes usually want @bartlomieju's review, and this could be a separate PR.
Other
- CI: no checks are visible yet — please confirm the
compile/bundlespec suite is green across all OSes before merge. - The
#[allow(dead_code, reason = ...)]onrewrote_paths(now read only by unit tests) is appropriately justified. 👍
Nice work on the inference plumbing and the unit + spec coverage for the naming case.
|
Addressed the managed-npm global-cache case: referenced bundle paths are now classified through the npm resolver instead of only checking for a |
|
Pushed a follow-up formatting fix for |
This reverts commit 8cd9c36.
d906e01 to
5f9cb90
Compare
|
Pushed a follow-up fix after rebasing onto latest |
Summary
deno compile --bundlerewrites the entrypoint to a temporary bundle file.deno_compile_bundle_*node_modulesembedding while preserving existing bundle/runtime npm coverageValidation