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 proxy implementation for https over http #1086

Closed
wants to merge 3 commits into from

Conversation

kumarrishav
Copy link

@kumarrishav kumarrishav commented Apr 12, 2022

#1067 (comment)

other solution could to use https-proxy-agent instead of tunnel module
https://stackoverflow.com/questions/43240483/how-to-use-axios-to-make-an-https-call

@apollo-cla
Copy link

@kumarrishav: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Apr 12, 2022

👷 Deploy request for apollo-cli-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit cbfc430

@kumarrishav
Copy link
Author

kumarrishav commented Apr 12, 2022

Found another issue: whatever code is in the GitHub doesn't match with the one in npm registry
see install function
here it is taking args (which is correct)
Github: https://github.com/EverlastingBugstopper/binary-install/blob/main/packages/binary-install/index.js#L54

but npm published one seems like old
https://unpkg.com/browse/[email protected]/index.js

can you look into it @EverlastingBugstopper seem like you are the owner of that module
CC: @farawaysouthwest

seems like that code got landed after 0.1.1 publish

@kumarrishav
Copy link
Author

@EverlastingBugstopper @farawaysouthwest can you take some time to look into it? Thanks

@kumarrishav
Copy link
Author

any update on this?

@EverlastingBugstopper
Copy link
Contributor

Hi @kumarrishav - I was out on vacation for a bit and had some other things come up so I have not had time to look at this until now.

You're correct that binary-install is a bit behind the times, there are a few things I want to land there before publishing a new version and I'm working on that now. We're planning a release next week and I'll try to make sure this gets in by then.

@EverlastingBugstopper
Copy link
Contributor

i've opened this PR, going to try to get a few reviews on it and merge it up, likely do this as well and then cut a release which we can use here

@EverlastingBugstopper
Copy link
Contributor

OK I've gone ahead and published v1.0.0 of binary-install which now includes accepting those fetchOptions.

@kumarrishav or @farawaysouthwest can we get that https/http snippet included in a new version of configureProxy?

@kumarrishav
Copy link
Author

kumarrishav commented Apr 22, 2022

Thanks for publishing that.

Also, Not a ts expert here but I have raised the PR: https://github.com/farawaysouthwest/axios-proxy-builder/pull/1/files

Feel free to close farawaysouthwest/axios-proxy-builder#1 and raise a new one ts one (if you prefer)

once we have released a new version of Axios-proxy-builder. then I can clean that code from this PR and update binary-install as well

@farawaysouthwest @EverlastingBugstopper

@farawaysouthwest
Copy link
Contributor

Working up a new pr based on yours @kumarrishav, there were some TS related issues.

@farawaysouthwest
Copy link
Contributor

proxybuilder 0.1.2 published to npm, working with @kumarrishav, we believe this is ready to go.

@kumarrishav
Copy link
Author

closing this PR as logic moved to https://github.com/farawaysouthwest/axios-proxy-builder/

@kumarrishav
Copy link
Author

kumarrishav commented Apr 22, 2022

all we need now is to update binary-install to ^1 and release

@kumarrishav
Copy link
Author

kumarrishav commented Apr 22, 2022

never mind you already did. I think we are ready to go

@kumarrishav
Copy link
Author

can we publish now new version of @apollo/rover

@EverlastingBugstopper
Copy link
Contributor

there are a few things that I'm working on before I'll cut a new release but we should have one soon, sit tight!

@kumarrishav
Copy link
Author

@EverlastingBugstopper thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants