fix(resolver): resolve npm packages that shadow Node built-in names in BYONM mode

[bartlomieju]

Summary

Closes #30607

When using --node-modules-dir=manual (BYONM), npm packages whose names match Node.js built-in modules (e.g. events, assert, string_decoder) failed to resolve:

$ deno run --node-modules-dir=manual main.mts
error: Import "events" not a dependency
  hint: If you want to use a built-in Node module, add a "node:" prefix (ex. "node:events").

This happened because NodeResolver::resolve() unconditionally returns BuiltIn for any specifier matching a Node built-in name, before checking node_modules.

Fix

In resolve_if_for_npm_pkg, when the specifier resolves as a built-in but is also listed as a dependency in an ancestor package.json, try resolving it as an npm package first via the new NodeResolver::resolve_package() method (which skips the built-in check). If npm resolution fails, falls back to the built-in. This matches Node.js behavior where node_modules packages shadow built-ins.

Test plan

• Added spec test byonm_builtin_shadow with a pre-built node_modules/events/ that shadows the Node built-in
• All existing BYONM tests pass
• node:events imports still work correctly

🤖 Generated with Claude Code

[bartlomieju]

Addressed the review feedback in 1ebef1d -- removed the NpmResolver::Byonm guard so the builtin-shadow resolution applies to all npm resolver modes.

Note: in practice, the managed (non-BYONM) path does not hit this code because the workspace resolver maps package.json dependencies to npm: specifiers earlier in the resolution chain, before resolve_if_for_npm_pkg is reached. I verified this -- import "events" with managed node_modules already works without this fix. So the existing byonm_builtin_shadow test is the only scenario that exercises this path. The guard removal is a correctness improvement to avoid an unnecessary mode check.

[Copilot]

Pull request overview

Fixes BYONM (--node-modules-dir=manual) resolution so npm packages that share names with Node built-ins (for example events) can be resolved from node_modules/ instead of being treated as built-ins.

Changes:

• Add a BYONM resolution path that attempts npm package resolution when a bare specifier matches a Node built-in.
• Introduce NodeResolver::resolve_package() to resolve packages while skipping the built-in-name short-circuit.
• Add a spec regression test fixture where node_modules/events/ shadows the Node built-in.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/specs/npm/byonm_builtin_shadow/package.json	Adds a minimal dependency declaration for the shadowing package fixture.
tests/specs/npm/byonm_builtin_shadow/node_modules/events/package.json	Adds a minimal npm package fixture for events.
tests/specs/npm/byonm_builtin_shadow/node_modules/events/index.js	Adds a minimal implementation proving resolution came from npm.
tests/specs/npm/byonm_builtin_shadow/main.mts	Imports events to assert the npm package shadows the built-in.
tests/specs/npm/byonm_builtin_shadow/test.jsonc	Adds the regression spec test invocation and expected output.
libs/resolver/npm/mod.rs	Adjusts BYONM fallback resolution to prefer resolving built-in-named specifiers as npm packages.
libs/resolver/lib.rs	Updates a comment near the BYONM bare-specifier fallback logic.
libs/node_resolver/resolution.rs	Adds NodeResolver::resolve_package() helper that skips the built-in check.

[Copilot]

In the BuiltIn branch, resolve_package() errors are currently swallowed unconditionally and the code falls back to returning a built-in. This can hide real npm resolution problems (ex. package exists but has invalid package.json/exports) and also prevents BYONM-specific errors like NodeModulesOutOfDate from triggering when a declared dep is missing. Consider (1) only attempting resolve_package() when the name is actually declared in an ancestor package.json (per BYONM rules), and (2) only falling back to the built-in on “package not found” style errors—otherwise propagate the resolution error.

Suggested change

	// If npm package resolution fails, return the built-in result
	Err(_) => Ok(Some(NodeResolution::BuiltIn(specifier.to_string()))),
	Err(err) => {
	let err = err.into_kind();
	Err(
	ResolveIfForNpmPackageErrorKind::NodeResolve(err.into())
	.into_box(),
	)
	}

[Copilot]

The fallback constructs NodeResolution::BuiltIn(specifier.to_string()) rather than reusing the module name from the earlier Ok(NodeResolution::BuiltIn(..)). If the built-in result ever differs from the original input (for example via normalization), this could return an incorrect built-in name. Consider preserving and reusing the built-in name from resolution_result when falling back.

[Copilot]

The doc comment says resolve_package is “Like resolve, but skips the built-in module check”, but the implementation also skips resolve’s URL handling (notably the node: built-in URL mapping and the explicit unsupported-scheme check). Either refactor to share the same logic as resolve (toggling only the built-in check) or narrow the doc comment to the actual intended usage (bare package names only) to avoid future misuse.