-
Notifications
You must be signed in to change notification settings - Fork 734
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
Update local AI fetcher to forward method and pathname to upstream #7315
Update local AI fetcher to forward method and pathname to upstream #7315
Conversation
🦋 Changeset detectedLatest commit: 931acf7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
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/11971817665/npm-package-wrangler-7315 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7315/npm-package-wrangler-7315 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-wrangler-7315 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-create-cloudflare-7315 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-kv-asset-handler-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-miniflare-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-pages-shared-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-vitest-pool-workers-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-workers-editor-shared-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-workers-shared-7315 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11971817665/npm-package-cloudflare-workflows-shared-7315 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
33e5ba9
to
e80448f
Compare
e80448f
to
6346622
Compare
a45b81d
to
77d5dfe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no test you can update or create for this behaviour?
@petebacondarwin sure, let me add a test and get back to you |
4c82497
to
7e38dd1
Compare
6136571
to
8d93ebc
Compare
Hey @petebacondarwin I've just updated the pr with new tests and new header name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests!
Sorry if I sent you off on a trip with that header name.
Please can you check it is valid and what you actually need on the server-side.
packages/wrangler/src/ai/fetcher.ts
Outdated
const reqHeaders = new Headers(request.headers); | ||
reqHeaders.delete("Host"); | ||
reqHeaders.delete("Content-Length"); | ||
reqHeaders.set("X-Forwarded-Host", request.url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear I am not sure that X-Forwarded-Host
is the correct thing here. It is what I was thinking of but I think is only for hosts not full URLs.
Do you have a requirement for this header on the server?
What is the header it is looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm writing the server part as well, so we are free to decide what we like best 😄
the only requirement i have is for this local fetcher to send the full path and query string to the server, so any header name does the job
Still LGTM! |
This pr starts forwarding method and url to upstream.
This is not a breaking change, because the AI binding currently just does POST requests.
workerd here