fix: make '@types' package resolution more consistent

[dsherret]

Shifts @types package resolution down into node_resolver so it's always done. We were being inconsistent.

[coderabbitai]

Walkthrough

Added a new resolve_types_package_folder method to the NpmPackageFolderResolver trait and implemented it for Byonm, Managed, Global, Local, and test resolvers. types_package_name in node_resolver was changed from public String to private Option<String>. Package-subpath resolution now can fall back to @types packages via new helper functions and snapshot lookup; TSC error handling was simplified to treat missing module/type subpaths uniformly. Tests had typos fixed in npm package names, and a workspace dependency version was bumped from 0.42.1 to 0.42.2.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.03% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: make '@types' package resolution more consistent' directly describes the main change: refactoring @types package resolution logic and moving it into node_resolver for consistency.
Description check	✅ Passed	The PR description clearly relates to the changeset, describing the core intent to shift @types package resolution into node_resolver for consistency.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

libs/node_resolver/resolution.rs (1)

1631-1671: Confirm intent of broad @types fallback on any Types error

In both resolve_package_subpath_for_package and resolve_package_dir_subpath, the @types fallback is triggered whenever resolution_kind.is_types() and the primary resolution result is any Err(...), without inspecting the underlying NodeJsErrorCode. That means we’ll also try @types/<name> for cases like misconfigured exports or invalid targets, not just for “module/types not found”.

If the goal is “whenever types resolution fails, see if an @types package can satisfy it”, this is fine as‑is. If you only meant to cover strict “not found” scenarios, you may want to gate the fallback on a specific code (for example ERR_TYPES_NOT_FOUND and/or ERR_MODULE_NOT_FOUND) before trying @types.

Right now it’s a behavioral choice rather than a correctness bug, but worth double‑checking that this broader fallback is what you want.

Also applies to: 1689-1718

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 824dc61 and 9d78ac8.

📒 Files selected for processing (4)

• cli/tsc/mod.rs (4 hunks)
• libs/node_resolver/errors.rs (1 hunks)
• libs/node_resolver/resolution.rs (5 hunks)
• libs/resolver/npm/mod.rs (2 hunks)

✅ Files skipped from review due to trivial changes (1)

• libs/node_resolver/errors.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• libs/node_resolver/resolution.rs
• cli/tsc/mod.rs
• libs/resolver/npm/mod.rs

---

⚙️ CodeRabbit configuration file

Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.

Files:

• libs/node_resolver/resolution.rs
• cli/tsc/mod.rs
• libs/resolver/npm/mod.rs

🧠 Learnings (1)

📚 Learning: 2025-11-24T16:19:37.808Z

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/module_loader.rs : Module loading and resolution is handled in `cli/module_loader.rs`

Applied to files:

• cli/tsc/mod.rs

🧬 Code graph analysis (2)

cli/tsc/mod.rs (2)

libs/node_resolver/errors.rs (1)

• from (63-65)

ext/node/polyfills/internal/errors.ts (1)

• ERR_MODULE_NOT_FOUND (2544-2551)

libs/resolver/npm/mod.rs (8)

libs/node_resolver/npm.rs (1)

• resolve_types_package_folder (25-30)

libs/resolver/npm/managed/global.rs (1)

• resolve_types_package_folder (184-197)

libs/resolver/npm/managed/common.rs (1)

• resolve_types_package_folder (81-99)

libs/resolver/npm/managed/local.rs (1)

• resolve_types_package_folder (207-228)

libs/resolver/npm/byonm.rs (1)

• resolve_types_package_folder (441-450)

libs/resolver/npm/managed/mod.rs (1)

• resolve_types_package_folder (269-280)

libs/resolver/workspace.rs (1)

• resolve_types_package_folder (1839-1846)

libs/node_resolver/resolution.rs (1)

• types_package_name (2510-2531)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: test debug linux-aarch64
• GitHub Check: test debug linux-x86_64
• GitHub Check: test debug windows-x86_64
• GitHub Check: test release linux-x86_64
• GitHub Check: test debug macos-aarch64
• GitHub Check: test debug macos-x86_64
• GitHub Check: lint debug windows-x86_64
• GitHub Check: build libs
• GitHub Check: lint debug linux-x86_64
• GitHub Check: lint debug macos-x86_64

🔇 Additional comments (4)

cli/tsc/mod.rs (1)

580-603: Npm types error wiring and resolution path look consistent

Adding ResolveError::ResolvePkgFolderFromDenoReq and switching both graph and non-graph npm types resolution to use resolve_pkg_folder_from_deno_module_req—while treating ERR_MODULE_NOT_FOUND and ERR_TYPES_NOT_FOUND uniformly as “no types found” (returning None)—is coherent with the new resolver API and keeps the CLI’s behavior straightforward. I don’t see correctness or propagation issues here.

Also applies to: 663-687, 769-775

libs/resolver/npm/mod.rs (1)

13-13: resolve_types_package_folder delegation matches trait and backends

The new Version import and NpmPackageFolderResolver for NpmResolver<TSys>::resolve_types_package_folder implementation are straightforward: they just forward (types_package_name, maybe_package_version, maybe_referrer) to the underlying Byonm/Managed resolvers, which is exactly what the trait expects. The behavior will mainly be shaped by those backend impls; at this layer, the wiring looks correct.

Also applies to: 327-363

libs/node_resolver/resolution.rs (2)

2079-2117: Helpers for resolving @types package folders are consistent

resolve_types_package_folder_from_package_json and resolve_types_package_folder_with_name_and_version correctly:

• Derive (name, version?) from PackageJson when available, parsing the version via Version::parse_from_npm best‑effort.
• Compute the corresponding @types package name with types_package_name, bailing out for already‑@types packages.
• Delegate to npm_pkg_folder_resolver.resolve_types_package_folder and emit a debug log with the effective name@version (or *).

The logic is tight and keeps all @types lookup specifics encapsulated.

---

2508-2531: types_package_name Option-returning helper and tests look good

Making types_package_name private and return Option<String> (None for inputs already under @types/) matches how it’s used by the new helpers and prevents double‑wrapping @types/@types/*. The implementation correctly handles scoped packages by mapping / to __, and the updated tests cover:

• Unscoped: "name" -> Some("@types/name")
• Scoped: "@scoped/package" -> Some("@types/scoped__package")
• Already-typed: "@types/node" -> None

This behavior lines up with DefinitelyTyped conventions and the new resolver logic.

Also applies to: 2842-2848

[nathanwhit]

LGTM