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

add ability to pass proxyOptions #688

Merged
merged 3 commits into from Oct 11, 2021

Conversation

yannickglt
Copy link
Contributor

@yannickglt yannickglt commented Jun 9, 2021

Replaces #459

http-proxy library that is used in http-server support options.

Fixes #214, fixes #246, fixes #638, fixes #719

@yannickglt
Copy link
Contributor Author

@thornjad already made the first review of #459.

I tried to bring the changes he expected, but I didn't get that two points:

@yannickglt
Copy link
Contributor Author

Is it possible to be reviewed or get an advice about this please?

@thornjad thornjad self-requested a review July 12, 2021 20:07
@thornjad thornjad added the minor version non-breaking, non-trivial change label Jul 12, 2021
@thornjad thornjad added this to the v0.13.0 milestone Jul 12, 2021
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

I think this looks good! Last couple things to do:

  • Add this option to doc/http-server.1
  • Tests! They don't need to be too complex since http-proxy already has its own tests, but we should make sure the options are getting through

@thornjad thornjad modified the milestones: v0.13.0, v14.0 Aug 6, 2021
@yannickglt
Copy link
Contributor Author

yannickglt commented Sep 15, 2021

I added the expected doc and an example (inspired from http-proxy) which tests proxy options and ensures the issue #214 is covered.

README.md Outdated Show resolved Hide resolved
bin/http-server Outdated Show resolved Hide resolved
@thornjad
Copy link
Member

thornjad commented Oct 5, 2021

Also, could you merge/rebase the current master in again? I don't expect conflicts, but it looks like this PR doesn't have the current tests setup, given that none are running on it

Nevermind, tests have run now

@dwjohnston
Copy link

dwjohnston commented Oct 6, 2021

This PR appears to solve #719

@thornjad thornjad modified the milestone: v14.0 Oct 11, 2021
@thornjad thornjad merged commit 35ff346 into http-party:master Oct 11, 2021
@yannickglt
Copy link
Contributor Author

Thanks a lot @thornjad

@yannickglt yannickglt deleted the http-proxy-options branch October 12, 2021 08:56
@thom4parisot
Copy link

Thanks @yannickglt, very helpful proposal!

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