-
Notifications
You must be signed in to change notification settings - Fork 772
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
miniflare: fix node 21.7.0 compatibility #5201
miniflare: fix node 21.7.0 compatibility #5201
Conversation
The ReadableStream constructor was being called with an invalid, numerical second argument. Node 21.7.0 now runs stricter checks on that parameter, which was causing construction to fail. We eliminate the second argument. Fixes cloudflare#5190
🦋 Changeset detectedLatest commit: e8f1492 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
Co-authored-by: MrBBot <me@mrbbot.dev>
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/8205333453/npm-package-wrangler-5201 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5201/npm-package-wrangler-5201 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-wrangler-5201 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-create-cloudflare-5201 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-cloudflare-kv-asset-handler-5201 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-miniflare-5201 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-cloudflare-pages-shared-5201 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8205333453/npm-package-cloudflare-vitest-pool-workers-5201 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5201 +/- ##
==========================================
+ Coverage 70.35% 70.40% +0.05%
==========================================
Files 298 298
Lines 15567 15567
Branches 4007 4007
==========================================
+ Hits 10952 10960 +8
+ Misses 4615 4607 -8 |
Can we get a new wrangler release with this change included? :) |
We do regular releases of workers-sdk packages on Tues and Thurs. So you should get one today. |
#5202 tracks running tests on the latest version of Node. |
The fix is out and this is working! |
The
ReadableStream
constructor was being called with an invalid, numerical second argument. Node 21.7.0 now runs stricter checks on that parameter, which was causing construction to fail.We eliminate the second argument.
What this PR solves / how to test
Fixes #5190
Author has addressed the following
We should ensure that tests are run against Node v
21.7.0
.Wrangler's tests are intentionally run on the lowest supported Node version for Wrangler, which is currently 16.x. Since Node 16 is EOL, we're planning to bump Wrangler's lowest supported Node version to 18.x in the near future, at which point we'll update the CI tests to run in Node 18. When we do that we should make sure we run some tests on the latest LTS.
This is tracked in #5202