Snowpack: Improve proxy support with http-proxy

[FredKSchott]

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!

[thisguychris]

hey @FredKSchott would love to work on this, can you provide a bit of guidance?

Probably nothing, our proxy was implemented very naively. See here for discussion to upgrade with http-proxy for better support! https://www.pika.dev/npm/snowpack/discuss/164#comment-6494

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?

[germanftorres]

Hi @thisguychris and @FredKSchott !

I kinda figured out how to fit http-proxy into snowpack dev command and made a very small PR #373 . Would love to hear some feedback as this is the first time I contribute to an oss project.

Thanks!

[fcamblor]

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 http-proxy may fix this issue, I will look at your PR @germanftorres as I'm very interested into my issue to be fixed (I had something different ready on my side, but it only fits my problem which is not the right direction I guess)

[stramel]

I will add your PR to my list to review today! Thanks for taking a stab at it!

[fcamblor]

@germanftorres FYI, my issue about content-encoding: gzip is fixed by your PR (=migrating to http-proxy), so that's definitely a 👍 from me here :)

[thisguychris]

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
So some improvements I made. I had SSL disabled and changeorigin to _true_. This would make https proxy work normally. The alternative is add our own certificate in snowpack which I don't think is a good idea since that's another thing we need to maintain.

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.

[thisguychris]

@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

[fcamblor]

@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 ?
I guess that would avoid too much dispersion.

[fcamblor]

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.

If we follow this reasoning, why then provide a proxy feature into snowpack if, as soon as we have to work a bit with proxy, we have to build our own middleware on top of it ?

[thisguychris]

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 👌

If we follow this reasoning, why then provide a proxy feature into snowpack if, as soon as we have to work a bit with proxy, we have to build our own middleware on top of it ?

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

[FredKSchott]

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.

[thisguychris]

@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?

[fcamblor]

@thisguychris the SSL support is only 2 lines of code (https://github.com/pikapkg/snowpack/pull/375/commits/6c69fe2b08cca50a25e6606c8def2e9192150e88), why not simply add it to #373 ?

I'm not preaching for having my commit integrated in it.
I'm preaching for simplicity and avoiding to have 2 different PRs for only 2 lines of code differences :-)

I didn't pushed another PR for my +56/-35 line changes :-)

[FredKSchott]

Lets get #373 merged, and then @thisguychris you can rebase onto master

[FredKSchott]

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
@fcamblor @stramel @germanftorres I'll create a new issue to discuss that better config format

[thisguychris]

@FredKSchott rebased :)