-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add support for compression #9593
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
Comments
For context: This was originally removed from the Node adapter in #5506 because of bugs in the underlying I do think, however, that this should be adapter-specific, and not built into the main framework. Each deployment target is presumably going to have its own way of dealing with compressing on the fly built into the service itself, or will have a way it wants us to deal with files that have been compressed once at build time during prerendering. In the meantime, the recommendation is to do this compression at another layer of your service, like you would be handling SSL termination. |
Those are some great points. Probably more specific to the Node adapter anyways. But like you said could be handled in other layers. My particular use case is actually for a prototype my group is building but the network is so slow. I was hoping to enable compression and just hit the node service directly. Since it wont ever see the light of day I didn't want to spend the time setting up another layer to do such. |
For anyone else that just wants a quick hack for compression check out this golang snip I created. Feel free to use, modify, whatever, under any means. package main
import (
"net/http"
"net/http/httputil"
"net/url"
"github.com/klauspost/compress/gzhttp"
)
func NewProxy(host string) *httputil.ReverseProxy {
url, err := url.Parse(host)
if err != nil {
panic(err)
}
return httputil.NewSingleHostReverseProxy(url)
}
func main() {
proxy := NewProxy("http://localhost:3000")
if err := http.ListenAndServe("0.0.0.0:8080", gzhttp.GzipHandler(proxy)); err != nil {
panic(err)
}
} For my dataset it reduced the response from 3MB to sub 400kb and thats with streaming from sveltekit |
The |
Oh my goodness. I'm not sure we should be using |
@benmccann what are you talking about? I'm don't mind what module anyone uses, but I find it strage you are saying that
I don't see any limits on that module and I'm not even the only admin on it. Seems quite unfair to accuse someone of doing something. Can you provide what evidence you have that I specifically did what you are claiming? |
Perhaps if you are going to throw around such statements you should not use any modules I work on, for both our sakes. |
@dougwilson apologies if it was wrong of me to assume you locked it. No one else had more than a single commit to the repo since 2014, so it certainly looked like you were the only maintainer of the repo. If someone else is locking everyone else out of the repo without your knowledge perhaps it makes sense to review the audit log and determine if there is someone who should be coordinating with you or removed as an administrator. Or perhaps it was just some temporary GitHub fluke saying that the repository was locked to external contributors when the settings weren't set in that way. In any case, thank you for unlocking the repository. I see that we can now comment and file issues against the repo once again, which I appreciate. |
I did no such thing, either. |
I made no changes to the repo besides looking at it when I saw your rude public comment. Please do not comment in the repo, though, as I do not want to associate with you. |
I am very stressed out right now with a bunch of stupid security reports and no help. I don't appreciate getting random flack on top of it via github mentions. |
Perhaps in the future you will consider politely emailed whoever it is you think did it, asking if it was a mistake or something going on insteqd of blasting them in public as the first method. |
PRs and issues were giving a message saying that an administrator had restricted them to organization members only and GitHub wasn't showing me an email address, website, Twitter, or other contact on your profile, so I didn't have a lot of other options in terms of method of contact, but I will be more careful with my wording in the future. I'll also respect your wishes and avoid the repository. I certainly understand that open source can be a thankless job and the pressures of dealing with security issues. Thank you for your work as an open source maintainer and sorry for adding stress to your day. |
if this take more time, we should celebrate the birthday of the fix... beers on me |
@fernandolguevara, thank you so much for opening a fix for this. It would be so great to get this working! I wanted to ask a question that doug posed here. Do you know if dropping support for older versions of node would allow us to avoid the hack that's currently blocking this PR? |
@fernandolguevara thanks for putting the PR together. I see it's stalled out, so I thought I'd provide some info related to the open question there. I see Of course I don't know if you can currently continue work on your PR as most users are currently blocked from the repository and I don't know whether having an open PR that hasn't been merged is enough for GitHub to consider you a contributor to the repo. It's a bit sad to see no one can file issues against such a widely used library. I understand that open source can be a thankless job, but it feels more responsible to locate additional maintainers rather than blocking all issues from being filed. Oh well, to each their own I guess 🤷 |
I think the chances of the main We should probably roll our own at some point. If lukeed/polka#148 gets merged we could use it |
Awesome, @benmccann! @polka/compression looks promising. I'd be happy to help take this on too if that package doesn't pan out. |
|
@benmccann potentially stupid question, but is there something special we need to do to get this working? I'm seeing the latest version Here's what I've got:
Without |
Hmm, that's a bummer. I sure thought it supported streaming, but perhaps it doesn't yet. Luke has been great about merging PRs though, so if anyone's able to send a PR with a fix I feel it's likely it will get reviewed. Perhaps you or @fernandolguevara would like to try porting over the pending PR from |
Describe the problem
I have a few sites that are pretty heavy in the data, but it's not random data so its a great candidate for compression. However looking through the docs I don't see anything about how to enable it.
Describe the proposed solution
I would like to see compression enabled and on by default with a way to opt out if a user wants to. I think we should be compressing anything that hits the wire.
Alternatives considered
No response
Importance
would make my life easier
Additional Information
No response
The text was updated successfully, but these errors were encountered: