fix(dts): resolve missing types in lib.deno.ns.d.ts #31349
Conversation
|
|
WalkthroughAdded a new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/tsc/dts/lib.deno.shared_globals.d.ts (1)
499-499: Constructor spacing normalization is safe and consistent with code style updates.The
newkeyword spacing has been standardized across constructor declarations (removing space:new (...)→new(...)). This is purely stylistic—both formats are valid TypeScript. Verify this normalization is applied consistently across related.d.tsfiles mentioned in the PR.Also applies to: 517-520, 726-726, 780-780, 835-835, 859-859, 883-883
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/tsc/dts/lib.deno.ns.d.ts(18 hunks)cli/tsc/dts/lib.deno.shared_globals.d.ts(8 hunks)cli/tsc/dts/lib.deno.window.d.ts(5 hunks)cli/tsc/dts/lib.deno_web.d.ts(37 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tsc/dts/lib.deno.shared_globals.d.ts (1)
ext/node/polyfills/worker_threads.ts (1)
WorkerOptions(72-92)
⏰ 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). (7)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (16)
cli/tsc/dts/lib.deno.window.d.ts (1)
4-8: Triple‑slash path references correctly wire Deno/window/shared globalsAdding explicit
/// <reference path="./lib.deno.ns.d.ts" />,./lib.deno.shared_globals.d.ts,./lib.deno_webstorage.d.ts, and./lib.deno_cache.d.tsensures the window lib sees the Deno namespace and Web Storage/cache types without relying on implicit lib resolution. This directly supports the missing‑types fix; remaining~changes in this file are formatting-only (unions and constructor spacing) and do not alter type shapes.cli/tsc/dts/lib.deno.ns.d.ts (11)
5-11: New lib.deno_ path references align ns lib with Web/URL/Net/WebSocket typings*The added path references to
./lib.deno_url.d.ts,./lib.deno_web.d.ts,./lib.deno_fetch.d.ts,./lib.deno_net.d.ts,./lib.deno.shared_globals.d.ts, and./lib.deno_websocket.d.tsare consistent with the other libs in this folder and should resolve previously missing symbols likePerformanceMark,WebSocket,BufferSource, etc., when consuming the Deno namespace. Cyclic refs with other lib.deno_*.d.ts are acceptable for TS and will be validated by the usual typecheck pipeline.
153-195:DenoSystemError+ OS error classes exposingcode?: stringlook correctDefining
export interface DenoSystemError extends Error { code?: string; }and having OS‑level error classes (e.g.,NotFound,PermissionDenied,ConnectionRefused, etc.)extends Error implements DenoSystemError { code?: string; }cleanly models the runtime behavior where OS errno codes are surfaced ascode. This is backwards‑compatible (property is optional) and significantly improves typings for Node‑style error handling.
280-389: Other Deno.errors changes are type‑only and safeThe rest of the
Deno.errorsupdates here (addingimplements DenoSystemErrorplus optionalcodeon other OS‑backed errors, and leaving purely logical errors likeInvalidData,BadResource,Http,NotCapableas plainError) are consistent and non‑breaking. The closingexport { };is a cosmetic normalization fromexport {}and doesn’t affect consumers.
3583-3597: FsEvent.kind union reflow is cosmeticReformatting
FsEvent.kindinto a multi‑line string‑literal union ("any" | "access" | "create" | ... | "other") doesn’t change the allowed values and keeps the type precise. No behavioral or compatibility impact.
4204-4222: SysPermissionDescriptor.kind union remains precise after formatting changeThe multi‑line union for
SysPermissionDescriptor.kind("loadavg" | "hostname" | ... | "setPriority") preserves the exact same set of string literals; only layout changed. Safe and readable.
4519-4529: Deno.build.os string union formatting change is benignReflowing the
Deno.build.osunion into one per line ("darwin" | "linux" | ... | "illumos") is a style-only change; the OS identifiers remain identical to the previous definition.
5789-5910: FFI helper conditional types: tokens unchanged, just rewrappedThe
ToNativeType,ToNativeResultType,ToNativeParameterTypes,FromNativeType,FromNativeResultType, andFromNativeParameterTypesaliases are only reformatted over multiple lines; the nested conditional branches and inferred enum/typed‑pointer cases are otherwise identical. Given their centrality to FFI, it’s good that the change is purely cosmetic.
5965-5999:StaticForeignSymboland related FFI mapping types are unaffected logicallyThe
StaticForeignSymbol,FromForeignFunction, andStaticForeignLibraryInterfacedefinitions retain the same conditional logic and mapped‑type behavior; only spacing and line breaks changed (includingexport { };normalization). No impact on FFI call-site types.
6195-6232: UnsafeCallback changes are purely layoutThe
UnsafeCallbackgeneric declaration and itsthreadSafeconstructor signature are unchanged aside from line breaks; lifetime and ref/unref semantics are unaffected. Given how sensitive FFI callbacks are, it’s good there’s no behavioral/type change here.
6537-6575: Deno.telemetry namespace export sentinel is style‑onlyThe added
export { }; // only export exportsline inDeno.telemetryjust normalizes the empty export style and doesn’t alter the exposed API (tracerProvider,contextManager,meterProvider). Safe to merge.
6577-6578: Finalexport { };sentinel is harmlessThe terminal
export { }; // only export exportsinside theDenonamespace is equivalent to the priorexport {}usage and simply preserves module semantics for tooling. No behavioral impact.cli/tsc/dts/lib.deno_web.d.ts (2)
7-7: New reference to lib.deno.window.d.ts completes Web/window type wiringAdding
/// <reference path="./lib.deno.window.d.ts" />here ensures the web lib has direct access to the window/navigator/location (and related event/stream) declarations defined inlib.deno.window.d.ts, which in turn now references the ns/shared globals libs. This matches the PR’s goal of fixing missing global/Web API typings in the Deno namespace.
42-1696: Constructor/union formatting tweaks across web lib are cosmeticAll the
~changes across this file (constructor signatures likenew(): Xornew(label?: string, ...), union types reflowed over multiple lines, and controller/streamnew(): neverdeclarations) preserve the same parameter lists, return types, and literal unions as before. There are no observable type‑shape or API surface changes—just normalization of spacing.cli/tsc/dts/lib.deno.shared_globals.d.ts (2)
1-1: Flag: AI-generated summary describes types not present in this file.The summary mentions
DenoSystemErrorinterface and OS-level error classes implementing it, but these do not appear in the provided file. This inconsistency suggests the summary may conflate changes from multiple files. Cross-reference the PR description with actual file changes to ensure alignment.
8-16: All referenced declaration files exist and paths resolve correctly.The reference directive conversion from implicit
lib=to explicitpath=references is valid. All 9.d.tsfiles are present in thecli/tsc/dtsdirectory and paths resolve as expected. The constructor formatting changes (removing spaces afternewkeyword) are consistent stylistic improvements with no functional impact.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
repro_issue.ts (1)
5-9: Update repro comments and remove obsolete@ts-ignoreGiven
Deno.errors.NotFoundnow hascode?: stringin the typings, the@ts-ignoreon Line 5 and the comment on Line 8 ("should fail type checking currently") are outdated. The line should type-check successfully after your changes.Recommend:
- Drop the
@ts-ignoreon Line 5.- Rephrase the Line 8 comment to something like: “This line used to fail type checking before the fix; it should now compile and see
codeas an optional string.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
check_ns.ts(1 hunks)check_tsconfig.json(1 hunks)cli/tsc/dts/lib.deno.ns.d.ts(8 hunks)cli/tsc/dts/lib.deno.shared_globals.d.ts(1 hunks)cli/tsc/dts/lib.deno.window.d.ts(1 hunks)cli/tsc/dts/lib.deno_web.d.ts(1 hunks)repro_issue.ts(1 hunks)tests/unit/error_test.ts(1 hunks)tests/unit/read_file_test.ts(3 hunks)verify_fix.js(1 hunks)verify_fix.ts(1 hunks)verify_tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
verify_fix.ts (1)
verify_fix.js (2)
c(4-4)cs(5-5)
repro_issue.ts (1)
cli/tsc/dts/lib.deno.ns.d.ts (1)
NotFound(192-194)
🔇 Additional comments (7)
cli/tsc/dts/lib.deno.window.d.ts (1)
4-8: Reference structure looks correct.The path-based references establish proper dependencies on the Deno namespace and shared globals before loading specialized APIs (webstorage, cache). The ordering is appropriate.
cli/tsc/dts/lib.deno_web.d.ts (1)
7-7: Appropriate reference addition.Adding the window type reference here allows web API types to properly resolve Window-related interfaces. This aligns with the PR's goal of fixing missing Web API type detection.
cli/tsc/dts/lib.deno.shared_globals.d.ts (1)
8-16: All referenced files exist and are properly structured.Verification confirms all 10 referenced .d.ts files exist in the same directory. The circular reference between lib.deno.shared_globals.d.ts and lib.deno.ns.d.ts (at lines 16 and 9 respectively) is a standard, safe pattern in TypeScript declaration files and poses no issues for type resolution. The conversion from ambient to explicit path-based references should resolve the intended type resolution issues.
cli/tsc/dts/lib.deno.ns.d.ts (2)
152-163: LGTM! Well-designed interface for OS-level errors.The DenoSystemError interface with optional
codeproperty cleanly separates OS-level errors (which may include system error codes like "ENOENT") from Deno-specific errors. The documentation is clear and the design allows backward compatibility.
192-375: Correct implementation of DenoSystemError across error classes.The error classes are properly categorized:
- OS-level errors (NotFound, PermissionDenied, ConnectionRefused, etc.) correctly implement DenoSystemError with optional
code?: string- Deno-specific errors (InvalidData, BadResource, NotCapable, etc.) correctly remain as plain Error without the code property
This matches the intended behavior described in the tests.
tests/unit/read_file_test.ts (1)
167-171: Good type-safe approach to accessing the optional code property.Using type guards before accessing the
codeproperty is the correct pattern. This ensures type safety and handles cases where the error might be of an unexpected type. The changes properly align with the new DenoSystemError interface.Also applies to: 182-186
tests/unit/error_test.ts (1)
36-68: Comprehensive test coverage for the DenoSystemError interface.The test properly validates:
- OS-level errors (NotFound, PermissionDenied, ConnectionRefused) have the optional
codeproperty- Deno-specific errors (InvalidData, BadResource, NotCapable) do not have the
codeproperty- The
@ts-expect-errorcomments provide compile-time validationThis ensures the type system correctly reflects the intended design.
verify_tsconfig.json
Outdated
| { | ||
| "compilerOptions": { | ||
| "target": "esnext", | ||
| "lib": [ | ||
| "esnext" | ||
| ], | ||
| "types": [], | ||
| "skipLibCheck": true | ||
| }, | ||
| "files": [ | ||
| "./verify_fix.ts" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Missing "dom" lib in configuration.
The verify_fix.ts file uses Cache and CacheStorage, which are DOM types, but this config doesn't include "dom" in the lib array. While the triple-slash reference in verify_fix.ts might work, this creates an inconsistency with check_tsconfig.json which explicitly includes the "dom" lib.
Also note the different skipLibCheck value (true here vs false in check_tsconfig.json) - if both are verification/test configs, consider making them consistent.
Apply this diff:
{
"compilerOptions": {
"target": "esnext",
"lib": [
- "esnext"
+ "esnext",
+ "dom"
],
"types": [],
- "skipLibCheck": true
+ "skipLibCheck": false
},
"files": [
"./verify_fix.ts"
]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "compilerOptions": { | |
| "target": "esnext", | |
| "lib": [ | |
| "esnext" | |
| ], | |
| "types": [], | |
| "skipLibCheck": true | |
| }, | |
| "files": [ | |
| "./verify_fix.ts" | |
| ] | |
| } | |
| { | |
| "compilerOptions": { | |
| "target": "esnext", | |
| "lib": [ | |
| "esnext", | |
| "dom" | |
| ], | |
| "types": [], | |
| "skipLibCheck": false | |
| }, | |
| "files": [ | |
| "./verify_fix.ts" | |
| ] | |
| } |
🤖 Prompt for AI Agents
In verify_tsconfig.json around lines 1 to 13, the config omits the "dom" lib and
has skipLibCheck: true causing inconsistency with check_tsconfig.json; update
the "lib" array to include "dom" (e.g. ["esnext","dom"]) and set "skipLibCheck"
to the same value as check_tsconfig.json (change to false) so DOM types like
Cache and CacheStorage resolve consistently.
kantrolv
force-pushed
the
missingtypes
branch
2 times, most recently
from
bf474c4 to
f699ff6
Compare
There was a problem hiding this comment.
Why are tests in this file changed?
There was a problem hiding this comment.
Why are tests in this file changed?
I've reverted the read_file_test.ts changes. The PR now only contains type definitions in lib.deno.ns.d.ts and type-level tests in error_test.ts. Thanks for the feedback! Let me know if there is anything I can do.
kantrolv
force-pushed
the
missingtypes
branch
from
dfcdc4f to
ec034a5
Compare
Added code?: string property to all error classes implementing DenoSystemError. This allows TypeScript to accept accessing the code property on error instances without type errors.
kantrolv
force-pushed
the
missingtypes
branch
from
c1aa8d8 to
3538226
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/tsc/dts/lib.deno.ns.d.ts(17 hunks)
⏰ 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). (2)
- GitHub Check: test debug linux-x86_64
- GitHub Check: build libs
🔇 Additional comments (1)
cli/tsc/dts/lib.deno.ns.d.ts (1)
5-10: LGTM - Reference directives updated correctly.The reference directives now use relative paths (
./), which aligns with the PR objectives to ensure correct type resolution. Lines 9-10 specifically add the references mentioned in the PR description.
cli/tsc/dts/lib.deno.ns.d.ts
Outdated
| /// <reference path="./lib.deno_net.d.ts" /> | ||
| /// <reference path="./lib.deno_url.d.ts" /> | ||
| /// <reference path="./lib.deno_web.d.ts" /> | ||
| /// <reference path="./lib.deno_fetch.d.ts" /> | ||
| /// <reference path="./lib.deno_websocket.d.ts" /> | ||
| /// <reference path="./lib.deno.shared_globals.d.ts" /> |
There was a problem hiding this comment.
Critical inconsistency between PR description and actual changes.
The PR objectives state this change adds reference directives to resolve missing types (lines 5-10 show these reference additions). However, the bulk of this diff (lines 174-342) introduces a completely unrelated DenoSystemError interface and modifies 21+ error classes to implement it.
This suggests either:
- The wrong diff was uploaded for review
- The PR description is incorrect
- Multiple unrelated changes were accidentally committed together
Lines 5-10 match the PR description, but lines 174-342 are a major error system refactoring that has nothing to do with "resolving missing types in lib.deno.ns.d.ts via reference directives."
Please clarify: Is this the correct diff for PR #31349, or have unrelated changes been mixed in?
Also applies to: 174-342
🤖 Prompt for AI Agents
In cli/tsc/dts/lib.deno.ns.d.ts around lines 5-10 and 174-342, the PR adds
reference directives at lines 5-10 (matching the description) but also
introduces an unrelated DenoSystemError interface and modifies many error
classes at lines 174-342; this is a mixed-change issue — either remove the
unrelated error-system refactor from this PR or split it into a separate PR and
update the commit/branch accordingly. Revert the DenoSystemError/interface and
error-class changes from this branch (or move those commits to a new branch/PR),
keep only the reference directive changes here, and update the PR
title/description to accurately reflect the contained change; if you intend to
keep both, split into two logical commits with clear descriptions and update the
review to reference both PRs.
Added code?: string property to all error classes implementing DenoSystemError. This allows TypeScript to accept accessing the code property on error instances.
3 participants
fix(dts): resolve missing type references in
lib.deno.ns.d.tsOverview
This PR resolves multiple type resolution issues in Deno’s TypeScript declaration files, where global and Web API types such as
PerformanceMark,WebSocket, andBufferSourcewere not being detected. These missing type references caused incomplete or incorrect typing within theDenonamespace.Changes
Added missing reference directives to
cli/tsc/dts/lib.deno.ns.d.ts:/// <reference path="lib.deno.shared_globals.d.ts" />/// <reference path="lib.deno_websocket.d.ts" />Updated path references in:
cli/tsc/dts/lib.deno.shared_globals.d.tscli/tsc/dts/lib.deno.window.d.tsConverted
libreferences to relative paths to ensure correct type resolution without relying on compiler defaults.Fixes
#22807