-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix(release): allow downloading binary via proxy again #407
Conversation
33a3acd
to
9d06ed6
Compare
9d06ed6
to
9c5c2c3
Compare
@theoludwig @mstruebing let me know what you think! I tested this already in our corporate enviornment and should work fine. |
9c5c2c3
to
f818f96
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.
Thank you for the PR. 🚀
Mostly looks good to me.
I will try to test it locally, this afternoon.
We might find a way to refactor to avoid the any
and maybe we can directly use the built-in Node.js fetch (which uses undici
under the hood), but that's not a blocker to merge and release this if we can't do the refactor. 😄
Thanks for the quick review @theoludwig! Ah ok, I'm just not sure if the bundled undici exposes ProxyAgent, so I just imported fetch explicitly along with it. I still need to dive a bit more into the ecosystem though :) Also apologies if I'm mixing styles a bit here. just noticed and pushed, maybe needs more linting for people like me 😅 |
@nejch I refactored the code, to avoid the About using Node.js built-in fetch for
No worries, I formatted the code with Prettier, and now, it is also verified with CI. |
🎉 This PR is included in version 5.1.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Co-authored-by: Théo LUDWIG <[email protected]>
What changes this PR introduce?
I managed to find a way to test this, plus using the new undici methods and agents. This coincidentally solves #402, since the built javascript is back to 1MB.
The new undici/octokit has absolutely no support for standard environment variables for proxies, so I had to add a rather old
proxy-from-env
library, but in the end I realized that's what the old proxy-agent library uses anyway - and in fact it's a solved problem so I don't see it being from 2020 a real issue.I reused octokit's approach to proxy testing with a real little proxy server, this should help if people refactor code to still tes something close to a real scenario.
Edit: since I added some real tests, this refactors the naming of the script to be a bit more accurate.
I also had to bump tsx due to CI failures (see e.g. redwoodjs/redwood#9653)
List any relevant issue numbers
Closes #405
Closes #402 (node-fetch and proxy-agent replaced with undici's implementations)
Is there anything you'd like reviewers to focus on?
Not sure if we should add a note to not delete those tests to avoid any further regressions 😅