-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(ext/node): New async setInterval function to improve the nodejs compatibility #26703
Conversation
Thanks for the PR!
I would recommend adding your test as a unit test to this file: deno/tests/unit_node/timers_test.ts Lines 102 to 110 in 770ef14
Assuming you've added your test to
|
@nathanwhit I've moved the test to the appropriate directory, so the PR should be ready for merging. Please take a look when you can. Also, thanks for the tip about running directly from the target directory—it made things a lot easier! |
Signed-off-by: Bartek Iwańczuk <biwanczuk@gmail.com>
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.
LGTM, thanks!
Fixes tests added in #26703 by increasing tolerance due to noisy CI machines.
#26499
This PR adds a better implementation of
setInterval
using async functions, similar to the one in Node.js. It's mostly based on the Node.js version, with some updates to error handling and logic.The PR is complete, but I need help with:
Currently, I have a test to validate my code, but I’m running it outside of
cargo test
because running all tests at once is frustrating and often causes my system to freeze due to high memory use. This issue occurred even before my changes.