sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.45k stars 1.89k forks source link

Add support for compression #9593

Closed evanlurvey closed 6 months ago

evanlurvey commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

The compression middleware is still broken while we wait for the fix to be reviewed: https://github.com/expressjs/compression/pull/183

benmccann commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

benmccann commented 1 year ago

@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 commented 1 year ago

In any case, thank you for unlocking the repository.

I did no such thing, either.

dougwilson commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

john-michael-murphy commented 9 months ago

@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 commented 6 months ago

@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 :shrug:

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

benmccann commented 6 months ago

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 https://github.com/lukeed/polka/pull/148 gets merged we could use it

john-michael-murphy commented 6 months ago

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

benmccann commented 6 months ago

@polka/compression has now been published

john-michael-murphy commented 5 months ago

@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 commented 5 months ago

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