Skip to content

Conversation

@dsherret
Copy link
Member

@dsherret dsherret commented Dec 7, 2025

@dsherret dsherret added this to the 2.6.0 milestone Dec 7, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 7, 2025

Walkthrough

This change modifies the internal specifier validation in package_imports_resolve_internal to permit specifiers starting with "#/" to continue through the resolution pipeline rather than being rejected early. Previously, the condition rejected any specifier that was exactly "#", started with "#/", or ended with '/'. Now only "#" and trailing '/' are rejected. Accompanying the core change is a new test case (imports_slash_wildcard) that validates this behavior using an ES module with an import map that aliases "#/*" to "./src/*.js", including a main entry point and an exported utility function.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% 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 clearly describes the main change: allowing subpath imports starting with '#/' by removing the validation restriction.
Description check ✅ Passed The description references a Node.js PR that motivated this change, relating to the feature being implemented.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c27d8f0 and e795fac.

⛔ Files ignored due to path filters (1)
  • tests/specs/node/imports_slash_wildcard/main.out is excluded by !**/*.out
📒 Files selected for processing (5)
  • libs/node_resolver/resolution.rs (1 hunks)
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc (1 hunks)
  • tests/specs/node/imports_slash_wildcard/main.js (1 hunks)
  • tests/specs/node/imports_slash_wildcard/package.json (1 hunks)
  • tests/specs/node/imports_slash_wildcard/src/add.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

  • tests/specs/node/imports_slash_wildcard/src/add.js
  • tests/specs/node/imports_slash_wildcard/main.js
**/*.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

⚙️ 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
tests/specs/**/__test__.jsonc

📄 CodeRabbit inference engine (CLAUDE.md)

Spec tests should be created in tests/specs/ with a __test__.jsonc file describing test steps and input files

Files:

  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
tests/specs/**/{__test__.jsonc,*.out}

📄 CodeRabbit inference engine (CLAUDE.md)

Output assertions in spec tests should use __test__.jsonc inline fields or .out files with special matching syntax: [WILDCARD], [WILDLINE], [WILDCHAR], [WILDCHARS(n)], [UNORDERED_START]/[UNORDERED_END], and [# comment]

Files:

  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to tests/specs/**/{__test__.jsonc,*.out} : Output assertions in spec tests should use `__test__.jsonc` inline fields or `.out` files with special matching syntax: `[WILDCARD]`, `[WILDLINE]`, `[WILDCHAR]`, `[WILDCHARS(n)]`, `[UNORDERED_START]`/`[UNORDERED_END]`, and `[# comment]`
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to tests/specs/**/*.out : Use `[WILDCHAR]` in spec test output files to match the next single character
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to tests/specs/**/*.out : Use `[WILDCARD]` in spec test output files to match 0 or more of any character (can cross newlines)
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to tests/specs/**/*.out : Use `[WILDLINE]` in spec test output files to match 0 or more of any character ending at the end of a line
📚 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 tests/specs/**/__test__.jsonc : Spec tests should be created in `tests/specs/` with a `__test__.jsonc` file describing test steps and input files

Applied to files:

  • tests/specs/node/imports_slash_wildcard/package.json
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/{__test__.jsonc,*.out} : Output assertions in spec tests should use `__test__.jsonc` inline fields or `.out` files with special matching syntax: `[WILDCARD]`, `[WILDLINE]`, `[WILDCHAR]`, `[WILDCHARS(n)]`, `[UNORDERED_START]`/`[UNORDERED_END]`, and `[# comment]`

Applied to files:

  • tests/specs/node/imports_slash_wildcard/package.json
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use `[WILDCARD]` in spec test output files to match 0 or more of any character (can cross newlines)

Applied to files:

  • tests/specs/node/imports_slash_wildcard/package.json
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use `[WILDCHAR]` in spec test output files to match the next single character

Applied to files:

  • tests/specs/node/imports_slash_wildcard/package.json
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use `[WILDLINE]` in spec test output files to match 0 or more of any character ending at the end of a line

Applied to files:

  • tests/specs/node/imports_slash_wildcard/package.json
  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use `[WILDCHARS(n)]` in spec test output files to match any of the next n characters

Applied to files:

  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use `[UNORDERED_START]` and `[UNORDERED_END]` in spec test output files to match lines in any order for non-deterministic output

Applied to files:

  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
📚 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 tests/specs/**/*.out : Use line comments in spec test output files with format `[# example]`

Applied to files:

  • tests/specs/node/imports_slash_wildcard/__test__.jsonc
🧬 Code graph analysis (1)
tests/specs/node/imports_slash_wildcard/main.js (1)
tests/specs/node/imports_slash_wildcard/src/add.js (1)
  • add (1-3)
⏰ 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 release linux-x86_64
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: build libs
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: lint debug windows-x86_64
🔇 Additional comments (5)
tests/specs/node/imports_slash_wildcard/package.json (1)

1-6: package.json imports mapping is well-formed for the '#/add' test

The "imports": { "#/*": "./src/*.js" } mapping is valid and correctly set up to resolve #/add to ./src/add.js for this spec test. No issues here.

libs/node_resolver/resolution.rs (1)

1039-1049: Relaxed internal imports name validation correctly allows #/subpath

The updated guard to only reject name == "#" || name.ends_with('/') is appropriate: it keeps bare # and trailing-slash specifiers invalid while now letting #/foo flow into the normal imports resolution path (via resolve_pkg_json_import and resolve_package_target). That matches the PR’s goal and should only change behavior for the previously hard‑rejected #/subpath cases.

tests/specs/node/imports_slash_wildcard/src/add.js (1)

1-3: add helper is fine

Simple, side‑effect‑free implementation; adequate for exercising the import resolution behavior.

tests/specs/node/imports_slash_wildcard/main.js (1)

1-3: Main script correctly exercises #/add resolution

import { add } from "#/add"; plus console.log(add(1, 2)); is a minimal, appropriate harness to verify the #/subpath imports behavior.

tests/specs/node/imports_slash_wildcard/__test__.jsonc (1)

1-4: Verify that main.out exists and asserts the expected output

This spec test references "output": "main.out", so the harness will expect a file at tests/specs/node/imports_slash_wildcard/main.out. If this file is missing, the test will not properly assert output. Per coding guidelines, spec tests declaring an output file must provide a corresponding .out file with the expected output content using the standard matching syntax ([WILDCARD], [WILDLINE], etc.).

Confirm that main.out exists in this directory with the expected output (likely the single line 3 for console.log(add(1, 2));).


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

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

@dsherret dsherret merged commit 17cdcc2 into denoland:main Dec 8, 2025
20 checks passed
@dsherret dsherret deleted the feat_allow_subpath_imports_hash_slash branch December 8, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants