-
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
[WIP] pages: Fix "worker called response.clone()" warnings in `wrangler pages dev #4861
base: main
Are you sure you want to change the base?
Conversation
|
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/7699751320/npm-package-wrangler-4861 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/4861/npm-package-wrangler-4861 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7699751320/npm-package-wrangler-4861 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7699751320/npm-package-create-cloudflare-4861 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7699751320/npm-package-miniflare-4861 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7699751320/npm-package-cloudflare-pages-shared-4861 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
Whilst solving one issue, this appears to be a breaking change currently: <!-- public/index.html -->
<form action="/submit" method="post">
<input type="text" name="name" placeholder="Jane Doe" />
<button type="submit">Submit</button>
</form> // functions/_middleware.ts
export const onRequestPost = async ({ request, next }) => {
const formData = await request.formData();
console.log("log", Object.fromEntries(formData.entries()));
return next();
}; // functions/submit.ts
export const onRequestPost = async ({ request }) => {
const formData = await request.formData();
return Response.json(Object.fromEntries(formData.entries()));
}; This code works with the latest version of wrangler, but does not using the pre-release from this PR. |
@Cherry thanks a lot, you're totally right, we already internally discussed this. I think the solution will be to remove the cloning there only when there is a single handler removing this error/warning for just some cases, not ideal but the best solution we can currently implement without introducing a breaking change Then later in a major bump or whenever possible we should remove the cloning altogether, leaving the responsibility of doing the cloning to the user instead of Pages doing it for them under the hood (so basically your example would break and you'd have to do the cloning of the request yourself to make it work) What do you think? |
This makes sense to me, thanks @dario-piotrowicz. I totally agree that it should be the responsibility of the developer to clone the Request as needed, but also agree that this needs to be a new major version before landing. I imagine something like this could work in the short-term: |
@Cherry ah yeah thanks a lot for the suggestion! Somehow your suggestion didn't show up in our discussion 😅, it looks great, I feel like that should work nicely, when I get to this I will look into it and test it out! Thanks a lot! 🙂👍 |
Is there anything blocking merging this and bumping the major version? (Using wrangler in a pages project w/ middleware is basically unusable due to all the warnings.) |
Fixes #3259
What this PR solves / how to test:
Author has addressed the following:
Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr review
so future reviewers can take inspiration and learn from it.