Closed nejch closed 5 months ago
@theoludwig @mstruebing let me know what you think! I tested this already in our corporate enviornment and should work fine.
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 :sweat_smile:
@nejch I refactored the code, to avoid the any
type, by importing RequestInit
type from undici
.
About using Node.js built-in fetch for ProxyAgent
, it is not yet possible. So we must still use undici
, but apparently in future Node.js versions, we will maybe be able to, there is an open issue here: https://github.com/nodejs/node/issues/43187
When undici
will directly be exposed with Node.js, we should be able to use it, and reduce again the package size.
But for now, it is good enough, 1MB. :smile:
Also apologies if I'm mixing styles a bit here. just noticed and pushed, maybe needs more linting for people like me 😅
No worries, I formatted the code with Prettier, and now, it is also verified with CI.
:tada: This PR is included in version 5.1.3 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
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. https://github.com/redwoodjs/redwood/pull/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 :sweat_smile: