Skip to content

fix(stylua-npm-bin): adjust axios config to work with proxy env variables#868

Merged
JohnnyMorganz merged 1 commit intoJohnnyMorganz:mainfrom
antoineauger:fix/axios-config-proxy
Jun 19, 2024
Merged

fix(stylua-npm-bin): adjust axios config to work with proxy env variables#868
JohnnyMorganz merged 1 commit intoJohnnyMorganz:mainfrom
antoineauger:fix/axios-config-proxy

Conversation

@antoineauger
Copy link
Copy Markdown
Contributor

@antoineauger antoineauger commented Jun 19, 2024

Hello,

First of all, thanks for providing this code formatter on so many distribution channels 🙏🏻

We wanted to pull the wrapper from npm so we added it to our yarn dependencies.

However, since we are behind a corporate proxy, the install just failed. More precisely, axios was not correctly resolving the GitHub redirects in addition to our proxy environment variables:

Some output from our GitLab CI job 
$ https_proxy=$PROXY no_proxy=$NO_PROXY yarn --frozen-lockfile
yarn install v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
warning [email protected]: The engine "pnpm" appears to be invalid.
[3/4] Linking dependencies...
warning "@commitlint/cli > @commitlint/load > [email protected]" has unmet peer dependency "@types/node@*".
warning "@commitlint/cli > @commitlint/load > [email protected]" has unmet peer dependency "typescript@>=4".
[4/4] Building fresh packages...
error /builds/<REDACTED>/node_modules/@johnnymorganz/stylua-bin: Command failed.
Exit code: 1
Command: node ./install.js
Arguments: 
Directory: /builds/<REDACTED>/node_modules/@johnnymorganz/stylua-bin
Output:
Downloading release from https://github.com/johnnymorganz/stylua/releases/download/v0.20.0/stylua-linux-x86_64.zip
Error fetching release: Maximum number of redirects exceeded
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

This is kind of a known issue, see axios/axios#2072 for instance.

This MR implements the fix proposed in axios/axios#4531 (comment).

If you are open to it, another option would be to get rid of axios. I see it is only used once so I could refactor the code and just switch to a native node fetch. I could use the undici - fetch for instance: we recently contributed upstream to editorconfig-checker to also bring back support for http proxies 😉

/cc @nejch @fgreinacher @bufferoverflow

🛠️ with ❤️ by Siemens

@antoineauger
Copy link
Copy Markdown
Contributor Author

antoineauger commented Jun 19, 2024

@JohnnyMorganz let me know what you think regarding this MR / a refactoring without axios, happy to contribute 🙏🏻

JohnnyMorganz
JohnnyMorganz approved these changes Jun 19, 2024
View reviewed changes
Copy link
Copy Markdown
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your interest and the PR! Fix looks pretty reasonable to me, happy to merge it in. I haven't tested it myself but the general smoke test still works so I'll take your word for it if the proxy setup works too 😁

Also happy to switch away from using axios if there is a benefit in doing so. I don't remember any particular reason it was chosen (other than being able to stream and pipe the result, but maybe other libraries can do that too). I just noticed in your diff that we already have a dependency on node-fetch, but don't seem to use it... If it doesn't really bring any improvements though then it's probably fine to just leave it be.

It might take a bit of time for this fix to make its way to a release unfortunately. I'm hoping to pick up some work and get a v2 release out, but that may take a while. Let me know if it would be beneficial for a patch release.

@JohnnyMorganz JohnnyMorganz merged commit 1ad8d05 into JohnnyMorganz:main Jun 19, 2024
@antoineauger
Copy link
Copy Markdown
Contributor Author

antoineauger commented Jun 20, 2024

Thanks for the quick merge @JohnnyMorganz!

I haven't tested it myself but the general smoke test still works so I'll take your word for it if the proxy setup works too

TBH, I was not expecting such a quick merge 😁.

I can still add a small test a-la https://github.com/TooTallNate/proxy-agents/blob/main/packages/proxy-agent/test/test.ts with a mock proxy server if you think it is necessary (seems a simpler solution than testing this with GitHub actions but maybe I'm wrong).

but maybe other libraries can do that too

Yes, see https://medium.com/deno-the-complete-reference/download-file-with-fetch-in-node-js-57dd370c973a for instance.

If it doesn't really bring any improvements though then it's probably fine to just leave it be.

After having went through the proxy-related axios issues, I do think switching for a more-adopted solution would only make the maintenance easier (and you already have the dep on node-fetch as you mentioned).

But I don't want to force your decision here, just let me know and I would be happy to refactor.

It might take a bit of time for this fix to make its way to a release unfortunately. I'm hoping to pick up some work and get a v2 release out, but that may take a while. Let me know if it would be beneficial for a patch release.

Alright, no pressure. I will implement a quick wget-based workaround on our end for the time being and I will watch out for any releases.

Thanks again for maintaining this tool 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohnnyMorganz JohnnyMorganz JohnnyMorganz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@antoineauger @JohnnyMorganz