Skip to content
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

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

nejch
Copy link
Contributor

@nejch nejch commented Feb 3, 2024

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 😅

src/release.ts Outdated Show resolved Hide resolved
@nejch nejch marked this pull request as ready for review February 3, 2024 11:13
@nejch
Copy link
Contributor Author

nejch commented Feb 3, 2024

@theoludwig @mstruebing let me know what you think! I tested this already in our corporate enviornment and should work fine.

theoludwig
theoludwig previously approved these changes Feb 3, 2024
Copy link
Member

@theoludwig theoludwig left a 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. 😄

@nejch
Copy link
Contributor Author

nejch commented Feb 3, 2024

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 😅

@theoludwig
Copy link
Member

theoludwig commented Feb 3, 2024

@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: nodejs/node#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. 😄

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.

@theoludwig theoludwig merged commit 97339d5 into editorconfig-checker:master Feb 3, 2024
3 checks passed
Copy link

github-actions bot commented Feb 3, 2024

🎉 This PR is included in version 5.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

mstruebing pushed a commit that referenced this pull request Mar 8, 2024
Co-authored-by: Théo LUDWIG <contact@theoludwig.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5.1.2 no longer supports HTTP proxy Builds v5.0.0-5.1.1 include lots of suspicious looking, unnecessary code
2 participants