Skip to content

Enable nodejs style global timers with a compat flag.#5751

Merged
danlapid merged 1 commit intomainfrom
dlapid/nodejs_global_timers
Dec 23, 2025
Merged

Enable nodejs style global timers with a compat flag.#5751
danlapid merged 1 commit intomainfrom
dlapid/nodejs_global_timers

Conversation

@danlapid
Copy link
Copy Markdown
Collaborator

Using the compat flag "enable_nodejs_global_timers" (now experimental), users can replace the global timer functions with NodeJS style timers.

Comment thread src/node/internal/internal_timers.ts Outdated
Comment thread src/node/internal/internal_timers.ts Outdated
vicb
vicb previously requested changes Dec 23, 2025
Copy link
Copy Markdown
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see that intervals and timeouts will now adhere to Node APIs.

I think we should also handle setImmediate and clearImmediate in this PR.

ref: #5662

@vicb vicb linked an issue Dec 23, 2025 that may be closed by this pull request
@danlapid danlapid force-pushed the dlapid/nodejs_global_timers branch 2 times, most recently from d208f6f to ff010e2 Compare December 23, 2025 10:58
@danlapid danlapid requested a review from vicb December 23, 2025 10:59
@danlapid danlapid marked this pull request as ready for review December 23, 2025 11:00
@danlapid danlapid requested review from a team as code owners December 23, 2025 11:00
Using the compat flag "enable_nodejs_global_timers" (now experimental),
users can replace the global timer functions with NodeJS style timers.
@danlapid danlapid force-pushed the dlapid/nodejs_global_timers branch from ff010e2 to d51725b Compare December 23, 2025 11:03
Copy link
Copy Markdown
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dan ⏰

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 23, 2025

CodSpeed Performance Report

Merging #5751 will degrade performance by 11.59%

Comparing dlapid/nodejs_global_timers (d51725b) with main (be1dafd)

Summary

⚡ 1 improvement
❌ 1 regression
✅ 55 untouched
⏩ 34 skipped1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Efficiency
Encode_ASCII_256[TextEncoder][0/0/256] 3.5 ms 3.1 ms +12.49%
simpleStringBody[Response] 19.1 µs 21.7 µs -11.59%

Footnotes

  1. 34 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Dec 23, 2025

@vicb .. what do you mean by handling clearImmediate/setImmediate? That is already available.

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 23, 2025

@vicb .. what do you mean by handling clearImmediate/setImmediate? That is already available.

Dan added the implementation following my comment. All good now.

Comment thread src/workerd/api/node/tests/timers-global-override-test.js
@danlapid danlapid merged commit 7a5c452 into main Dec 23, 2025
21 checks passed
@danlapid danlapid deleted the dlapid/nodejs_global_timers branch December 23, 2025 18:24
Comment on lines +30 to +31
globalThis.setImmediate = setImmediate;
globalThis.clearImmediate = clearImmediate;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding the setImmediate/clearImmediate should be unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Global timer function do not match Node API in nodejs_compat mode

4 participants