Skip to content
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

remove functions that are removed from node:util #7942

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jan 28, 2025

Removes methods that are removed from node.js


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: they are removed from node.js
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: removing deprecated/removed functions
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: they're removed from node.js

@anonrig anonrig requested review from a team as code owners January 28, 2025 15:54
Copy link

changeset-bot bot commented Jan 28, 2025

🦋 Changeset detected

Latest commit: 58f34f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/unenv-preset Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@anonrig anonrig force-pushed the yagiz/remove-removed-util-methods branch from ee2e57c to 42bbdaa Compare January 28, 2025 15:55
@anonrig anonrig changed the title remove removed functions from node:util polyfills remove functions that are removed from node:util Jan 28, 2025
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Copy link
Contributor

github-actions bot commented Jan 28, 2025

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-wrangler-7942

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7942/npm-package-wrangler-7942

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-wrangler-7942 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-workers-bindings-extension-7942 -O ./cloudflare-workers-bindings-extension.0.0.0-v7f7716a6d.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v7f7716a6d.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-create-cloudflare-7942 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-kv-asset-handler-7942

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-miniflare-7942

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-pages-shared-7942

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-unenv-preset-7942

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-vite-plugin-7942

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-vitest-pool-workers-7942

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-workers-editor-shared-7942

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-workers-shared-7942

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13014632501/npm-package-cloudflare-workflows-shared-7942

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.106.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250124.0
workerd 1.20250124.0 1.20250124.0
workerd --version 1.20250124.0 2025-01-24

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@Cherry
Copy link
Contributor

Cherry commented Jan 28, 2025

Is there a documented backwards compatibility strategy for nodejs_compat? If I have a worker in production with this:

import { isNumber } from 'node:util';

export default {
	async fetch(request, env, ctx): Promise<Response> {
		const bar = Math.random() > 0.5 ? 1 : 'foo';
		if (isNumber(bar)) {
			return new Response('Number!');
		}
		return new Response('Hello World!');
	},
} satisfies ExportedHandler<Env>;

which works completely fine with nodejs_compat, will it break after this lands in the latest version of wrangler and I go to redeploy? In the latest version of Node.js 22, util.isNumber('cd') works without issue, though is indeed marked as deprecated.

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.

Let's wait before merging this...

2 reasons:

  • First use the preset in this repo matching unenv and start diverging later
  • We have at least one user telling us he is using the API

Idea: maybe we can add a flag (--disable-node-deprecated-apis-v18) or something.

(Requesting changes to make sure this doesn't get in too early)

@anonrig
Copy link
Member Author

anonrig commented Jan 28, 2025

which works completely fine with nodejs_compat, will it break after this lands in the latest version of wrangler and I go to redeploy? In the latest version of Node.js 22, util.isNumber('cd') works without issue, though is indeed marked as deprecated.

You will not have any issues when you redeploy. If you use a specific version of wrangler it will always come with the polyfill. On another hand, if you update wrangler, then you'll just see that isNumber doesn't exist anymore.

These functions are removed on April 29, 2024: nodejs/node#52744

@Cherry
Copy link
Contributor

Cherry commented Jan 28, 2025

You will not have any issues when you redeploy. If you use a specific version of wrangler it will always come with the polyfill. On another hand, if you update wrangler, then you'll just see that isNumber doesn't exist anymore.

These functions are removed on April 29, 2024: nodejs/node#52744

If I'm understanding correctly, these will be removed in Node.js 23, and therefore any future versions as well. This makes sense as it's a new major version bump, and breaking changes are expected.

wrangler / nodejs_compat wouldn't have that here, so expecting these functions to disappear in a minor version would not be a great DX. This is why I was asking primarily if there was a documented strategy around nodejs_compat versioning and backwards compatibility.

@jasnell
Copy link
Member

jasnell commented Jan 28, 2025

hmmm... I think we need to carefully think about this. If these were implemented in the runtime we would need to keep them around forever with or without a compatibility flag. I think breaking changes in polyfill implementations that we choose to bundle are fundamentally different from ones that the user chooses to bundle and the burden of support is significantly higher.

Instead of removing these like this, it would likely be a much better idea to noisily deprecate them for a few versions of wrangler, detecting when they are used and complaining loudly for a while before dropping them.

@petebacondarwin
Copy link
Contributor

@Cherry makes a good point. This is actually a breaking change to Wrangler so should be rolled out in v4?

@petebacondarwin petebacondarwin added the breaking change Change that will result in breaking existing behavior label Jan 29, 2025
@petebacondarwin petebacondarwin marked this pull request as draft January 29, 2025 08:37
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Let's hold on this removal and follow the path suggested by @vicb.

getSystemErrorMap,
getSystemErrorName,
isArray,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: isArray is not removed from Node and the timeline to remove the other functions is Node 23.

Copy link
Member Author

Choose a reason for hiding this comment

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

isArray is available and implemented on workerd

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have commented on l94 instead.
What I meant is that isArray is not part of the functions removed in Node23.

But if we have some of the functions here implemented in workerd (and not going away) we should definitely use the workerd version instead - I guess it will be a different PR.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change that will result in breaking existing behavior
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants