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

remove compression middleware from adapter-node output server #5506

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

Rich-Harris
Copy link
Member

This is an alternative to #5491. Adding a new option that adds or disables the middleware feels like a bad precedent, and we've often wondered if it really belongs here (rather than at the load balancer level, for example).

Since we clearly document how to use the exported handler with a custom server, I think it's safe to err on the side of simplicity and just get rid of it

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2022

🦋 Changeset detected

Latest commit: 90f1764

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I much prefer the original PR #5491. If we want to avoid the option, maybe we could merge this PR and then revert it after compression is fixed?

@Rich-Harris
Copy link
Member Author

Adding options for this sort of thing is just (sorry) bad API design. It's out of place, and having it be a boolean value assumes that we know best what options to set for threshold, level etc. It was a mistake to ever add the middleware

@benmccann
Copy link
Member

Sure, but it doesn't have to be a boolean. We can pass through the compression options by making it a build time option instead of runtime option

@fernandolguevara
Copy link
Contributor

I'm more on the idea of letting end-user play with middlewares each one considers necessary for each case (instrumentation, logging, compression, rate limit, tenant handling)

+1 to letting us configure middleware layer on build time @benmccann

@Rich-Harris Rich-Harris merged commit 5ed9a32 into master Jul 14, 2022
@Rich-Harris Rich-Harris deleted the remove-compression branch July 14, 2022 19:33
@Rich-Harris
Copy link
Member Author

We decided to go ahead with this for now, just so we have a working version. Whether and how we reinstate it is still up for debate

@coryvirok
Copy link
Contributor

Sad panda on this one. Anyone have a viable alternative to compression to use? I've tried creating a Vite plugin using compression but have run into the same hanging issue that this PR addresses.

Also, does anyone know why this only started happening with the Vite 3 release?

@benmccann
Copy link
Member

It's not related to Vite 3, but because SvelteKit started streaming recently and compression's streaming implementation is broken. We're waiting for the fix: expressjs/compression#183

@itssumitrai
Copy link

Is there some alternative which could be applied in mean time to prevent the node hanging issue ? Most of us who use Sveltekit on production environment are currently completely blocked with the streaming & compression change. We have to keep Kit pinned at 1.0.0-next.378

@benmccann
Copy link
Member

Compression has been removed, so if you update to the latest it will work. You can do compression in your load balancer or a hook if you need. We can consider adding compression back after it's been fixed

@coryvirok
Copy link
Contributor

If someone can figure out a way to implement that hook, I'd be interested. It's pretty crazy how many of the frameworks use the compression module and just how long it's been broken for.

I host on Google Cloud Run which doesn't have an option to add compression so my users are sort of stuck with large payloads till there's a workaround.

@fernandolguevara
Copy link
Contributor

fernandolguevara commented Jul 23, 2022

@coryvirok an alternative could be to wrap the sveltekit handler (generated by the adapter) with a custom server implementation and in this server u need to enable the comoression middleware.

below an example, anyway keep in mind that there is a bug on the compression lib

/* global ENV_PREFIX */

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

export const path = env('SOCKET_PATH', false);
export const host = env('HOST', '0.0.0.0');
export const port = env('PORT', !path && '3000');
export const compression_enabled = env('COMPRESSION_ENABLED', 'true') === 'true';
export const compression_threshold = parseInt(env('COMPRESSION_THRESHOLD', '0'), 10);

if (isNaN(compression_threshold) || compression_threshold < 0) {
	throw Error(`${ENV_PREFIX}COMPRESSION_THRESHOLD should be a positve number`);
}

const compression_middleware = compression_enabled
	? compression({ threshold: compression_threshold })
	: undefined;

const middlewares = [compression_middleware, handler].filter(Boolean);

// https://github.com/lukeed/polka/issues/173
// @ts-ignore - nothing we can do about so just ignore it
const server = polka().use(...middlewares);

server.listen({ path, host, port }, () => {
	console.log(`Listening on ${path ? path : host + ':' + port}`);
});

export { server };

@lkj4

This comment was marked as off-topic.

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

Successfully merging this pull request may close these issues.

None yet

6 participants