-
Notifications
You must be signed in to change notification settings - Fork 5.8k
fix(node): make Server.setTimeout a no-op to match Node.js #31332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughReplaces the previous "Not implemented" error in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Fastify / user code
participant Server as ServerImpl.setTimeout()
Note over Server: Previously threw "Not implemented" error
Caller->>Server: setTimeout(msecs?, callback?)
Server-->>Caller: returns ServerImpl (this) %% no-op shim replaces error %%
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ext/node/polyfills/http.ts (2)
2160-2163: LGTM! Implementation correctly achieves the PR objective.The no-op implementation eliminates the console warning while maintaining method chaining by returning
this. The parameter names with underscore prefixes appropriately signal they're intentionally unused.Consider adding an explicit return type annotation for clarity:
- setTimeout(_msecs?: number, _callback?: () => void) { + setTimeout(_msecs?: number, _callback?: () => void): this { // No-op for Node.js compatibility return this; }
2165-2165: Minor: Extra blank line.The additional blank line at this location appears unnecessary, though this is a minor stylistic detail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/polyfills/http.ts(1 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). (9)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release macos-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
b01435f to
2366ab6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/polyfills/http.ts (1)
2160-2163: LGTM! Successfully eliminates console warnings while maintaining API compatibility.The no-op implementation correctly:
- Returns
thisfor method chaining (matches Node.js)- Accepts the expected parameters
- Suppresses the previous "Not implemented" error
Optional enhancement: In Node.js, when
callbackis provided, it's registered as a 'timeout' event listener viathis.on('timeout', callback). While not required for the current use case (suppressing Fastify warnings), registering the callback would improve compatibility:setTimeout(_msecs?: number, _callback?: () => void) { // No-op for Node.js compatibility + if (_callback) { + this.on('timeout', _callback); + } return this; }However, since the PR explicitly aims for a no-op and frameworks like Fastify work without this, the current implementation is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/polyfills/http.ts(1 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). (8)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
|
I don't think silencing the error does any justice here. In node's implementation, the |
This PR updates the Node.js HTTP polyfill so that
Server.setTimeout()behaves as a no-op, matching Node.js behavior.Currently, Deno prints:
Not implemented: Server.setTimeout()
when frameworks like Fastify call this method internally.
This warning is unnecessary and creates console noise even though the call is harmless.
Related Issue
Fixes #31221
Details
In Node.js, calling
server.setTimeout()without arguments is a no-op.The previous Deno polyfill printed a warning instead of following this behavior.
This PR changes the method in:
deno/ext/node/polyfills/http.ts
from: