-
Notifications
You must be signed in to change notification settings - Fork 658
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
In pages functions avoid cloning the request when not necessary #4916
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b489e23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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/7777149625/npm-package-wrangler-4916 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/4916/npm-package-wrangler-4916 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7777149625/npm-package-wrangler-4916 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7777149625/npm-package-create-cloudflare-4916 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7777149625/npm-package-miniflare-4916 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7777149625/npm-package-cloudflare-pages-shared-4916 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
32e5d22
to
e6ee10e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4916 +/- ##
==========================================
+ Coverage 70.62% 70.69% +0.06%
==========================================
Files 292 292
Lines 15187 15187
Branches 3865 3865
==========================================
+ Hits 10726 10736 +10
+ Misses 4461 4451 -10 |
99ffb58
to
d9c58ad
Compare
d9c58ad
to
b489e23
Compare
I wonder if we could use Request.bodyUsed alongside Request.body.cancel() to remove the warning in all cases, by cancelling the body streams of any requests where its not been read? |
@Skye-31 that's an interesting idea. I think it would make the warnings go away (because the clones would be cleaned up right before the warning logic sees them), but unfortunately I think the underlying issue that is being warned about would remain. You'd still be calling |
There's two things in one here:
|
Partially fixes #3259 (the full fix can only be applied as a breaking change so it'll have to wait for a wrangler major release unfortunately)
Supersedes #4861
Example for the changes: https://github.com/dario-piotrowicz/cf-pages-functions-request-clone-repro
TODO: