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
Improve proxy support with http-proxy #371
Comments
hey @FredKSchott would love to work on this, can you provide a bit of guidance?
I see that we already have proxy enable and we want to move it to use http-proxy? May I know where's that part of proxy implementation in the code? Does snowpack have some sort of community messaging, aside from https://www.pika.dev/npm/snowpack/discuss Like a discord or slack server? Or is it okay to DM you directly in Twitter if I get stuck? |
Hi @thisguychris and @FredKSchott ! I kinda figured out how to fit Thanks! |
FYI I have currently an issue about proxy due to "content-encoding: gzip" : see https://www.pika.dev/npm/snowpack/discuss/259 I imagine that |
I will add your PR to my list to review today! Thanks for taking a stab at it! |
@germanftorres FYI, my issue about |
Hey @germanftorres, I hope you don't mind, I've created a new PR with your commit in it for proper attribution, since I don't have access to your fork. @FredKSchott @germanftorres This would now work on both POST and GET methods, as mentioned here This will also fix @fcamblor issue as I've tested, it forwards the encoding headers correctly: Some room for improvements, @FredKSchott do you think we need to pass the options for the the http-proxy in order for other people to extend it? As it currently stands, if someone wants to extend it, they can just make a new server and just pass it in the proxy and they can do whatever they want with that http-server. So I personally don't think we need to make it more complicated as it is. |
@fcamblor just saw your commit on @germanftorres fork, so some feedback. I'm not sure if @FredKSchott wanted to make it configurable, as it just complicates snowpack. As it currently stand, someone can just make a proxy server and pass this in snowpack if they wanted to add more configurations. Also I've tested @germanftorres it doesn't work on proxied SSL |
@thisguychris why not propose a PR on @germanftorres fork (which evolved with an improvement from my side .. he was very quick to merge it already :-) and I didn't had any visibility issue) ? Also, I guess that #373 has a lot of interesting comments in it, why not close #375 and continue working/discussing on #373 ? |
If we follow this reasoning, why then provide a |
hey @fcamblor , When we work in our company, we usually just push the fork, since I don't have access to @germanftorres I just made a new PR, with his own commits, giving him proper attribution. Now regarding with your improvements, Let us just wait for @FredKSchott take, or any maintainer. I can easily pull your commit for proper attribution, as long as if we agree that's the better approach 👌
The thing is, having this default fixes the majority of usecase. I feel like that adding much complexity would be a disservice to snowpack's simplicity. Which I think I share the same sentiments as @stramel here |
Thanks for getting involved everyone! Let's get https://github.com/pikapkg/snowpack/pull/373/files merged first, which would fix a bunch of bugs in our current implementation and match our current support level. Then we can look into followup PRs to fix specific issues, add configuration, etc. |
@FredKSchott some problem with #373, it doesn't work with SSL, so if you want @fcamblor commit, I can pull it up now and update #375? |
@thisguychris the SSL support is only 2 lines of code (6c69fe2), why not simply add it to #373 ? I'm not preaching for having my commit integrated in it. I didn't pushed another PR for my +56/-35 line changes :-) |
Lets get #373 merged, and then @thisguychris you can rebase onto master |
#373 has been merged, thanks for your help on that @germanftorres. @thisguychris If you can rebase your changes onto master, I can take a look at #375 |
@FredKSchott rebased :) |
Original Discussion: https://www.pika.dev/npm/snowpack/discuss/164#comment-6494
/cc @thisguychris, @FredKSchott, @opus131, @hn3000, @stramel, @germanftorres, @ToucheSir
Move to http-proxy for
proxy
build scripts, which would solve a whole host of issues with the current implementation.@thisguychris would love your help if you're still game to tackle. Just let me know how I can help!
The text was updated successfully, but these errors were encountered: