fix(check): regression with tsgo and node globals by dsherret · Pull Request #31621 · denoland/deno

dsherret · 2025-12-15T21:19:57Z

coderabbitai · 2025-12-15T21:20:14Z

Walkthrough

This pull request optimizes node source file detection in the TypeScript compiler and adds test coverage for Deno's timer behavior. The isNodeSourceFile callback now includes a fast path that immediately returns true for paths starting with asset:///node/, while preserving existing resolution logic for other paths. A new test specification and test file are added to validate Deno.unrefTimer functionality with setTimeout, ensuring timers can prevent process exit when configured.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(check): regression with tsgo and node globals' directly describes the main fix—addressing a tsgo regression related to node globals—which aligns with the changeset's fix to the isNodeSourceFile logic.
Description check	✅ Passed	The description references issue #31562, which documents the exact problem being fixed (tsgo using wrong setTimeout global), making it directly relevant to the changeset.
Linked Issues check	✅ Passed	The code changes directly address the regression: the asset:/// prefix check in isNodeSourceFile ensures node globals are correctly identified, fixing the setTimeout incompatibility with Deno.unrefTimer described in issue #31562.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the tsgo node globals regression: the logic fix in cli/tsc/go.rs and the test files in tests/specs/check/set_timeout_deno_unref_timer/ are both directly related to validating the fix.

✨ 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 cc34594 and 0424944.

📒 Files selected for processing (3)

cli/tsc/go.rs (1 hunks)
tests/specs/check/set_timeout_deno_unref_timer/__test__.jsonc (1 hunks)
tests/specs/check/set_timeout_deno_unref_timer/main.ts (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/check/set_timeout_deno_unref_timer/main.ts

**/*.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:

cli/tsc/go.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:

cli/tsc/go.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/check/set_timeout_deno_unref_timer/__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/check/set_timeout_deno_unref_timer/__test__.jsonc

🧠 Learnings (2)

📚 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/check/set_timeout_deno_unref_timer/__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 : Spec tests should be created in `tests/specs/` with a `__test__.jsonc` file describing test steps and input files

Applied to files:

tests/specs/check/set_timeout_deno_unref_timer/__test__.jsonc

⏰ 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-aarch64
GitHub Check: test debug macos-x86_64
GitHub Check: lint debug windows-x86_64
GitHub Check: lint debug macos-x86_64
GitHub Check: lint debug linux-x86_64
GitHub Check: build libs

🔇 Additional comments (3)

tests/specs/check/set_timeout_deno_unref_timer/main.ts (1)

1-1: LGTM!

Clean test case that directly validates the fix for the setTimeout type mismatch issue.

tests/specs/check/set_timeout_deno_unref_timer/__test__.jsonc (1)

1-15: LGTM!

Good test coverage with both tsgo and tsc variants. Correctly expects empty output to verify no type errors occur.

cli/tsc/go.rs (1)

510-523: LGTM!

The fast path for asset:///node/ correctly identifies node built-in assets without unnecessary parsing. Existing resolution logic is preserved for other paths.

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

nathanwhit

LGTM

fix(check): regression with tsgo and node globals

0424944

bartlomieju requested a review from nathanwhit December 15, 2025 22:16

nathanwhit approved these changes Dec 15, 2025

View reviewed changes

dsherret merged commit 5396fed into denoland:main Dec 15, 2025
20 checks passed

dsherret deleted the fix_tsgo_node_globals branch December 15, 2025 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(check): regression with tsgo and node globals#31621

fix(check): regression with tsgo and node globals#31621
dsherret merged 1 commit intodenoland:mainfrom
dsherret:fix_tsgo_node_globals

dsherret commented Dec 15, 2025

Uh oh!

coderabbitai Bot commented Dec 15, 2025 •

edited

Loading

Uh oh!

nathanwhit left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dsherret commented Dec 15, 2025

Uh oh!

coderabbitai Bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks and finishing touches

Uh oh!

nathanwhit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Dec 15, 2025 •

edited

Loading