-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Adding changelog for node compat improvements #19341
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
Conversation
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.
1 files reviewed, 2 total issue(s) found.
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
37f91ac to
b3dc231
Compare
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| ``` | ||
| </TypeScriptExample> | ||
|
|
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.
I think we should callout that the node:timers are "not interoperable" with the global timer functions because this behavior is different from Node.
We could also say "not fully interoperable" but we would then need to give details on what is vs what is not which could bring confusion.
For example (not sure we want to add this here):
import timers from 'node:timer';
// no-op, the timer is not cancelled
clearTimeout(timers.setTimeout(...));
// the timer is cancelled
timers.clearTimeout(setTimeout(...));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.
I think we should callout that the node:timers are "not interoperable" with the global timer functions because this behavior is different from Node.
IMHO: This should be mentioned in nodejs docs about node:timers, not in cloudflare.
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.
Well that is the exact issue here, this problem is specific to Cloudflare and does not exist on Node
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.
That's why I think we should add a callout - we are not compatible with Node.js here.
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.
Do we have a thread on this generally? I'm not sure I fully understand what's going on here.
On the example provided, so in a Node.js environment the timer could be cancelled in the first example clearTimeout(timers.setTimeout(...)); but for us it is not cancelled?
If so, yeah I think that deserves a callout if behavior differs from Node.js.
Any other cases to mention?
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.
On the example provided, so in a Node.js environment the timer could be cancelled in the first example
clearTimeout(timers.setTimeout(...));but for us it is not cancelled?
Your understanding is correct.
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.
setTimeout, setImmediate and setInterval doesn't return Timeout/Immediate classes on cloudflare because the web spec says that it should return an ID. Whereas, Node.js doesn't implement this. But this fact doesn't change with the new changes. We are not changing the existing behavior, we are just adding new modules. I don't think this changelog is the place to list every single differences that we have.
I recommend keeping this post as simple as possible, and just mention the behavior that we have added. IMHO: Other information should be mentioned in the documentation (not on changelog)
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.
Okay, I updated the node:timers docs to include a Note here. I didn't go super in depth though FWIW.
Kept out of the changelog since this changelog post is already super long and felt docs-y.
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.
Oh... You probably got confused by the title of the PR (can it be updated?)
But we are now commenting on the documentation for the timers module, similar to https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/
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.
Oh, no I gotcha - Just being overly clear on where I added it :)
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
vicb
left a 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.
Awesome work 🎉
It will be really helpful for our users!
Added some comment inline.
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
src/content/changelogs-next/2025-01-22-nodejs-compat-improvements.mdx
Outdated
Show resolved
Hide resolved
Deploying cloudflare-docs with
|
| Latest commit: |
b52cf38
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68c49ea0.cloudflare-docs-7ou.pages.dev |
| Branch Preview URL: | https://mnomitch-node-changelog.cloudflare-docs-7ou.pages.dev |
Adding changelog for node compat improvements and node API docs
Adding changelog for node compat improvements and node API docs
Adds Changelog for new Node APIs and docs for them
Summary
Adds a changelog for 3 additional node modules we now support and related docs.
Screenshots (optional)