Skip to content

Conversation

@mikenomitch
Copy link
Contributor

@mikenomitch mikenomitch commented Jan 22, 2025

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)

Screenshot 2025-01-28 at 10 59 25 AM
Screenshot 2025-01-28 at 10 59 29 AM
Screenshot 2025-01-28 at 11 00 21 AM

Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a 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.

@mikenomitch mikenomitch force-pushed the mnomitch/node-changelog branch from 37f91ac to b3dc231 Compare January 24, 2025 23:45
@github-actions github-actions bot added product:workers Related to Workers product size/m and removed size/s labels Jan 24, 2025
@mikenomitch mikenomitch marked this pull request as ready for review January 24, 2025 23:46

```
</TypeScriptExample>

Copy link
Contributor

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(...));

Copy link
Member

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.

Copy link
Contributor

@vicb vicb Jan 27, 2025

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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/

Copy link
Contributor Author

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 :)

Copy link
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.

Awesome work 🎉

It will be really helpful for our users!

Added some comment inline.

@mikenomitch mikenomitch changed the title WIP - Adding changelog for node compat improvements Adding changelog for node compat improvements Jan 28, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 28, 2025

Deploying cloudflare-docs with  Cloudflare Pages  Cloudflare Pages

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

View logs

@mikenomitch mikenomitch merged commit 911aef9 into production Jan 28, 2025
12 checks passed
@mikenomitch mikenomitch deleted the mnomitch/node-changelog branch January 28, 2025 22:36
deadlypants1973 pushed a commit that referenced this pull request Jan 29, 2025
Adding changelog for node compat improvements and node API docs
kodster28 pushed a commit that referenced this pull request Jan 30, 2025
Adding changelog for node compat improvements and node API docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product:workers Related to Workers product size/m

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants