Fix timer guards to avoid TypeError under fake‐timers and polyfilled …#4213
Fix timer guards to avoid TypeError under fake‐timers and polyfilled …#4213mcollina merged 10 commits intonodejs:mainfrom
Conversation
metcoder95
left a comment
metcoder95
left a comment
There was a problem hiding this comment.
Can you please:
- Remove stylistic changes
- Add tests to cover it up
|
|
||
| let extractBody | ||
|
|
||
| async function lazyllhttp () { |
There was a problem hiding this comment.
Can you remove the styling changes?
Done! |
metcoder95
left a comment
metcoder95
left a comment
There was a problem hiding this comment.
Can you add some tests for it?
Sure! The tests have been added under |
|
@mcollina I've added comments explaining why those checks are necessary. Let me know if anything needs clarification! |
|
Can you fix the listing issues? |
|
@metcoder95 If you were referring to the linting issues, they should be resolved with the latest commit. Let me know if there’s anything else! |
|
Linting seems to still fail for the testing files you added |
|
I wonder... In deno setTimeout is returning a number because of spec compliance with the web spec. But if you want the node behavior, you should require the setTimeout from So I would actually recommend to require setTimeout fro node:timers. |
|
I still think we should require setTimeout from node:timers. |
…cific helper methods for timers, if refresh is not existing, create a new timeout for refreshTimeout
|
I modified it and I think it is now good to go. |
|
PTAL |
#4213) * Fix timer guards to avoid TypeError under fake‐timers and polyfilled environments * Remove styling changes * Add tests * Add comments * fix(lint): satisfy eslint stylistic rules for EOF, indent and jest globals * Update lib/dispatcher/client-h1.js * fix linting issue * use setTimeout from node:timers to increase chance of having node specific helper methods for timers, if refresh is not existing, create a new timeout for refreshTimeout * Apply suggestions from code review --------- Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
|
Can this be backported to v6? We're facing this issue (with jest 29) but we're still on Node 22 while it's the latest LTS. |
nodejs#4213) * Fix timer guards to avoid TypeError under fake‐timers and polyfilled environments * Remove styling changes * Add tests * Add comments * fix(lint): satisfy eslint stylistic rules for EOF, indent and jest globals * Update lib/dispatcher/client-h1.js * fix linting issue * use setTimeout from node:timers to increase chance of having node specific helper methods for timers, if refresh is not existing, create a new timeout for refreshTimeout * Apply suggestions from code review --------- Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
5 participants
This PR addresses two separate but related issues in environments where
setTimeout/setFastTimeoutmay return a numeric ID instead of a nativeTimeoutobject (e.g., Jest modern fake timers, browser/Electron polyfills). Without these guards, calls to.unref()or.refresh()throw:TypeError: this.timeout.unref is not a functioninParser#setTimeoutTypeError: fastNowTimeout.refresh is not a function(and similarly.unref()) inutil/timers.jsChanges include:
lib/dispatcher/client-h1.js–Parser#setTimeoutthis.timeout.unref()in atypeof … === 'function'guard.setTimeoutreturns a numeric ID, we skip calling.unref()and avoid the TypeError.lib/util/timers.js–refreshTimeoutfastNowTimeout.refresh()in atypeof … === 'function'check.fastNowTimeout.unref()when instantiating the timer.fastNowTimeoutis not a properTimeoutobject.Why:
.unref()/.refresh()still work as before.Testing:
Parser#setTimeoutnorutil/timers#refreshTimeoutthrows.Status