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 support for compression #9593

Closed
evanlurvey opened this issue Apr 3, 2023 · 21 comments · Fixed by #11957
Closed

Add support for compression #9593

evanlurvey opened this issue Apr 3, 2023 · 21 comments · Fixed by #11957

Comments

@evanlurvey
Copy link

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

@Conduitry
Copy link
Member

For context: This was originally removed from the Node adapter in #5506 because of bugs in the underlying compression library's handling of streams. I don't know whether those have been fixed. Rich was against adding it as a boolean. Maybe it could be a config object, I don't know.

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.

@evanlurvey
Copy link
Author

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.

@evanlurvey
Copy link
Author

evanlurvey commented Apr 4, 2023

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

@benmccann
Copy link
Member

The compression middleware is still broken while we wait for the fix to be reviewed: expressjs/compression#183

@benmccann
Copy link
Member

Oh my goodness. I'm not sure we should be using compression at all. @dougwilson has removed the ability for anyone to file issues against it which is quite scary given that the library is currently in a completely broken state

@dougwilson
Copy link

@benmccann what are you talking about? I'm don't mind what module anyone uses, but I find it strage you are saying that

@dougwilson has removed the ability for anyone to file issues against it

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?

@dougwilson
Copy link

Perhaps if you are going to throw around such statements you should not use any modules I work on, for both our sakes.

@benmccann
Copy link
Member

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

@dougwilson
Copy link

In any case, thank you for unlocking the repository.

I did no such thing, either.

@dougwilson
Copy link

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.

@dougwilson
Copy link

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.

@dougwilson
Copy link

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.

@benmccann
Copy link
Member

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.

@fernandolguevara
Copy link
Contributor

if this take more time, we should celebrate the birthday of the fix... beers on me
🎂

@john-michael-murphy
Copy link

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

@benmccann
Copy link
Member

@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 ERR_STREAM_DESTROYED and ERR_STREAM_WRITE_AFTER_END in the docs for Node.js 10.x, but not in the docs for Node.js 8.x. I'm not sure how far back the project would be willing to drop support for older versions of Node, but Node 8 went EOL about 4.25 years ago on December 31, 2019.

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 🤷

Screenshot from 2024-03-03 08-00-56

@benmccann
Copy link
Member

I think the chances of the main compression library getting fixed are slim.

Screenshot from 2024-03-06 15-06-08

We should probably roll our own at some point. If lukeed/polka#148 gets merged we could use it

@john-michael-murphy
Copy link

Awesome, @benmccann! @polka/compression looks promising. I'd be happy to help take this on too if that package doesn't pan out.

@benmccann
Copy link
Member

@polka/compression has now been published

@john-michael-murphy
Copy link

john-michael-murphy commented Apr 1, 2024

@benmccann potentially stupid question, but is there something special we need to do to get this working? I'm seeing the latest version @polka/compression exhibit the same behavior as compression.

Here's what I've got:

import polka from 'polka';
import { handler } from '../build/handler.js';
import compression from '@polka/compression';

const app = polka();

app.use(compression());
app.use(handler);
app.listen(3000)

Without compression(), SvelteKit responses stream as expected.

@benmccann
Copy link
Member

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 compression in expressjs/compression#183 to @polka/compression? The code for @polka/compression lives here: https://github.com/lukeed/polka/blob/next/packages/compression/index.js

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