-
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
feat: implement proxy support #1051
Conversation
🦋 Changeset detectedLatest commit: 4826964 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.developers.workers.dev/runs/2378532024/npm-package-wrangler-1051 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1051/npm-package-wrangler-1051 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2378532024/npm-package-wrangler-1051 dev path/to/script.js |
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.
Nice simple fix! Approving with a request and a question.
Could you add a bit of explanation to the changeset of how this fixes the problem? Something like, configures the undici library to send all requests via a proxy selected from the first non-empty environment variable from "https_proxy", "HTTPS_PROXY", "http_proxy" and "HTTP_PROXY".
I wonder if we could add a test for this? Since we mock out fetch
in our unit tests, I don't think it is possible there...
3434bd0
to
8950df1
Compare
Thanks @petebacondarwin, I've updated the changeset. Also yeah, not entirely sure how we'd test this, still trying to work that out. |
It's fine if we don't write this test I think. If we had to, we'd make another fixture app under /examples and set it up there, but it would be slow and not much return for the effort. But yay excited to unblock people behind corporate proxies! Did you have a look at proxy.ts? I think we may have to do stuff there too. It's fine if it's in a follow up PR. |
Good spot @threepointone - I think we do need to fix up to work with a proxy: const remote = connect(`https://${previewToken.host}`); It might be worth creating a manual one-off test setup with a proxy (blocking direct access to CF - if that is not ridiculously hard to do??); and then run through all the Wrangler commands to check we haven't missed anything. |
packages/wrangler/package.json
Outdated
@@ -96,7 +96,7 @@ | |||
"timeago.js": "^4.0.2", | |||
"tmp-promise": "^3.0.3", | |||
"ts-dedent": "^2.2.0", | |||
"undici": "^4.15.1", | |||
"undici": "^5.2.0", |
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.
Huh that's weird, I thought we were already at 5. Damn, nice catch.
prefer lower-case http proxy env vars
update calm-oranges-walk.md
87a5075
to
b960b39
Compare
Closing this PR as I keep finding problems while testing, and I don't want to accidentally merge it. |
Waiting until nodejs/undici#1451 gets released before we can land this PR. |
…ded" This reverts commit 338759b.
Confirming that undici v5.3.0 (https://github.com/nodejs/undici/releases/tag/v5.3.0) fixes the issues I was seeing. Running |
Closes #988